Skip to content

Commit 0b6074b

Browse files
author
Filip Pizlo
committed
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): Canonical link: https://commits.webkit.org/200694@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@231250 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent 487cf10 commit 0b6074b

8 files changed

Lines changed: 85 additions & 10 deletions

File tree

‎Source/JavaScriptCore/ChangeLog‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,37 @@
1+
2018-05-01 Filip Pizlo <fpizlo@apple.com>
2+
3+
JSC should be able to cache custom setter calls on the prototype chain
4+
https://bugs.webkit.org/show_bug.cgi?id=185174
5+
6+
Reviewed by Saam Barati.
7+
8+
We broke custom-setter-on-the-prototype-chain caching when we fixed a bug involving the conditionSet.isEmpty()
9+
condition being used to determine if we have an alternateBase. The fix in r222671 incorrectly tried to add
10+
impossible-to-validate conditions to the conditionSet by calling generateConditionsForPrototypePropertyHit() instead
11+
of generateConditionsForPrototypePropertyHitCustom(). The problem is that the former function will always fail for
12+
custom accessors because it won't find the custom property in the structure.
13+
14+
The fix is to add a virtual hasAlternateBase() function and use that instead of conditionSet.isEmpty().
15+
16+
This is a 4x speed-up on assign-custom-setter.js.
17+
18+
* bytecode/AccessCase.cpp:
19+
(JSC::AccessCase::hasAlternateBase const):
20+
(JSC::AccessCase::alternateBase const):
21+
(JSC::AccessCase::generateImpl):
22+
* bytecode/AccessCase.h:
23+
(JSC::AccessCase::alternateBase const): Deleted.
24+
* bytecode/GetterSetterAccessCase.cpp:
25+
(JSC::GetterSetterAccessCase::hasAlternateBase const):
26+
(JSC::GetterSetterAccessCase::alternateBase const):
27+
* bytecode/GetterSetterAccessCase.h:
28+
* bytecode/ObjectPropertyConditionSet.cpp:
29+
(JSC::generateConditionsForPrototypePropertyHitCustom):
30+
* bytecode/ObjectPropertyConditionSet.h:
31+
* jit/Repatch.cpp:
32+
(JSC::tryCacheGetByID):
33+
(JSC::tryCachePutByID):
34+
135
2018-05-02 Dominik Infuehr <dinfuehr@igalia.com>
236

337
[MIPS] Implement and16 and store16 for MacroAssemblerMIPS

‎Source/JavaScriptCore/bytecode/AccessCase.cpp‎

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,16 @@ std::unique_ptr<AccessCase> AccessCase::fromStructureStubInfo(
121121
}
122122
}
123123

