Should help with perf.
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
Created attachment 406349 [details] patch
Comment on attachment 406349 [details] patch r=me
Looks like some failures are occurring?
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.
Created attachment 406356 [details] WIP test on EWS
Created attachment 406357 [details] WIP
Created attachment 406440 [details] patch
Created attachment 406442 [details] patch
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?
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.
Created attachment 406489 [details] patch for landing
Committed r265600: <https://trac.webkit.org/changeset/265600> All reviewed patches have been landed. Closing bug and clearing flags on attachment 406489 [details].
<rdar://problem/66960215>