RESOLVED FIXED 184706
A put is not an ExistingProperty put when we transition a structure because of an attributes change
https://bugs.webkit.org/show_bug.cgi?id=184706
Summary A put is not an ExistingProperty put when we transition a structure because o...
JF Bastien
Reported 2018-04-17 13:40:05 PDT
tryCachePutByID checks that the baseValue's structure is the same as the structure that's passed in. That's not necessarily true for some put operations, because they used to be able to do something like putDirect between the time the structure is obtained from the baseValue and the time tryCachePutByID is called. Instead we should delay getting the structure from baseValue until after putDirect is called, so that a change caused by putDirect doesn't cause confusion.
Attachments
patch (6.90 KB, patch)
2018-04-17 13:44 PDT, JF Bastien
saam: review-
saam: commit-queue-
patch (5.12 KB, patch)
2018-04-17 14:29 PDT, JF Bastien
saam: review+
patch (6.82 KB, patch)
2018-04-17 14:55 PDT, JF Bastien
no flags
JF Bastien
Comment 1 2018-04-17 13:41:11 PDT
JF Bastien
Comment 2 2018-04-17 13:44:53 PDT
Saam Barati
Comment 3 2018-04-17 13:50:14 PDT
Comment on attachment 338145 [details] patch I don't see how this doesn't disable caching of all transitions.
Filip Pizlo
Comment 4 2018-04-17 13:50:48 PDT
Comment on attachment 338145 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=338145&action=review This change is wrong. It disables transition inline caching. That may fix a bug in transition inline caching, but it's not a correct fix. > JSTests/ChangeLog:3 > + put by ID should be aware of structure transitions This title is super scary and probably incorrect. put_by_id (the bytecode) and PutById (the DFG node) both know about structure transitions. They know *a lot* about them. So, you should make this title a lot more specific. > Source/JavaScriptCore/jit/JITOperations.cpp:528 > - Structure* structure = baseValue.isCell() ? baseValue.asCell()->structure(*vm) : nullptr; > baseValue.putInline(exec, ident, value, slot); > RETURN_IF_EXCEPTION(scope, void()); > > if (accessType != static_cast<AccessType>(stubInfo->accessType)) > return; > - > + > + Structure* structure = baseValue.isCell() ? baseValue.asCell()->structure(*vm) : nullptr; This is way wrong. The transition inline cache needs to know about the structure before the transition. > Source/JavaScriptCore/jit/JITOperations.cpp:556 > - Structure* structure = baseValue.isCell() ? baseValue.asCell()->structure(*vm) : nullptr; > baseValue.putInline(exec, ident, value, slot); > RETURN_IF_EXCEPTION(scope, void()); > > if (accessType != static_cast<AccessType>(stubInfo->accessType)) > return; > - > + > + Structure* structure = baseValue.isCell() ? baseValue.asCell()->structure(*vm) : nullptr; Ditto. > Source/JavaScriptCore/jit/JITOperations.cpp:582 > - > - Structure* structure = baseObject->structure(*vm); > + > baseObject->putDirect(*vm, ident, value, slot); > > if (accessType != static_cast<AccessType>(stubInfo->accessType)) > return; > - > + > + Structure* structure = baseObject->structure(*vm); Ditto. > Source/JavaScriptCore/jit/JITOperations.cpp:608 > - > - Structure* structure = baseObject->structure(*vm); > + > baseObject->putDirect(*vm, ident, value, slot); > > if (accessType != static_cast<AccessType>(stubInfo->accessType)) > return; > - > + > + Structure* structure = baseObject->structure(*vm); Ditto.
JF Bastien
Comment 5 2018-04-17 14:29:48 PDT
Saam Barati
Comment 6 2018-04-17 14:31:56 PDT
Comment on attachment 338149 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=338149&action=review > JSTests/ChangeLog:3 > + A property isn't existing if we transition a structure This bug title is hard to parse, I'd suggest something like: A put is not an ExistingProperty put when we transition a structure because of an attributes change
Saam Barati
Comment 7 2018-04-17 14:32:45 PDT
Comment on attachment 338149 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=338149&action=review > JSTests/stress/put-by-id-direct-strict-transition.js:11 > + const obj = { > + set hello(ignored) {}, > + [theglobal]: 0 > + }; Please also add a test for obj = { [theglobal]:0, set hello(ignored){} };
JF Bastien
Comment 8 2018-04-17 14:55:16 PDT
JF Bastien
Comment 9 2018-04-17 15:29:49 PDT
Confirmed perf neutral: Geomean of preferred means: <scaled-result> 18.2710+-0.1104 18.1661+-0.0614 might be 1.0058x faster
WebKit Commit Bot
Comment 10 2018-04-17 16:48:06 PDT
Comment on attachment 338153 [details] patch Clearing flags on attachment: 338153 Committed r230740: <https://trac.webkit.org/changeset/230740>
WebKit Commit Bot
Comment 11 2018-04-17 16:48:07 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.