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.
Created attachment 291314 [details] patch for landing
Comment on attachment 291314 [details] patch for landing Ooops, wrong bug.
I'm going to start work on this now.
Created attachment 291984 [details] WIP It at least runs one program.
Created attachment 292012 [details] WIP It can run many tests, but not all of them.
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.
Created attachment 292271 [details] WIP Pretty close to done, but there is a octane/regexp regression I want to fix.
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.
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.
Created attachment 292454 [details] patch rebased
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.
Created attachment 292469 [details] patch fix the build after renaming things
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.
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.
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.
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.
Created attachment 292633 [details] patch
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.
> 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?
(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.
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.
> 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.
(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.
Created attachment 293317 [details] patch for landing
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.
Created attachment 293319 [details] patch for landing fix efl build
Created attachment 293320 [details] patch for landing more efl build fix
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.
Comment on attachment 293320 [details] patch for landing Clearing flags on attachment: 293320 Committed r208117: <http://trac.webkit.org/changeset/208117>
All reviewed patches have been landed. Closing bug.
Reverted r208117 and r208160 for reason: Regressed Speedometer by >1.5% Committed r208588: <http://trac.webkit.org/changeset/208588>