Bug 184706 - A put is not an ExistingProperty put when we transition a structure because of an attributes change
Summary: A put is not an ExistingProperty put when we transition a structure because o...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: JF Bastien
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-04-17 13:40 PDT by JF Bastien
Modified: 2018-04-17 16:48 PDT (History)
9 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description JF Bastien 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.
Comment 1 JF Bastien 2018-04-17 13:41:11 PDT
<rdar://problem/38871451>
Comment 2 JF Bastien 2018-04-17 13:44:53 PDT
Created attachment 338145 [details]
patch
Comment 3 Saam Barati 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.
Comment 4 Filip Pizlo 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.
Comment 5 JF Bastien 2018-04-17 14:29:48 PDT
Created attachment 338149 [details]
patch
Comment 6 Saam Barati 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
Comment 7 Saam Barati 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){}
};
Comment 8 JF Bastien 2018-04-17 14:55:16 PDT
Created attachment 338153 [details]
patch
Comment 9 JF Bastien 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
Comment 10 WebKit Commit Bot 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>
Comment 11 WebKit Commit Bot 2018-04-17 16:48:07 PDT
All reviewed patches have been landed.  Closing bug.