124+
bool AccessCase::hasAlternateBase() const
125+
{
126+
return !conditionSet().isEmpty();
127+
}
128+
129+
JSObject* AccessCase::alternateBase() const
130+
{
131+
return conditionSet().slotBaseCondition().object();
132+
}
133+
124134
std::unique_ptr<AccessCase> AccessCase::clone() const
125135
{
126136
std::unique_ptr<AccessCase> result(new AccessCase(*this));
@@ -572,10 +582,10 @@ void AccessCase::generateImpl(AccessGenerationState& state)
572582

573583
if (isValidOffset(m_offset)) {
574584
Structure* currStructure;
575-
if (m_conditionSet.isEmpty())
585+
if (!hasAlternateBase())
576586
currStructure = structure();
577587
else
578-
currStructure = m_conditionSet.slotBaseCondition().object()->structure();
588+
currStructure = alternateBase()->structure();
579589
currStructure->startWatchingPropertyForReplacements(vm, offset());
580590
}
581591

@@ -603,7 +613,7 @@ void AccessCase::generateImpl(AccessGenerationState& state)
603613
// but it'd require emitting the same code to load the base twice.
604614
baseForAccessGPR = scratchGPR;
605615
} else {
606-
if (!m_conditionSet.isEmpty()) {
616+
if (hasAlternateBase()) {
607617
jit.move(
608618
CCallHelpers::TrustedImmPtr(alternateBase()), scratchGPR);
609619
baseForAccessGPR = scratchGPR;
@@ -1101,10 +1111,10 @@ void AccessCase::generateImpl(AccessGenerationState& state)
11011111
// We need to ensure the getter value does not move from under us. Note that GetterSetters
11021112
// are immutable so we just need to watch the property not any value inside it.
11031113
Structure* currStructure;
1104-
if (m_conditionSet.isEmpty())
1114+
if (!hasAlternateBase())
11051115
currStructure = structure();
11061116
else
1107-
currStructure = m_conditionSet.slotBaseCondition().object()->structure();
1117+
currStructure = alternateBase()->structure();
11081118
currStructure->startWatchingPropertyForReplacements(vm, offset());
11091119

11101120
this->as<IntrinsicGetterAccessCase>().emitIntrinsicGetter(state);

‎Source/JavaScriptCore/bytecode/AccessCase.h‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,9 @@ class AccessCase {
149149

150150
ObjectPropertyConditionSet conditionSet() const { return m_conditionSet; }
151151

152-
virtual JSObject* alternateBase() const { return conditionSet().slotBaseCondition().object(); }
152+
virtual bool hasAlternateBase() const;
153+
virtual JSObject* alternateBase() const;
154+
153155
virtual WatchpointSet* additionalSet() const { return nullptr; }
154156
virtual bool viaProxy() const { return false; }
155157

‎Source/JavaScriptCore/bytecode/GetterSetterAccessCase.cpp‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -100,11 +100,18 @@ std::unique_ptr<AccessCase> GetterSetterAccessCase::clone() const
100100
return WTFMove(result);
101101
}
102102

103+
bool GetterSetterAccessCase::hasAlternateBase() const
104+
{
105+
if (customSlotBase())
106+
return true;
107+
return Base::hasAlternateBase();
108+
}
109+
103110
JSObject* GetterSetterAccessCase::alternateBase() const
104111
{
105112
if (customSlotBase())
106113
return customSlotBase();
107-
return conditionSet().slotBaseCondition().object();
114+
return Base::alternateBase();
108115
}
109116

110117
void GetterSetterAccessCase::dumpImpl(PrintStream& out, CommaPrinter& comma) const

‎Source/JavaScriptCore/bytecode/GetterSetterAccessCase.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ class GetterSetterAccessCase : public ProxyableAccessCase {
4343
JSObject* customSlotBase() const { return m_customSlotBase.get(); }
4444
std::optional<DOMAttributeAnnotation> domAttribute() const { return m_domAttribute; }
4545

46+
bool hasAlternateBase() const override;
4647
JSObject* alternateBase() const override;
4748

4849
void emitDOMJITGetter(AccessGenerationState&, const DOMJIT::GetterSetter*, GPRReg baseForGetGPR);

‎Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.cpp‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -374,6 +374,24 @@ ObjectPropertyConditionSet generateConditionsForPrototypePropertyHit(
374374
});
375375
}
376376

377+
ObjectPropertyConditionSet generateConditionsForPrototypePropertyHitCustom(
378+
VM& vm, JSCell* owner, ExecState* exec, Structure* headStructure, JSObject* prototype,
379+
UniquedStringImpl* uid)
380+
{
381+
return generateConditions(
382+
vm, exec->lexicalGlobalObject(), headStructure, prototype,
383+
[&] (Vector<ObjectPropertyCondition>& conditions, JSObject* object) -> bool {
384+
if (object == prototype)
385+
return true;
386+
ObjectPropertyCondition result =
387+
generateCondition(vm, owner, object, uid, PropertyCondition::Absence);
388+
if (!result)
389+
return false;
390+
conditions.append(result);
391+
return true;
392+
});
393+
}
394+
377395
ObjectPropertyConditionSet generateConditionsForPrototypeEquivalenceConcurrently(
378396
VM& vm, JSGlobalObject* globalObject, Structure* headStructure, JSObject* prototype, UniquedStringImpl* uid)
379397
{

‎Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,9 @@ ObjectPropertyConditionSet generateConditionsForPropertySetterMiss(
165165
ObjectPropertyConditionSet generateConditionsForPrototypePropertyHit(
166166
VM&, JSCell* owner, ExecState*, Structure* headStructure, JSObject* prototype,
167167
UniquedStringImpl* uid);
168+
ObjectPropertyConditionSet generateConditionsForPrototypePropertyHitCustom(
169+
VM&, JSCell* owner, ExecState*, Structure* headStructure, JSObject* prototype,
170+
UniquedStringImpl* uid);
168171

169172
ObjectPropertyConditionSet generateConditionsForPrototypeEquivalenceConcurrently(
170173
VM&, JSGlobalObject*, Structure* headStructure, JSObject* prototype,

‎Source/JavaScriptCore/jit/Repatch.cpp‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -359,7 +359,7 @@ static InlineCacheAction tryCacheGetByID(ExecState* exec, JSValue baseValue, con
359359
newCase = GetterSetterAccessCase::create(
360360
vm, codeBlock, type, offset, structure, conditionSet, loadTargetFromProxy,
361361
slot.watchpointSet(), slot.isCacheableCustom() ? slot.customGetter() : nullptr,
362-
slot.isCacheableCustom() ? slot.slotBase() : nullptr,
362+
slot.isCacheableCustom() && slot.slotBase() != baseValue ? slot.slotBase() : nullptr,
363363
domAttribute, WTFMove(prototypeAccessChain));
364364
}
365365
}
@@ -529,7 +529,7 @@ static InlineCacheAction tryCachePutByID(ExecState* exec, JSValue baseValue, Str
529529
if (!usesPolyProto) {
530530
prototypeAccessChain = nullptr;
531531
conditionSet =
532-
generateConditionsForPrototypePropertyHit(
532+
generateConditionsForPrototypePropertyHitCustom(
533533
vm, codeBlock, exec, structure, slot.base(), ident.impl());
534534
if (!conditionSet.isValid())
535535
return GiveUpOnCache;
@@ -538,7 +538,7 @@ static InlineCacheAction tryCachePutByID(ExecState* exec, JSValue baseValue, Str
538538

539539
newCase = GetterSetterAccessCase::create(
540540
vm, codeBlock, slot.isCustomAccessor() ? AccessCase::CustomAccessorSetter : AccessCase::CustomValueSetter, structure, invalidOffset,
541-
conditionSet, WTFMove(prototypeAccessChain), slot.customSetter(), slot.base());
541+
conditionSet, WTFMove(prototypeAccessChain), slot.customSetter(), slot.base() != baseValue ? slot.base() : nullptr);
542542
} else {
543543
ObjectPropertyConditionSet conditionSet;
544544
std::unique_ptr<PolyProtoAccessChain> prototypeAccessChain;

0 commit comments

Comments
 (0)