Skip to content

Commit a512fe6

Browse files
kmiller68kmiller68
committed
[JSC] GetFromScope and PutToScope should cache the StructureID
https://bugs.webkit.org/show_bug.cgi?id=300504 rdar://162364409 Reviewed by Yusuke Suzuki and Dan Hecht. Right now we the whole Structure* but that's kinda pointless because we are going to load a StructureID from the scope we are attempting to load from. Instead we should just cache the StructureID and compare those directly. Canonical link: https://commits.webkit.org/301358@main
1 parent 2b43f90 commit a512fe6

7 files changed

Lines changed: 51 additions & 54 deletions

File tree

‎Source/JavaScriptCore/bytecode/BytecodeList.rb‎

Lines changed: 17 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@
5151
:PropertyOffset,
5252
:PutByIdFlags,
5353
:ResolveType,
54-
:Structure,
54+
# FIXME: We should use WriteBarrierStructureID instead.
5555
:StructureID,
5656
:StructureChain,
5757
:SymbolTable,
@@ -60,6 +60,7 @@
6060
:TypeLocation,
6161
:WasmBoundLabel,
6262
:WatchpointSet,
63+
:WriteBarrierStructureID,
6364

6465
:ValueProfileAndVirtualRegisterBuffer,
6566
:ArrayProfile,
@@ -504,22 +505,21 @@
504505

