Attachment 339236 Details for Bug 185174 – the patch

[patch] the patch

blah.patch (text/plain), 10.65 KB, created by Filip Pizlo on 2018-05-01 16:08:59 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Filip Pizlo

Created: 2018-05-01 16:08:59 PDT

Size: 10.65 KB

patch

obsolete

>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 231215)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,37 @@
>+2018-05-01  Filip Pizlo  <fpizlo@apple.com>
>+
>+        JSC should be able to cache custom setter calls on the prototype chain
>+        https://bugs.webkit.org/show_bug.cgi?id=185174
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        We broke custom-setter-on-the-prototype-chain caching when we fixed a bug involving the conditionSet.isEmpty()
>+        condition being used to determine if we have an alternateBase. The fix in r222671 incorrectly tried to add
>+        impossible-to-validate conditions to the conditionSet by calling generateConditionsForPrototypePropertyHit() instead
>+        of generateConditionsForPrototypePropertyHitCustom(). The problem is that the former function will always fail for
>+        custom accessors because it won't find the custom property in the structure.
>+
>+        The fix is to add a virtual hasAlternateBase() function and use that instead of conditionSet.isEmpty().
>+
>+        This is a 4x speed-up on assign-custom-setter.js.
>+
>+        * bytecode/AccessCase.cpp:
>+        (JSC::AccessCase::hasAlternateBase const):
>+        (JSC::AccessCase::alternateBase const):
>+        (JSC::AccessCase::generateImpl):
>+        * bytecode/AccessCase.h:
>+        (JSC::AccessCase::alternateBase const): Deleted.
>+        * bytecode/GetterSetterAccessCase.cpp:
>+        (JSC::GetterSetterAccessCase::hasAlternateBase const):
>+        (JSC::GetterSetterAccessCase::alternateBase const):
>+        * bytecode/GetterSetterAccessCase.h:
>+        * bytecode/ObjectPropertyConditionSet.cpp:
>+        (JSC::generateConditionsForPrototypePropertyHitCustom):
>+        * bytecode/ObjectPropertyConditionSet.h:
>+        * jit/Repatch.cpp:
>+        (JSC::tryCacheGetByID):
>+        (JSC::tryCachePutByID):
>+
> 2018-04-30  Filip Pizlo  <fpizlo@apple.com>
> 
>         B3::demoteValues should be able to handle patchpoint terminals
>Index: Source/JavaScriptCore/bytecode/AccessCase.cpp
>===================================================================
>--- Source/JavaScriptCore/bytecode/AccessCase.cpp	(revision 231201)
>+++ Source/JavaScriptCore/bytecode/AccessCase.cpp	(working copy)
>@@ -121,6 +121,16 @@ std::unique_ptr<AccessCase> AccessCase::
>     }
> }
> 
>+bool AccessCase::hasAlternateBase() const
>+{
>+    return !conditionSet().isEmpty();
>+}
>+
>+JSObject* AccessCase::alternateBase() const
>+{
>+    return conditionSet().slotBaseCondition().object();
>+}
>+
> std::unique_ptr<AccessCase> AccessCase::clone() const
> {
>     std::unique_ptr<AccessCase> result(new AccessCase(*this));
>@@ -572,10 +582,10 @@ void AccessCase::generateImpl(AccessGene
> 
>         if (isValidOffset(m_offset)) {
>             Structure* currStructure;
>-            if (m_conditionSet.isEmpty())
>+            if (!hasAlternateBase())
>                 currStructure = structure();
>             else
>-                currStructure = m_conditionSet.slotBaseCondition().object()->structure();
>+                currStructure = alternateBase()->structure();
>             currStructure->startWatchingPropertyForReplacements(vm, offset());
>         }
> 
>@@ -603,7 +613,7 @@ void AccessCase::generateImpl(AccessGene
>             // but it'd require emitting the same code to load the base twice.
>             baseForAccessGPR = scratchGPR;
>         } else {
>-            if (!m_conditionSet.isEmpty()) {
>+            if (hasAlternateBase()) {
>                 jit.move(
>                     CCallHelpers::TrustedImmPtr(alternateBase()), scratchGPR);
>                 baseForAccessGPR = scratchGPR;
>@@ -1101,10 +1111,10 @@ void AccessCase::generateImpl(AccessGene
>         // We need to ensure the getter value does not move from under us. Note that GetterSetters
>         // are immutable so we just need to watch the property not any value inside it.
>         Structure* currStructure;
>-        if (m_conditionSet.isEmpty())
>+        if (!hasAlternateBase())
>             currStructure = structure();
>         else
>-            currStructure = m_conditionSet.slotBaseCondition().object()->structure();
>+            currStructure = alternateBase()->structure();
>         currStructure->startWatchingPropertyForReplacements(vm, offset());
>         
>         this->as<IntrinsicGetterAccessCase>().emitIntrinsicGetter(state);
>Index: Source/JavaScriptCore/bytecode/AccessCase.h
>===================================================================
>--- Source/JavaScriptCore/bytecode/AccessCase.h	(revision 231201)
>+++ Source/JavaScriptCore/bytecode/AccessCase.h	(working copy)
>@@ -149,7 +149,9 @@ public:
> 
>     ObjectPropertyConditionSet conditionSet() const { return m_conditionSet; }
> 
>-    virtual JSObject* alternateBase() const { return conditionSet().slotBaseCondition().object(); }
>+    virtual bool hasAlternateBase() const;
>+    virtual JSObject* alternateBase() const;
>+    
>     virtual WatchpointSet* additionalSet() const { return nullptr; }
>     virtual bool viaProxy() const { return false; }
> 
>Index: Source/JavaScriptCore/bytecode/GetterSetterAccessCase.cpp
>===================================================================
>--- Source/JavaScriptCore/bytecode/GetterSetterAccessCase.cpp	(revision 231201)
>+++ Source/JavaScriptCore/bytecode/GetterSetterAccessCase.cpp	(working copy)
>@@ -100,11 +100,18 @@ std::unique_ptr<AccessCase> GetterSetter
>     return WTFMove(result);
> }
> 
>+bool GetterSetterAccessCase::hasAlternateBase() const
>+{
>+    if (customSlotBase())
>+        return true;
>+    return Base::hasAlternateBase();
>+}
>+
> JSObject* GetterSetterAccessCase::alternateBase() const
> {
>     if (customSlotBase())
>         return customSlotBase();
>-    return conditionSet().slotBaseCondition().object();
>+    return Base::alternateBase();
> }
> 
> void GetterSetterAccessCase::dumpImpl(PrintStream& out, CommaPrinter& comma) const
>Index: Source/JavaScriptCore/bytecode/GetterSetterAccessCase.h
>===================================================================
>--- Source/JavaScriptCore/bytecode/GetterSetterAccessCase.h	(revision 231201)
>+++ Source/JavaScriptCore/bytecode/GetterSetterAccessCase.h	(working copy)
>@@ -43,6 +43,7 @@ public:
>     JSObject* customSlotBase() const { return m_customSlotBase.get(); }
>     std::optional<DOMAttributeAnnotation> domAttribute() const { return m_domAttribute; }
> 
>+    bool hasAlternateBase() const override;
>     JSObject* alternateBase() const override;
> 
>     void emitDOMJITGetter(AccessGenerationState&, const DOMJIT::GetterSetter*, GPRReg baseForGetGPR);
>Index: Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.cpp
>===================================================================
>--- Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.cpp	(revision 231201)
>+++ Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.cpp	(working copy)
>@@ -374,6 +374,24 @@ ObjectPropertyConditionSet generateCondi
>         });
> }
> 
>+ObjectPropertyConditionSet generateConditionsForPrototypePropertyHitCustom(
>+    VM& vm, JSCell* owner, ExecState* exec, Structure* headStructure, JSObject* prototype,
>+    UniquedStringImpl* uid)
>+{
>+    return generateConditions(
>+        vm, exec->lexicalGlobalObject(), headStructure, prototype,
>+        [&] (Vector<ObjectPropertyCondition>& conditions, JSObject* object) -> bool {
>+            if (object == prototype)
>+                return true;
>+            ObjectPropertyCondition result =
>+                generateCondition(vm, owner, object, uid, PropertyCondition::Absence);
>+            if (!result)
>+                return false;
>+            conditions.append(result);
>+            return true;
>+        });
>+}
>+
> ObjectPropertyConditionSet generateConditionsForPrototypeEquivalenceConcurrently(
>     VM& vm, JSGlobalObject* globalObject, Structure* headStructure, JSObject* prototype, UniquedStringImpl* uid)
> {
>Index: Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.h
>===================================================================
>--- Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.h	(revision 231201)
>+++ Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.h	(working copy)
>@@ -165,6 +165,9 @@ ObjectPropertyConditionSet generateCondi
> ObjectPropertyConditionSet generateConditionsForPrototypePropertyHit(
>     VM&, JSCell* owner, ExecState*, Structure* headStructure, JSObject* prototype,
>     UniquedStringImpl* uid);
>+ObjectPropertyConditionSet generateConditionsForPrototypePropertyHitCustom(
>+    VM&, JSCell* owner, ExecState*, Structure* headStructure, JSObject* prototype,
>+    UniquedStringImpl* uid);
> 
> ObjectPropertyConditionSet generateConditionsForPrototypeEquivalenceConcurrently(
>     VM&, JSGlobalObject*, Structure* headStructure, JSObject* prototype,
>Index: Source/JavaScriptCore/jit/Repatch.cpp
>===================================================================
>--- Source/JavaScriptCore/jit/Repatch.cpp	(revision 231201)
>+++ Source/JavaScriptCore/jit/Repatch.cpp	(working copy)
>@@ -359,7 +359,7 @@ static InlineCacheAction tryCacheGetByID
>                     newCase = GetterSetterAccessCase::create(
>                         vm, codeBlock, type, offset, structure, conditionSet, loadTargetFromProxy,
>                         slot.watchpointSet(), slot.isCacheableCustom() ? slot.customGetter() : nullptr,
>-                        slot.isCacheableCustom() ? slot.slotBase() : nullptr,
>+                        slot.isCacheableCustom() && slot.slotBase() != baseValue ? slot.slotBase() : nullptr,
>                         domAttribute, WTFMove(prototypeAccessChain));
>                 }
>             }
>@@ -529,7 +529,7 @@ static InlineCacheAction tryCachePutByID
>                     if (!usesPolyProto) {
>                         prototypeAccessChain = nullptr;
>                         conditionSet =
>-                            generateConditionsForPrototypePropertyHit(
>+                            generateConditionsForPrototypePropertyHitCustom(
>                                 vm, codeBlock, exec, structure, slot.base(), ident.impl());
>                         if (!conditionSet.isValid())
>                             return GiveUpOnCache;
>@@ -538,7 +538,7 @@ static InlineCacheAction tryCachePutByID
> 
>                 newCase = GetterSetterAccessCase::create(
>                     vm, codeBlock, slot.isCustomAccessor() ? AccessCase::CustomAccessorSetter : AccessCase::CustomValueSetter, structure, invalidOffset,
>-                    conditionSet, WTFMove(prototypeAccessChain), slot.customSetter(), slot.base());
>+                    conditionSet, WTFMove(prototypeAccessChain), slot.customSetter(), slot.base() != baseValue ? slot.base() : nullptr);
>             } else {
>                 ObjectPropertyConditionSet conditionSet;
>                 std::unique_ptr<PolyProtoAccessChain> prototypeAccessChain;

Flags:

saam: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 185174: 339236