Bug 101299 - DFG should not fall down to patchable GetById just because a prototype had things added to it
Summary: DFG should not fall down to patchable GetById just because a prototype had th...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-05 21:10 PST by Filip Pizlo
Modified: 2012-11-05 23:51 PST (History)
7 users (show)

See Also:


Attachments
the patch (3.93 KB, patch)
2012-11-05 21:18 PST, Filip Pizlo
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2012-11-05 21:10:18 PST
Currently StructureTransitionWatchpoints have a BadCache exit profile.  This is often a good thing - for example if we turned the CheckStructure of the base of an access into a StructureTransitionWatchpoint, and then the watchpoint was fired because there was an unexpected side-effect.  It's fair to assume that side effect might happen again, and so we ought to be conservative.

But if we set a watchpoint on the structure of the prototype, and that watchpoint fired, then we shouldn't necessarily fall off into using a patchable GetById.  Most likely, the reason why the watchpoint fired was just a modification to the prototype.  If such modifications happen frequently then the baseline JIT's get_by_id will go polymorphic and/or have a high slow path count - so the fact that the watchpoint fired doesn't mean we can't set the watchpoint again on the next recompile, if everything else tells us it is safe to do so.
Comment 1 Filip Pizlo 2012-11-05 21:18:25 PST
Created attachment 172483 [details]
the patch
Comment 2 Geoffrey Garen 2012-11-05 21:32:05 PST
Comment on attachment 172483 [details]
the patch

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

r=me

> Source/JavaScriptCore/ChangeLog:8
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

Boo.
Comment 3 Filip Pizlo 2012-11-05 21:42:31 PST
(In reply to comment #2)
> (From update of attachment 172483 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172483&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/ChangeLog:8
> > +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).
> 
> Boo.

Oops.  Forgot to save my ChangeLog.  Text is:

        DFG should not fall down to patchable GetById just because a prototype had things added to it
        https://bugs.webkit.org/show_bug.cgi?id=101299

        Reviewed by NOBODY (OOPS!).

        This looks like a slight win on V8v7 and SunSpider.

        * bytecode/DFGExitProfile.h:
        (JSC::DFG::exitKindToString):
        * dfg/DFGSpeculativeJIT64.cpp:
        (JSC::DFG::SpeculativeJIT::compile):
Comment 4 Filip Pizlo 2012-11-05 23:51:06 PST
Landed in http://trac.webkit.org/changeset/133567