505506
op :get_from_scope,
506507
args: {
507-
dst: VirtualRegister, # offset 1
508-
scope: VirtualRegister, # offset 2
509-
var: unsigned, # offset 3
510-
# $begin: :private,
508+
dst: VirtualRegister,
509+
scope: VirtualRegister,
510+
var: unsigned,
511511
getPutInfo: GetPutInfo,
512512
localScopeDepth: unsigned,
513513
offset: unsigned,
514-
valueProfile: unsigned, # offset 7
514+
valueProfile: unsigned,
515515
},
516516
metadata: {
517-
getPutInfo: GetPutInfo, # offset 4
518-
_: { #previously offset 5
517+
getPutInfo: GetPutInfo,
518+
_: {
519519
watchpointSet: WatchpointSet.*,
520-
structure: WriteBarrierBase[Structure],
520+
structureID: WriteBarrierStructureID,
521521
},
522-
operand: uintptr_t, #offset 6
522+
operand: uintptr_t,
523523
},
524524
metadata_initializers: {
525525
getPutInfo: :getPutInfo,
@@ -528,21 +528,20 @@
528528

529529
op :put_to_scope,
530530
args: {
531-
scope: VirtualRegister, # offset 1
532-
var: unsigned, # offset 2
533-
value: VirtualRegister, # offset 3
534-
# $begin: :private,
531+
scope: VirtualRegister,
532+
var: unsigned,
533+
value: VirtualRegister,
535534
getPutInfo: GetPutInfo,
536535
symbolTableOrScopeDepth: SymbolTableOrScopeDepth,
537536
offset: unsigned,
538537
},
539538
metadata: {
540-
getPutInfo: GetPutInfo, # offset 4
541-
_: { # offset 5
542-
structure: WriteBarrierBase[Structure],
539+
getPutInfo: GetPutInfo,
540+
_: {
541+
structureID: WriteBarrierStructureID,
543542
watchpointSet: WatchpointSet.*,
544543
},
545-
operand: uintptr_t, # offset 6
544+
operand: uintptr_t,
546545
},
547546
metadata_initializers: {
548547
getPutInfo: :getPutInfo,

‎Source/JavaScriptCore/bytecode/CodeBlock.cpp‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -604,7 +604,7 @@ bool CodeBlock::finishCreation(VM& vm, ScriptExecutable* ownerExecutable, Unlink
604604
if (op.type == GlobalVar || op.type == GlobalVarWithVarInjectionChecks || op.type == GlobalLexicalVar || op.type == GlobalLexicalVarWithVarInjectionChecks)
605605
metadata.m_watchpointSet = op.watchpointSet;
606606
else if (op.structure)
607-
metadata.m_structure.set(vm, this, op.structure);
607+
metadata.m_structureID.set(vm, this, op.structure);
608608
metadata.m_operand = op.operand;
609609
break;
610610
}
@@ -643,7 +643,7 @@ bool CodeBlock::finishCreation(VM& vm, ScriptExecutable* ownerExecutable, Unlink
643643
if (op.watchpointSet)
644644
op.watchpointSet->invalidate(vm, PutToScopeFireDetail(this, ident));
645645
} else if (op.structure)
646-
metadata.m_structure.set(vm, this, op.structure);
646+
metadata.m_structureID.set(vm, this, op.structure);
647647
metadata.m_operand = op.operand;
648648
break;
649649
}
@@ -1656,7 +1656,7 @@ void CodeBlock::finalizeLLIntInlineCaches()
16561656
if (getPutInfo.resolveType() == GlobalVar || getPutInfo.resolveType() == GlobalVarWithVarInjectionChecks
16571657
|| getPutInfo.resolveType() == ResolvedClosureVar || getPutInfo.resolveType() == GlobalLexicalVar || getPutInfo.resolveType() == GlobalLexicalVarWithVarInjectionChecks)
16581658
return;
1659-
WriteBarrierBase<Structure>& structure = metadata.m_structure;
1659+
WriteBarrierStructureID& structure = metadata.m_structureID;
16601660
if (!structure || vm.heap.isMarked(structure.get()))
16611661
return;
16621662
dataLogLnIf(Options::verboseOSR(), "Clearing scope access with structure ", RawPointer(structure.get()));

‎Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9465,8 +9465,8 @@ void ByteCodeParser::parseBlock(unsigned limit)
94659465
resolveType = getPutInfo.resolveType();
94669466
if (resolveType == GlobalVar || resolveType == GlobalVarWithVarInjectionChecks || resolveType == GlobalLexicalVar || resolveType == GlobalLexicalVarWithVarInjectionChecks)
94679467
watchpoints = metadata.m_watchpointSet;
9468-
else if (resolveType != UnresolvedProperty && resolveType != UnresolvedPropertyWithVarInjectionChecks)
9469-
structure = metadata.m_structure.get();
9468+
else if (resolveType == GlobalProperty || resolveType == GlobalPropertyWithVarInjectionChecks)
9469+
structure = metadata.m_structureID.get();
94709470
operand = metadata.m_operand;
94719471
}
94729472

@@ -9653,8 +9653,8 @@ void ByteCodeParser::parseBlock(unsigned limit)
96539653
resolveType = getPutInfo.resolveType();
96549654
if (resolveType == GlobalVar || resolveType == GlobalVarWithVarInjectionChecks || resolveType == ResolvedClosureVar || resolveType == GlobalLexicalVar || resolveType == GlobalLexicalVarWithVarInjectionChecks)
96559655
watchpoints = metadata.m_watchpointSet;
9656-
else if (resolveType != UnresolvedProperty && resolveType != UnresolvedPropertyWithVarInjectionChecks)
9657-
structure = metadata.m_structure.get();
9656+
else if (resolveType == GlobalProperty || resolveType == GlobalPropertyWithVarInjectionChecks)
9657+
structure = metadata.m_structureID.get();
96589658
operand = metadata.m_operand;
96599659
}
96609660

‎Source/JavaScriptCore/jit/JITPropertyAccess.cpp‎

Lines changed: 7 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1267,12 +1267,12 @@ void JIT::emit_op_get_from_scope(const JSInstruction* currentInstruction)
12671267
static_assert(!(metadataPointerAlignment % metadataMinAlignment));
12681268
static_assert(!(alignof(Metadata) % metadataPointerAlignment));
12691269
static_assert(!(Metadata::offsetOfGetPutInfo() % metadataMinAlignment));
1270-
static_assert(!(Metadata::offsetOfStructure() % metadataMinAlignment));
1270+
static_assert(!(Metadata::offsetOfStructureID() % metadataMinAlignment));
12711271
static_assert(!(Metadata::offsetOfOperand() % metadataPointerAlignment));
12721272
auto metadataAddress = computeBaseAddressForMetadata<metadataMinAlignment>(bytecode, metadataGPR);
12731273

12741274
auto getPutInfoAddress = metadataAddress.withOffset(Metadata::offsetOfGetPutInfo());
1275-
auto structureAddress = metadataAddress.withOffset(Metadata::offsetOfStructure());
1275+
auto structureIDAddress = metadataAddress.withOffset(Metadata::offsetOfStructureID());
12761276
auto operandAddress = metadataAddress.withOffset(Metadata::offsetOfOperand());
12771277

12781278
load32(getPutInfoAddress, scratch1GPR);
@@ -1281,9 +1281,7 @@ void JIT::emit_op_get_from_scope(const JSInstruction* currentInstruction)
12811281
switch (profiledResolveType) {
12821282
case GlobalProperty: {
12831283
addSlowCase(branch32(NotEqual, scratch1GPR, TrustedImm32(profiledResolveType)));
1284-
loadPtr(structureAddress, scratch1GPR);
1285-
addSlowCase(branchTestPtr(Zero, scratch1GPR));
1286-
emitEncodeStructureID(scratch1GPR, scratch1GPR);
1284+
load32(structureIDAddress, scratch1GPR);
12871285
emitGetVirtualRegisterPayload(scope, scopeGPR);
12881286
addSlowCase(branch32(NotEqual, Address(scopeGPR, JSCell::structureIDOffset()), scratch1GPR));
12891287
loadPtr(operandAddress, scratch1GPR);
@@ -1418,9 +1416,7 @@ MacroAssemblerCodeRef<JITThunkPtrTag> JIT::generateOpGetFromScopeThunk(VM& vm)
14181416
case GlobalProperty:
14191417
case GlobalPropertyWithVarInjectionChecks: {
14201418
// Structure check covers var injection since we don't cache structures for anything but the GlobalObject. Additionally, resolve_scope handles checking for the var injection.
1421-
jit.loadPtr(Address(metadataGPR, OpGetFromScope::Metadata::offsetOfStructure()), scratch1GPR);
1422-
slowCase.append(jit.branchTestPtr(Zero, scratch1GPR));
1423-
jit.emitEncodeStructureID(scratch1GPR, scratch1GPR);
1419+
jit.load32(Address(metadataGPR, OpGetFromScope::Metadata::offsetOfStructureID()), scratch1GPR);
14241420
slowCase.append(jit.branch32(NotEqual, Address(scopeGPR, JSCell::structureIDOffset()), scratch1GPR));
14251421

14261422
jit.jitAssert(scopedLambda<Jump(void)>([&] () -> Jump {
@@ -1577,12 +1573,12 @@ void JIT::emit_op_put_to_scope(const JSInstruction* currentInstruction)
15771573

15781574
constexpr size_t metadataPointerAlignment = alignof(void*);
15791575
static_assert(!(Metadata::offsetOfGetPutInfo() % metadataPointerAlignment));
1580-
static_assert(!(Metadata::offsetOfStructure() % metadataPointerAlignment));
1576+
static_assert(!(Metadata::offsetOfStructureID() % metadataPointerAlignment));
15811577
static_assert(!(Metadata::offsetOfOperand() % metadataPointerAlignment));
15821578
static_assert(!(Metadata::offsetOfWatchpointSet() % metadataPointerAlignment));
15831579
auto metadataAddress = computeBaseAddressForMetadata<metadataPointerAlignment>(bytecode, metadataGPR);
15841580
auto getPutInfoAddress = metadataAddress.withOffset(Metadata::offsetOfGetPutInfo());
1585-
auto structureAddress = metadataAddress.withOffset(Metadata::offsetOfStructure());
1581+
auto structureIDAddress = metadataAddress.withOffset(Metadata::offsetOfStructureID());
15861582
auto operandAddress = metadataAddress.withOffset(Metadata::offsetOfOperand());
15871583
auto watchpointSetAddress = metadataAddress.withOffset(Metadata::offsetOfWatchpointSet());
15881584

@@ -1597,10 +1593,8 @@ void JIT::emit_op_put_to_scope(const JSInstruction* currentInstruction)
15971593
constexpr GPRReg scratch1GPR1 = regT3;
15981594
constexpr GPRReg scratch1GPR2 = regT4;
15991595
static_assert(noOverlap(valueJSR, scopeGPR, scratch1GPR1, scratch1GPR2));
1600-
loadPtr(structureAddress, scratch1GPR1);
1596+
load32(structureIDAddress, scratch1GPR1);
16011597
emitGetVirtualRegisterPayload(scope, scopeGPR);
1602-
addSlowCase(branchTestPtr(Zero, scratch1GPR1));
1603-
emitEncodeStructureID(scratch1GPR1, scratch1GPR1);
16041598
addSlowCase(branch32(NotEqual, Address(scopeGPR, JSCell::structureIDOffset()), scratch1GPR1));
16051599

16061600
emitGetVirtualRegister(value, valueJSR);

‎Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2764,7 +2764,7 @@ end)
27642764
macro loadWithStructureCheck(opcodeStruct, get, operand, slowPath)
27652765
get(m_scope, t0)
27662766
loadp PayloadOffset[cfr, t0, 8], t0
2767-
loadp %opcodeStruct%::Metadata::m_structure[t5], t1
2767+
loadp %opcodeStruct%::Metadata::m_structureID[t5], t1
27682768
bineq JSCell::m_structureID[t0], t1, slowPath
27692769
end
27702770

‎Source/JavaScriptCore/llint/LowLevelInterpreter64.asm‎

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2919,12 +2919,11 @@ llintOpWithMetadata(op_resolve_scope, OpResolveScope, macro (size, get, dispatch
29192919
end)
29202920

29212921

2922-
macro loadWithStructureCheck(opcodeStruct, get, slowPath)
2923-
get(m_scope, t0)
2924-
loadq [cfr, t0, 8], t0
2925-
loadStructureWithScratch(t0, t2, t1)
2926-
loadp %opcodeStruct%::Metadata::m_structure[t5], t1
2927-
bpneq t2, t1, slowPath
2922+
macro loadScopeWithStructureCheck(opcodeStruct, get, metadata, scope, scratch, slowPath)
2923+
get(m_scope, scope)
2924+
loadq [cfr, scope, 8], scope
2925+
loadi JSCell::m_structureID[scope], scratch
2926+
bineq scratch, %opcodeStruct%::Metadata::m_structureID[metadata], slowPath
29282927
end
29292928

29302929
llintOpWithMetadata(op_get_from_scope, OpGetFromScope, macro (size, get, dispatch, metadata, return)
@@ -2957,7 +2956,7 @@ llintOpWithMetadata(op_get_from_scope, OpGetFromScope, macro (size, get, dispatc
29572956

29582957
#gGlobalProperty:
29592958
bineq t0, GlobalProperty, .gGlobalVar
2960-
loadWithStructureCheck(OpGetFromScope, get, .gDynamic) # This structure check includes lexical binding epoch check since when the epoch is changed, scope will be changed too.
2959+
loadScopeWithStructureCheck(OpGetFromScope, get, t5, t0, t1, .gDynamic) # This structure check includes lexical binding epoch check since when the epoch is changed, scope will be changed too.
29612960
getProperty()
29622961

29632962
.gGlobalVar:
@@ -2978,7 +2977,7 @@ llintOpWithMetadata(op_get_from_scope, OpGetFromScope, macro (size, get, dispatc
29782977

29792978
.gGlobalPropertyWithVarInjectionChecks:
29802979
bineq t0, GlobalPropertyWithVarInjectionChecks, .gGlobalVarWithVarInjectionChecks
2981-
loadWithStructureCheck(OpGetFromScope, get, .gDynamic) # This structure check includes lexical binding epoch check since when the epoch is changed, scope will be changed too.
2980+
loadScopeWithStructureCheck(OpGetFromScope, get, t5, t0, t1, .gDynamic) # This structure check includes lexical binding epoch check since when the epoch is changed, scope will be changed too.
29822981
getProperty()
29832982

29842983
.gGlobalVarWithVarInjectionChecks:
@@ -3067,7 +3066,7 @@ llintOpWithMetadata(op_put_to_scope, OpPutToScope, macro (size, get, dispatch, m
30673066

30683067
.pGlobalProperty:
30693068
bineq t0, GlobalProperty, .pGlobalVar
3070-
loadWithStructureCheck(OpPutToScope, get, .pDynamic) # This structure check includes lexical binding epoch check since when the epoch is changed, scope will be changed too.
3069+
loadScopeWithStructureCheck(OpPutToScope, get, t5, t0, t1, .pDynamic) # This structure check includes lexical binding epoch check since when the epoch is changed, scope will be changed too.
30713070
putProperty()
30723071
writeBarrierOnOperands(size, get, m_scope, m_value)
30733072
dispatch()
@@ -3095,7 +3094,7 @@ llintOpWithMetadata(op_put_to_scope, OpPutToScope, macro (size, get, dispatch, m
30953094

30963095
.pGlobalPropertyWithVarInjectionChecks:
30973096
bineq t0, GlobalPropertyWithVarInjectionChecks, .pGlobalVarWithVarInjectionChecks
3098-
loadWithStructureCheck(OpPutToScope, get, .pDynamic) # This structure check includes lexical binding epoch check since when the epoch is changed, scope will be changed too.
3097+
loadScopeWithStructureCheck(OpPutToScope, get, t5, t0, t1, .pDynamic) # This structure check includes lexical binding epoch check since when the epoch is changed, scope will be changed too.
30993098
putProperty()
31003099
writeBarrierOnOperands(size, get, m_scope, m_value)
31013100
dispatch()

‎Source/JavaScriptCore/runtime/CommonSlowPathsInlines.h‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -91,11 +91,15 @@ inline void tryCachePutToScopeGlobal(
9191
return;
9292
}
9393

94-
scope->structure()->didCachePropertyReplacement(vm, slot.cachedOffset());
94+
Structure* structure = scope->structure();
95+
structure->didCachePropertyReplacement(vm, slot.cachedOffset());
9596

96-
ConcurrentJSLocker locker(codeBlock->m_lock);
97-
metadata.m_structure.set(vm, codeBlock, scope->structure());
98-
metadata.m_operand = slot.cachedOffset();
97+
{
98+
ConcurrentJSLocker locker(codeBlock->m_lock);
99+
metadata.m_structureID.setWithoutWriteBarrier(structure);
100+
metadata.m_operand = slot.cachedOffset();
101+
}
102+
vm.writeBarrier(codeBlock);
99103
}
100104
}
101105

@@ -144,9 +148,10 @@ inline void tryCacheGetFromScopeGlobal(
144148
Structure* structure = scope->structure();
145149
{
146150
ConcurrentJSLocker locker(codeBlock->m_lock);
147-
metadata.m_structure.set(vm, codeBlock, structure);
151+
metadata.m_structureID.setWithoutWriteBarrier(structure);
148152
metadata.m_operand = slot.cachedOffset();
149153
}
154+
vm.writeBarrier(codeBlock);
150155
structure->startWatchingPropertyForReplacements(vm, slot.cachedOffset());
151156
}
152157
}

0 commit comments

Comments
 (0)