Bug 184706 – A put is not an ExistingProperty put when we transition a structure because of an attributes change

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[JF Bastien]

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.

[JF Bastien]

<rdar://problem/38871451>

[JF Bastien]

Created attachment 338145 [details]
patch

[Saam Barati]

Comment on attachment 338145 [details]
patch

I don't see how this doesn't disable caching of all transitions.

[Filip Pizlo]

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]

Created attachment 338149 [details]
patch

[Saam Barati]

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 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]

Created attachment 338153 [details]
patch

[JF Bastien]

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 on attachment 338153 [details]
patch

Clearing flags on attachment: 338153

Committed r230740: <https://trac.webkit.org/changeset/230740>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.