Bug 215250 – Inline cache Replace and Setters on PureForwardingProxy

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

[Saam Barati]

Should help with perf.

[Saam Barati]

Created attachment 406243 [details]
WIP

This implements replace and JS setters. Next I'll do custom value/custom accessor. Then it should be done.

15% faster on 3d-cube-SP

[Saam Barati]

Created attachment 406349 [details]
patch

[Yusuke Suzuki]

Comment on attachment 406349 [details]
patch

r=me

[Yusuke Suzuki]

Looks like some failures are occurring?

[Saam Barati]

Comment on attachment 406349 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=406349&action=review

> Source/JavaScriptCore/runtime/JSObject.cpp:822
> +                if (!this->structure(vm)->isUncacheableDictionary())

I wonder if this is the cause of the failure. If so, I suspect it might be revealing an implementation bug.

[Saam Barati]

Created attachment 406356 [details]
WIP

test on EWS

[Saam Barati]

Created attachment 406357 [details]
WIP

[Saam Barati]

Created attachment 406440 [details]
patch

[Saam Barati]

Created attachment 406442 [details]
patch

[Yusuke Suzuki]

Comment on attachment 406442 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=406442&action=review

r=me

> Source/JavaScriptCore/bytecode/AccessCase.cpp:1468
> +        if (viaProxy() && doesPropertyStorageLoads && !m_polyProtoAccessChain && !hasAlternateBase()) {
> +            // We only need this when loading an inline or out of line property. For customs, we can
> +            // invoke with a receiver value that is a JSProxy. For getters/setters, we'll also invoke
> +            // them with the JSProxy as |this|, but we need to load the actual GetterSetter cell from
> +            // the JSProxy's target.
> +
>              if (m_type == Getter || m_type == Setter)
> -                baseForGetGPR = scratchGPR;
> +                propertyOwnerGPR = scratchGPR;
>              else
> -                baseForGetGPR = valueRegsPayloadGPR;
> -
> -            ASSERT((m_type != Getter && m_type != Setter) || baseForGetGPR != baseGPR);
> -            ASSERT(m_type != Setter || baseForGetGPR != valueRegsPayloadGPR);
> +                propertyOwnerGPR = valueRegsPayloadGPR;
>  
>              jit.loadPtr(
> -                CCallHelpers::Address(baseGPR, JSProxy::targetOffset()),
> -                baseForGetGPR);
> -        } else
> -            baseForGetGPR = baseGPR;
> -
> -        GPRReg baseForAccessGPR;
> -        if (m_polyProtoAccessChain) {
> +                CCallHelpers::Address(baseGPR, JSProxy::targetOffset()), propertyOwnerGPR);
> +        } else if (m_polyProtoAccessChain) {
>              // This isn't pretty, but we know we got here via generateWithGuard,
>              // and it left the baseForAccess inside scratchGPR. We could re-derive the base,
>              // but it'd require emitting the same code to load the base twice.
> -            baseForAccessGPR = scratchGPR;
> -        } else {
> -            if (hasAlternateBase()) {
> -                jit.move(
> -                    CCallHelpers::TrustedImmPtr(alternateBase()), scratchGPR);
> -                baseForAccessGPR = scratchGPR;
> -            } else
> -                baseForAccessGPR = baseForGetGPR;
> -        }
> +            propertyOwnerGPR = scratchGPR;
> +        } else if (hasAlternateBase()) {
> +            jit.move(
> +                CCallHelpers::TrustedImmPtr(alternateBase()), scratchGPR);
> +            propertyOwnerGPR = scratchGPR;

I think reordering it as follows makes code easy to follow.

if (m_polyProtoAccessChain) {
   ...
} else if (hasAlternateBase()) {
   ...
} else if (viaProxy() && doesPropertyStorageLoads) {
   ...
} else if (viaProxy() && takesPropertyOwnerAsCFunctionArgument) {
   ...
} else {
    ASSERT(!viaProxy());
   ...
}

> Source/JavaScriptCore/jit/Repatch.cpp:612
> +        if (isProxy) {
> +            // We currently only cache Replace and JS/Custom Setters on JSProxy. We don't
> +            // cache transitions because global objects will never share the same structure
> +            // in our current implementation.
> +            bool isCacheableProxy = (slot.isCacheablePut() && slot.type() == PutPropertySlot::ExistingProperty)
> +                || slot.isCacheableSetter()
> +                || slot.isCacheableCustom();
> +            if (!isCacheableProxy)
> +                return GiveUpOnCache;
> +        }

How about putting this branch under `baseCell->type() == PureForwardingProxyType` branch too?

[Saam Barati]

Comment on attachment 406442 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=406442&action=review

>> Source/JavaScriptCore/bytecode/AccessCase.cpp:1468
>> +            propertyOwnerGPR = scratchGPR;
> 
> I think reordering it as follows makes code easy to follow.
> 
> if (m_polyProtoAccessChain) {
>    ...
> } else if (hasAlternateBase()) {
>    ...
> } else if (viaProxy() && doesPropertyStorageLoads) {
>    ...
> } else if (viaProxy() && takesPropertyOwnerAsCFunctionArgument) {
>    ...
> } else {
>     ASSERT(!viaProxy());
>    ...
> }

Sounds good. The only thing is the last assert in the else isn’t correct. For example, for custom accessors may be viaProxy

>> Source/JavaScriptCore/jit/Repatch.cpp:612
>> +        }
> 
> How about putting this branch under `baseCell->type() == PureForwardingProxyType` branch too?

Yeah makes sense. They were originally separate because i had some code between them. But I deleted that code.

[Saam Barati]

Created attachment 406489 [details]
patch for landing

[EWS]

Committed r265600: <https://trac.webkit.org/changeset/265600>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406489 [details].

[Radar WebKit Bug Importer]

<rdar://problem/66960215>