Skip to content

Commit ed2c128

Browse files
ConstellationConstellation
committed
[JSC] Introduce op_get_length
https://bugs.webkit.org/show_bug.cgi?id=272271 rdar://126009739 Reviewed by Alexey Shvayka. We found a very silly performance bug: we are getting ArrayProfile from GetById's metadata in DFG. But this metadata gets updated only from LLInt. So we end up not getting this profile data very well in some cases, leading to very bad code generation. Fundamental problem is that we are doing a bit weird ArrayProfile for GetById. Since we care it only when it is "length" property access and we know get_by_id's id at bytecode compile time, we should have specific bytecode which always get ArrayProfile. In this patch, we introduce op_get_length. This is "length" version of get_by_id. And it additionally holds ArrayProfile and LLInt / BaselineJIT / upper tiers always update this properly. Furthermore, iterating ArrayProfile gets further simplified and gets faster because now we do not need to iterate GetById's metadata: we only need to iterate GetLength's metadata. Also, op_get_length's size is smaller than op_get_by_id since we don't embed "length" id number. * Source/JavaScriptCore/assembler/JITOperationList.cpp: (JSC::llintOperations): * Source/JavaScriptCore/bytecode/BytecodeList.rb: * Source/JavaScriptCore/bytecode/BytecodeUseDef.cpp: (JSC::computeUsesForBytecodeIndexImpl): (JSC::computeDefsForBytecodeIndexImpl): * Source/JavaScriptCore/bytecode/CodeBlock.cpp: (JSC::CodeBlock::finishCreation): (JSC::CodeBlock::finalizeLLIntInlineCaches): (JSC::CodeBlock::getArrayProfile): (JSC::CodeBlock::updateAllArrayProfilePredictions): * Source/JavaScriptCore/bytecode/GetByIdMetadata.h: (JSC::GetByIdModeMetadata::setArrayLengthMode): (JSC::GetByIdModeMetadataArrayLength::offsetOfArrayProfile): Deleted. * Source/JavaScriptCore/bytecode/GetByStatus.cpp: (JSC::GetByStatus::computeFromLLInt): * Source/JavaScriptCore/bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.cpp: (JSC::LLIntPrototypeLoadAdaptiveStructureWatchpoint::fireInternal): * Source/JavaScriptCore/bytecode/Opcode.h: * Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp: (JSC::UnlinkedCodeBlock::allocateSharedProfiles): * Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp: (JSC::BytecodeGenerator::emitGetById): * Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp: (JSC::DFG::ByteCodeParser::parseGetById): (JSC::DFG::ByteCodeParser::parseBlock): * Source/JavaScriptCore/dfg/DFGOSRExit.cpp: (JSC::DFG::OSRExit::compileExit): * Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp: (JSC::DFG::callerReturnPC): * Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp: (JSC::FTL::compileStub): * Source/JavaScriptCore/jit/JIT.cpp: (JSC::JIT::privateCompileMainPass): (JSC::JIT::privateCompileSlowCases): * Source/JavaScriptCore/jit/JIT.h: * Source/JavaScriptCore/jit/JITPropertyAccess.cpp: (JSC::JIT::emit_op_get_by_id): (JSC::JIT::emit_op_get_length): (JSC::JIT::emitSlow_op_get_length): * Source/JavaScriptCore/llint/LLIntSlowPaths.cpp: (JSC::LLInt::performLLIntGetByID): (JSC::LLInt::LLINT_SLOW_PATH_DECL): * Source/JavaScriptCore/llint/LLIntSlowPaths.h: * Source/JavaScriptCore/llint/LLIntThunks.cpp: (JSC::LLInt::returnLocationThunk): * Source/JavaScriptCore/llint/LowLevelInterpreter.asm: * Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm: * Source/JavaScriptCore/llint/LowLevelInterpreter64.asm: * Source/JavaScriptCore/runtime/FileBasedFuzzerAgent.cpp: (JSC::FileBasedFuzzerAgent::getPredictionInternal): * Source/JavaScriptCore/runtime/PredictionFileCreatingFuzzerAgent.cpp: (JSC::PredictionFileCreatingFuzzerAgent::getPredictionInternal): Canonical link: https://commits.webkit.org/277165@main
1 parent 2bdec35 commit ed2c128

