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.
<rdar://problem/38871451>
Created attachment 338145 [details] patch
Comment on attachment 338145 [details] patch I don't see how this doesn't disable caching of all transitions.
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.
Created attachment 338149 [details] patch
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
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){} };
Created attachment 338153 [details] patch
Confirmed perf neutral: Geomean of preferred means: <scaled-result> 18.2710+-0.1104 18.1661+-0.0614 might be 1.0058x faster
Comment on attachment 338153 [details] patch Clearing flags on attachment: 338153 Committed r230740: <https://trac.webkit.org/changeset/230740>
All reviewed patches have been landed. Closing bug.