Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (231249 => 231250)
--- trunk/Source/_javascript_Core/ChangeLog 2018-05-02 18:44:10 UTC (rev 231249)
+++ trunk/Source/_javascript_Core/ChangeLog 2018-05-02 18:51:16 UTC (rev 231250)
@@ -1,3 +1,37 @@
+2018-05-01 Filip Pizlo <[email protected]>
+
+ JSC should be able to cache custom setter calls on the prototype chain
+ https://bugs.webkit.org/show_bug.cgi?id=185174
+
+ Reviewed by Saam Barati.
+
+ 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-05-02 Dominik Infuehr <[email protected]>
[MIPS] Implement and16 and store16 for MacroAssemblerMIPS
Modified: trunk/Source/_javascript_Core/bytecode/AccessCase.cpp (231249 => 231250)
--- trunk/Source/_javascript_Core/bytecode/AccessCase.cpp 2018-05-02 18:44:10 UTC (rev 231249)
+++ trunk/Source/_javascript_Core/bytecode/AccessCase.cpp 2018-05-02 18:51:16 UTC (rev 231250)
@@ -121,6 +121,16 @@
}
}
+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 @@
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 @@
// 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 @@
// 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);
Modified: trunk/Source/_javascript_Core/bytecode/AccessCase.h (231249 => 231250)
--- trunk/Source/_javascript_Core/bytecode/AccessCase.h 2018-05-02 18:44:10 UTC (rev 231249)
+++ trunk/Source/_javascript_Core/bytecode/AccessCase.h 2018-05-02 18:51:16 UTC (rev 231250)
@@ -149,7 +149,9 @@
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; }
Modified: trunk/Source/_javascript_Core/bytecode/GetterSetterAccessCase.cpp (231249 => 231250)
--- trunk/Source/_javascript_Core/bytecode/GetterSetterAccessCase.cpp 2018-05-02 18:44:10 UTC (rev 231249)
+++ trunk/Source/_javascript_Core/bytecode/GetterSetterAccessCase.cpp 2018-05-02 18:51:16 UTC (rev 231250)
@@ -100,11 +100,18 @@
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
Modified: trunk/Source/_javascript_Core/bytecode/GetterSetterAccessCase.h (231249 => 231250)
--- trunk/Source/_javascript_Core/bytecode/GetterSetterAccessCase.h 2018-05-02 18:44:10 UTC (rev 231249)
+++ trunk/Source/_javascript_Core/bytecode/GetterSetterAccessCase.h 2018-05-02 18:51:16 UTC (rev 231250)
@@ -43,6 +43,7 @@
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);
Modified: trunk/Source/_javascript_Core/bytecode/ObjectPropertyConditionSet.cpp (231249 => 231250)
--- trunk/Source/_javascript_Core/bytecode/ObjectPropertyConditionSet.cpp 2018-05-02 18:44:10 UTC (rev 231249)
+++ trunk/Source/_javascript_Core/bytecode/ObjectPropertyConditionSet.cpp 2018-05-02 18:51:16 UTC (rev 231250)
@@ -374,6 +374,24 @@
});
}
+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)
{
Modified: trunk/Source/_javascript_Core/bytecode/ObjectPropertyConditionSet.h (231249 => 231250)
--- trunk/Source/_javascript_Core/bytecode/ObjectPropertyConditionSet.h 2018-05-02 18:44:10 UTC (rev 231249)
+++ trunk/Source/_javascript_Core/bytecode/ObjectPropertyConditionSet.h 2018-05-02 18:51:16 UTC (rev 231250)
@@ -165,6 +165,9 @@
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,
Modified: trunk/Source/_javascript_Core/jit/Repatch.cpp (231249 => 231250)
--- trunk/Source/_javascript_Core/jit/Repatch.cpp 2018-05-02 18:44:10 UTC (rev 231249)
+++ trunk/Source/_javascript_Core/jit/Repatch.cpp 2018-05-02 18:51:16 UTC (rev 231250)
@@ -359,7 +359,7 @@
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 @@
if (!usesPolyProto) {
prototypeAccessChain = nullptr;
conditionSet =
- generateConditionsForPrototypePropertyHit(
+ generateConditionsForPrototypePropertyHitCustom(
vm, codeBlock, exec, structure, slot.base(), ident.impl());
if (!conditionSet.isValid())
return GiveUpOnCache;
@@ -538,7 +538,7 @@
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;
