Bug 163305 – We should have a way of profiling when a get_by_id is pure and to emit a PureGetById in the DFG/FTL

• Status: REOPENED
• Component: JavaScriptCore

[Saam Barati]

Fil just proposed this idea to me, and it could end up helping quite a bit for certain types of programs.
We could profile when a get_by_id never does effectful operations. If so, we can compile it in the DFG
as a PureGetById, which will cause the codeblock to invalidate if it is ever about to execute something
effectful. This could be a huge win because it could enable CSE around PureGetById and even code motion of PureGetById.

[Saam Barati]

Created attachment 291314 [details]
patch for landing

[Saam Barati]

Comment on attachment 291314 [details]
patch for landing

Ooops, wrong bug.

[Saam Barati]

I'm going to start work on this now.

[Saam Barati]

Created attachment 291984 [details]
WIP

It at least runs one program.

[Saam Barati]

Created attachment 292012 [details]
WIP

It can run many tests, but not all of them.

[Saam Barati]

Created attachment 292027 [details]
WIP

Passes more tests, however, I'm now dubious in many implementations of getOwnPropertySlot when we're using VMInquiry (note that this can mean the tryGetById is also broken for such cases). Just one example, JSFunction::getOwnPropertySlot may transition the structure of the thing we're inquiring about. This obviously goes against what we're asking when we do VMInquiry.

[Saam Barati]

Created attachment 292271 [details]
WIP

Pretty close to done, but there is a octane/regexp regression I want to fix.

[WebKit Commit Bot]

Attachment 292271 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:73:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 292447 [details]
patch

This patch is ready for review in terms of it will be good to get feedback from people on it.

However, there is still one more change I want to make which is to invalidate() a CodeBlock whenever the getOwnPropertySlot calls into some NS Plugin code's getOwnPropertySlot implementation. I will probably create some API on PropertySlot that is called willDoSideEffects() or something like that since we expect a call to getOwnPropertySlot(VMInquiry) not to do user observable side effects, but the plugin code probably doesn't adhere to this rule.

[Saam Barati]

Created attachment 292454 [details]
patch

rebased

[WebKit Commit Bot]

Attachment 292454 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:73:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 292469 [details]
patch

fix the build after renaming things

[WebKit Commit Bot]

Attachment 292469 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:73:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 292469 [details]
patch

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

> Source/JavaScriptCore/dfg/DFGClobberize.h:515
> +        write(JSCell_typeInfoType);
> +        write(JSCell_indexingType);

These should be deleted I believe since I don't think getOwnPropertySlot should do this.

> Source/JavaScriptCore/dfg/DFGClobberize.h:524
> +        write(Butterfly_publicLength);
> +        write(Butterfly_vectorLength);

I also don't think this is strictly accurate. I was trying to model the allocation of a butterfly, but this seems like it'll prevent CSE of things that read vectorLength/publicLength across a PureGetById. Also, it seems like it would be very weird for any getOwnPropertySlots to change the values of these fields.

[Saam Barati]

Comment on attachment 292469 [details]
patch

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:214
> +        getByIdFunction = operationGetByIdOptimize;

This is also obviously wrong. It should be operationPureGetByIdOptimize.

[Saam Barati]

Comment on attachment 292469 [details]
patch

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

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:214
>> +        getByIdFunction = operationGetByIdOptimize;
> 
> This is also obviously wrong. It should be operationPureGetByIdOptimize.

I'll add some tests for this since this would obviously break some programs and no tests caught it.

[Saam Barati]

Created attachment 292633 [details]
patch

[WebKit Commit Bot]

Attachment 292633 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:73:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

> Source/JavaScriptCore/ChangeLog:15
> +        effects. If it realizes that it must perform side effects, it will perform
> +        the side effect, but also invalidate the CodeBlock that compiled it,
> +        which will cause us to exit once we return back to the compiled code.
> +        This allows us to have stricter clobberize rules for PureGetById which
> +        model how getOwnPropertySlot(VMInquiry) behaves. This means that PureGetByIds

Is an invalidated CodeBlock guaranteed to exit forward (and not re-execute part or all of the get_by_id)?

