Bug 107736 - Replace ASSERT_NOT_REACHED with RELEASE_ASSERT_NOT_REACHED in JSC
Summary: Replace ASSERT_NOT_REACHED with RELEASE_ASSERT_NOT_REACHED in JSC
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Oliver Hunt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-23 14:20 PST by Oliver Hunt
Modified: 2013-01-24 15:12 PST (History)
0 users

See Also:


Attachments
Patch (100.92 KB, patch)
2013-01-23 14:22 PST, Oliver Hunt
mhahnenberg: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2013-01-23 14:20:41 PST
Replace ASSERT_NOT_REACHED with RELEASE_ASSERT_NOT_REACHED in JSC
Comment 1 Oliver Hunt 2013-01-23 14:22:11 PST
Created attachment 184311 [details]
Patch
Comment 2 Mark Hahnenberg 2013-01-23 14:24:59 PST
Comment on attachment 184311 [details]
Patch

r=me
Comment 3 Oliver Hunt 2013-01-23 14:26:33 PST
Committed r140594: <http://trac.webkit.org/changeset/140594>
Comment 4 Darin Adler 2013-01-24 12:06:09 PST
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?
Comment 5 Oliver Hunt 2013-01-24 15:12:09 PST
(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.