Replace ASSERT_NOT_REACHED with RELEASE_ASSERT_NOT_REACHED in JSC

• Component: New Bugs
• Status: RESOLVED FIXED

[Oliver Hunt]

Replace ASSERT_NOT_REACHED with RELEASE_ASSERT_NOT_REACHED in JSC

[Oliver Hunt]

Created attachment 184311 [details]
Patch

[Mark Hahnenberg]

Comment on attachment 184311 [details]
Patch

r=me

[Oliver Hunt]

Committed r140594: <http://trac.webkit.org/changeset/140594>

[Darin Adler]

Comment on attachment 184311 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184311&action=review

> Source/JavaScriptCore/ChangeLog:8
> +        Mechanical change with no performance impact.

What guarantees that adding this extra code in code paths we don’t execute has no performance impact?

[Oliver Hunt]

(In reply to comment #4)
> (From update of attachment 184311 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184311&action=review
> 
> > Source/JavaScriptCore/ChangeLog:8
> > +        Mechanical change with no performance impact.
> 
> What guarantees that adding this extra code in code paths we don’t execute has no performance impact?

Sorry, i don't understand what you're asking here.

I went through every ASSERT in JSC and applied the following logic:
1.  If this assertion failing won't cause badness (i.e. it's not meant to happen but we can handle it), then no change
2. If the assertion is in a hot path, then no change
3. Otherwise if the assertion failing would indicate relatively obvious incorrect behaviour w.r.t memory access, execution, reuse, etc then it becomes a RELEASE_ASSERT

I tried to be as aggressive as possible without impacting performance, and without touching any of the core YARR interpreter loop.  None of the benchmarks show any regression following these changes.

I'm planning to beat YARR with the RELEASE_ASSERT stick as a separate patch because there are already a bunch of similar release mode-y things that YARR does, so working out what things become release assertions and what become checked<> is less mechanical.