25 files changed

Lines changed: 241 additions & 89 deletions

‎Source/JavaScriptCore/assembler/JITOperationList.cpp‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -182,6 +182,7 @@ static LLIntOperations llintOperations()
182182
LLINT_OP(llint_generic_return_point)
183183

184184
LLINT_RETURN_LOCATION(op_get_by_id)
185+
LLINT_RETURN_LOCATION(op_get_length)
185186
LLINT_RETURN_LOCATION(op_get_by_val)
186187
LLINT_RETURN_LOCATION(op_put_by_id)
187188
LLINT_RETURN_LOCATION(op_put_by_val)

‎Source/JavaScriptCore/bytecode/BytecodeList.rb‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -352,6 +352,17 @@
352352
modeMetadata: GetByIdModeMetadata,
353353
}
354354

355+
op :get_length,
356+
args: {
357+
dst: VirtualRegister,
358+
base: VirtualRegister,
359+
valueProfile: unsigned,
360+
},
361+
metadata: {
362+
modeMetadata: GetByIdModeMetadata,
363+
arrayProfile: ArrayProfile,
364+
}
365+
355366
op :profile_type,
356367
args: {
357368
targetVirtualRegister: VirtualRegister,
@@ -1419,6 +1430,7 @@
14191430
op :op_call_varargs_return_location
14201431
op :op_construct_varargs_return_location
14211432
op :op_get_by_id_return_location
1433+
op :op_get_length_return_location
14221434
op :op_get_by_val_return_location
14231435
op :op_put_by_id_return_location
14241436
op :op_put_by_val_return_location

‎Source/JavaScriptCore/bytecode/BytecodeUseDef.cpp‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -175,6 +175,7 @@ void computeUsesForBytecodeIndexImpl(const JSInstruction* instruction, Checkpoin
175175
USES(OpToPropertyKeyOrNumber, src)
176176
USES(OpTryGetById, base)
177177
USES(OpGetById, base)
178+
USES(OpGetLength, base)
178179
USES(OpGetByIdDirect, base)
179180
USES(OpGetPrototypeOf, value)
180181
USES(OpInById, base)
@@ -485,6 +486,7 @@ void computeDefsForBytecodeIndexImpl(unsigned numVars, const JSInstruction* inst
485486
DEFS(OpConstruct, dst)
486487
DEFS(OpTryGetById, dst)
487488
DEFS(OpGetById, dst)
489+
DEFS(OpGetLength, dst)
488490
DEFS(OpGetByIdDirect, dst)
489491
DEFS(OpGetByIdWithThis, dst)
490492
DEFS(OpGetByValWithThis, dst)

‎Source/JavaScriptCore/bytecode/CodeBlock.cpp‎

Lines changed: 10 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -481,6 +481,7 @@ bool CodeBlock::finishCreation(VM& vm, ScriptExecutable* ownerExecutable, Unlink
481481
LINK(OpToThis)
482482

483483
LINK(OpGetById)
484+
LINK(OpGetLength)
484485

485486
LINK(OpEnumeratorNext)
486487
LINK(OpEnumeratorInByVal)
@@ -1376,6 +1377,10 @@ void CodeBlock::finalizeLLIntInlineCaches()
13761377
clearIfNeeded(metadata.m_modeMetadata, "get by id"_s);
13771378
});
13781379

1380+
m_metadata->forEach<OpGetLength>([&] (auto& metadata) {
1381+
clearIfNeeded(metadata.m_modeMetadata, "get length"_s);
1382+
});
1383+
13791384
m_metadata->forEach<OpTryGetById>([&] (auto& metadata) {
13801385
StructureID oldStructureID = metadata.m_structureID;
13811386
if (!oldStructureID || vm.heap.isMarked(oldStructureID.decode()))
@@ -1540,6 +1545,11 @@ void CodeBlock::finalizeLLIntInlineCaches()
15401545
LLIntPrototypeLoadAdaptiveStructureWatchpoint::clearLLIntGetByIdCache(instruction->as<OpGetById>().metadata(this).m_modeMetadata);
15411546
break;
15421547
}
1548+
case op_get_length: {
1549+
dataLogLnIf(Options::verboseOSR(), "Clearing LLInt property access.");
1550+
LLIntPrototypeLoadAdaptiveStructureWatchpoint::clearLLIntGetByIdCache(instruction->as<OpGetLength>().metadata(this).m_modeMetadata);
1551+
break;
1552+
}
15431553
case op_iterator_open: {
15441554
dataLogLnIf(Options::verboseOSR(), "Clearing LLInt iterator open property access.");
15451555
LLIntPrototypeLoadAdaptiveStructureWatchpoint::clearLLIntGetByIdCache(instruction->as<OpIteratorOpen>().metadata(this).m_modeMetadata);
@@ -2717,13 +2727,6 @@ ArrayProfile* CodeBlock::getArrayProfile(const ConcurrentJSLocker&, BytecodeInde
27172727

27182728
#undef CASE
27192729

2720-
case OpGetById::opcodeID: {
2721-
auto bytecode = instruction->as<OpGetById>();
2722-
auto& metadata = bytecode.metadata(this);
2723-
if (metadata.m_modeMetadata.mode == GetByIdMode::ArrayLength)
2724-
return &metadata.m_modeMetadata.arrayLengthMode.arrayProfile;
2725-
break;
2726-
}
27272730
default:
27282731
break;
27292732
}
@@ -2887,15 +2890,6 @@ void CodeBlock::updateAllArrayProfilePredictions()
28872890
++index;
28882891
};
28892892

2890-
m_metadata->forEach<OpGetById>([&] (auto& metadata) {
2891-
if (metadata.m_modeMetadata.mode == GetByIdMode::ArrayLength)
2892-
process(metadata.m_modeMetadata.arrayLengthMode.arrayProfile);
2893-
else {
2894-
// We reserve an index per GetById whether or not it's currently in ArrayLength mode.
2895-
++index;
2896-
}
2897-
});
2898-
28992893
#define VISIT(__op) \
29002894
m_metadata->forEach<__op>([&] (auto& metadata) { process(metadata.m_arrayProfile); });
29012895

‎Source/JavaScriptCore/bytecode/GetByIdMetadata.h‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -49,8 +49,9 @@ struct GetByIdModeMetadataUnset {
4949
static_assert(sizeof(GetByIdModeMetadataUnset) == 12);
5050

5151
struct GetByIdModeMetadataArrayLength {
52-
static ptrdiff_t offsetOfArrayProfile() { return OBJECT_OFFSETOF(GetByIdModeMetadataArrayLength, arrayProfile); }
53-
ArrayProfile arrayProfile;
52+
unsigned padding1;
53+
unsigned padding2;
54+
unsigned padding3;
5455
};
5556
static_assert(sizeof(GetByIdModeMetadataArrayLength) == 12);
5657

@@ -140,7 +141,6 @@ inline void GetByIdModeMetadata::setUnsetMode(Structure* structure)
140141
inline void GetByIdModeMetadata::setArrayLengthMode()
141142
{
142143
mode = GetByIdMode::ArrayLength;
143-
new (&arrayLengthMode.arrayProfile) ArrayProfile;
144144
// Prevent the prototype cache from ever happening.
145145
hitCountForLLIntCaching = 0;
146146
}

‎Source/JavaScriptCore/bytecode/GetByStatus.cpp‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,10 +75,23 @@ GetByStatus GetByStatus::computeFromLLInt(CodeBlock* profiledBlock, BytecodeInde
7575
break;
7676
}
7777

78+
case op_get_length: {
79+
auto& metadata = instruction->as<OpGetLength>().metadata(profiledBlock);
80+
// FIXME: We should not just bail if we see a get_by_id_proto_load.
81+
// https://bugs.webkit.org/show_bug.cgi?id=158039
82+
if (metadata.m_modeMetadata.mode != GetByIdMode::Default)
83+
return GetByStatus(NoInformation, false);
84+
structureID = metadata.m_modeMetadata.defaultMode.structureID;
85+
86+
identifier = &vm.propertyNames->length;
87+
break;
88+
}
89+
7890
case op_try_get_by_id:
7991
structureID = instruction->as<OpTryGetById>().metadata(profiledBlock).m_structureID;
8092
identifier = &(profiledBlock->identifier(instruction->as<OpTryGetById>().m_property));
8193
break;
94+
8295
case op_get_by_id_direct:
8396
structureID = instruction->as<OpGetByIdDirect>().metadata(profiledBlock).m_structureID;
8497
identifier = &(profiledBlock->identifier(instruction->as<OpGetByIdDirect>().m_property));

‎Source/JavaScriptCore/bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.cpp‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,10 @@ void LLIntPrototypeLoadAdaptiveStructureWatchpoint::fireInternal(VM& vm, const F
7878
clearLLIntGetByIdCache(instruction->as<OpGetById>().metadata(m_owner.get()).m_modeMetadata);
7979
break;
8080

81+
case op_get_length:
82+
clearLLIntGetByIdCache(instruction->as<OpGetLength>().metadata(m_owner.get()).m_modeMetadata);
83+
break;
84+
8185
case op_iterator_open:
8286
clearLLIntGetByIdCache(instruction->as<OpIteratorOpen>().metadata(m_owner.get()).m_modeMetadata);
8387
break;

‎Source/JavaScriptCore/bytecode/Opcode.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,7 @@ static constexpr unsigned bitWidthForMaxBytecodeStructLength = WTF::getMSBSetCon
104104
macro(OpGetByVal) \
105105
macro(OpEnumeratorGetByVal) \
106106
macro(OpGetById) \
107+
macro(OpGetLength) \
107108
macro(OpGetByIdWithThis) \
108109
macro(OpTryGetById) \
109110
macro(OpGetByIdDirect) \
@@ -135,6 +136,7 @@ static constexpr unsigned bitWidthForMaxBytecodeStructLength = WTF::getMSBSetCon
135136
macro(OpCallIgnoreResult) \
136137

137138
#define FOR_EACH_OPCODE_WITH_SIMPLE_ARRAY_PROFILE(macro) \
139+
macro(OpGetLength) \
138140
macro(OpGetByVal) \
139141
macro(OpInByVal) \
140142
macro(OpPutByVal) \

‎Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -338,7 +338,6 @@ void UnlinkedCodeBlock::allocateSharedProfiles(unsigned numBinaryArithProfiles,
338338
FOR_EACH_OPCODE_WITH_SIMPLE_ARRAY_PROFILE(COUNT)
339339
#undef COUNT
340340
numberOfArrayProfiles += m_metadata->numEntries<OpIteratorNext>();
341-
numberOfArrayProfiles += m_metadata->numEntries<OpGetById>();
342341
m_arrayProfiles = FixedVector<UnlinkedArrayProfile>(numberOfArrayProfiles);
343342
}
344343

‎Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2735,7 +2735,10 @@ RegisterID* BytecodeGenerator::emitGetById(RegisterID* dst, RegisterID* base, co
27352735
{
27362736
ASSERT_WITH_MESSAGE(!parseIndex(property), "Indexed properties should be handled with get_by_val.");
27372737

2738-
OpGetById::emit(this, kill(dst), base, addConstant(property), nextValueProfileIndex());
2738+
if (property == m_vm.propertyNames->length)
2739+
OpGetLength::emit(this, kill(dst), base, nextValueProfileIndex());
2740+
else
2741+
OpGetById::emit(this, kill(dst), base, addConstant(property), nextValueProfileIndex());
27392742
return dst;
27402743
}
27412744

0 commit comments

Comments
 (0)