> Source/JavaScriptCore/jit/JITOperations.cpp:82
> +            exec->codeBlock()->jettison(Profiler::JettisonDueToPureGetByIdEffects);
> +            return baseValue.get(exec, uid);

Do we need to update the StructureStubInfo so that we know not to do the PureGetById optimization in the future? Alternatively, could the optimizing path check whether we've ever jettisoned with this reason before?

[Saam Barati]

(In reply to comment #19)
> > Source/JavaScriptCore/ChangeLog:15
> > +        effects. If it realizes that it must perform side effects, it will perform
> > +        the side effect, but also invalidate the CodeBlock that compiled it,
> > +        which will cause us to exit once we return back to the compiled code.
> > +        This allows us to have stricter clobberize rules for PureGetById which
> > +        model how getOwnPropertySlot(VMInquiry) behaves. This means that PureGetByIds
> 
> Is an invalidated CodeBlock guaranteed to exit forward (and not re-execute
> part or all of the get_by_id)?
Yeah, that's why we actually finish performing the get() operation. We invalidate the CodeBlock, which will mean we OSR exit at the InvalidationPoint that follows the PureGetById. It's guaranteed to be followed by an InvalidationPoint because we claim it writes to Watchpoint_fire.
So you'll get DFG code like this:
x: PureGetById(...)
y: MovHint(@x, locX)
z: InvalidationPoint // Where we will exit

> 
> > Source/JavaScriptCore/jit/JITOperations.cpp:82
> > +            exec->codeBlock()->jettison(Profiler::JettisonDueToPureGetByIdEffects);
> > +            return baseValue.get(exec, uid);
> 
> Do we need to update the StructureStubInfo so that we know not to do the
> PureGetById optimization in the future? Alternatively, could the optimizing
> path check whether we've ever jettisoned with this reason before?

I've given this some thought, and we probably should try to update the baseline version to reflect that we did effects.
The reason I didn't write code to do this is that if we do jettison, it's likely we will
to the side-effecting thing again, in which case, we won't ever compile it as a PureGetById.
However, it's possible we do a side-effecting thing once, and then never again, and in which
case, it would be profitable to compile it as a PureGetById. However, if we think this is common,
it's probably better to simply OSR exit in such circumstances and not jettison. For now, I'll
add code which will try to find the baseline JITs stub and write to its didSideEffects bit.

[Keith Miller]

Comment on attachment 292633 [details]
patch

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

r=me with some code de-duplification.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4274
> +    case PureGetById: {

I think it would be better if you changed to just use a JSValueRegs. Then you wouldn't need a 32-bit and 64-bit version. You could probably modify compileTryGetById to work with PureGetById.

[Geoffrey Garen]

> For now, I'll
> add code which will try to find the baseline JITs stub and write to its
> didSideEffects bit.

I think this is probably the right balance. I agree that varying a property's purity is probably rare. So, the defense we need is just defense against pathological behavior (compile, jettison, compile, jettison). Backing off to impure get_by_id is a small price to pay, and way way way less costly than continuing to jettison.

[Saam Barati]

(In reply to comment #21)
> Comment on attachment 292633 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=292633&action=review
> 
> r=me with some code de-duplification.
> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4274
> > +    case PureGetById: {
> 
> I think it would be better if you changed to just use a JSValueRegs. Then
> you wouldn't need a 32-bit and 64-bit version. You could probably modify
> compileTryGetById to work with PureGetById.

I'll keep the function separate from compileTryGetById, but I'll create a version that works both with 32 and 64 bit platforms.

Thanks for the review.

[Saam Barati]

Created attachment 293317 [details]
patch for landing

[WebKit Commit Bot]

Attachment 293317 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:74:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:84:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 293319 [details]
patch for landing

fix efl build

[Saam Barati]

Created attachment 293320 [details]
patch for landing

more efl build fix

[WebKit Commit Bot]

Attachment 293320 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:74:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:84:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 293320 [details]
patch for landing

Clearing flags on attachment: 293320

Committed r208117: <http://trac.webkit.org/changeset/208117>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Reverted r208117 and r208160 for reason:

Regressed Speedometer by >1.5%

Committed r208588: <http://trac.webkit.org/changeset/208588>