[webkit-reviews] review granted: [Bug 215250] Inline cache Replace and Setters on PureForwardingProxy : [Attachment 406442] patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted Saam Barati <sbarati at apple.com>'s
request for review:
Bug 215250: Inline cache Replace and Setters on PureForwardingProxy
https://bugs.webkit.org/show_bug.cgi?id=215250

Attachment 406442: patch

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




--- Comment #10 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 406442
  --> https://bugs.webkit.org/attachment.cgi?id=406442
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?