WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
patch
(5.12 KB, patch)
2018-04-17 14:29 PDT
,
JF Bastien
saam
: review+
Details
Formatted Diff
Diff
patch
(6.82 KB, patch)
2018-04-17 14:55 PDT
,
JF Bastien
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
JF Bastien
Comment 1
2018-04-17 13:41:11 PDT
<
rdar://problem/38871451
>
JF Bastien
Comment 2
2018-04-17 13:44:53 PDT
Created
attachment 338145
[details]
patch
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
Created
attachment 338149
[details]
patch
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
Created
attachment 338153
[details]
patch
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.
Top of Page
Format For Printing
XML
Clone This Bug