Skip to content

Commit 24527bb

Browse files
kmiller68kmiller68
committed
[Wasm] Optimize JITCallee publication
https://bugs.webkit.org/show_bug.cgi?id=320747 rdar://183747593 Reviewed by Yusuke Suzuki. Publishing a freshly compiled wasm function held CalleeGroup::m_lock across the two most expensive steps in the sequence: a per-callsite icache flush for every outgoing call, and barrierCacheOnAllThreads(), which forces a kernel round-trip per tracked wasm thread. Neither reads nor mutates CalleeGroup state, yet every other compiler thread blocked behind them. This does three things. First, correctness: Relying on LinkBuffer's finalize flush to cover a function's outgoing call sites is unsafe: those targets are written after finalization, so an adjacent live instruction sharing a cache line with a call site can pull the stale pre-link bytes into the instruction cache. relink{Jump,Call,TailCall} and repatchNearCall gain a RepatchingInfo template parameter so a repatch can skip its flush, with flush{Jump,Call,TailCall} counterparts to perform it later, and wasm's LinkBuffers are constructed with CacheFlushOnFinalize::No. A single whole-range cacheFlush then runs after every outgoing call is linked. machineCodeCopy already static_asserts that a memcpy repatch carries no Flush bit, so the split cannot be quietly broken. Second, the critical section: installOptimizedCallee now links outgoing calls under the lock, drops it via DropLockForScope to flush and synchronize, then re-acquires to publish and relink callers. The window is safe because the callee is not yet in m_optimizedCallees, so no execution thread can reach unflushed code. It is not, however, undiscoverable: m_pendingPublishCallees keeps it findable as a direct-call target, so a thread that retires one of its callees during the window still relinks it rather than leaving it calling code that is about to be freed. Since an OMG install for the same index can land mid-window, the decision to redirect callers is recomputed after re-acquiring; acting on the stale one would relink callers to BBQ over a newer OMG and tier the function back down. Third, the readers: Maintains the lock-free tier-up peeks from 299970@main for the two hot paths — the IPInt loop back-edge and the BBQ loop's OMG check — now that the critical section they contended with is short. Both are sound because a callee only appears in m_optimizedCallees after its code is flushed and published. The staging slot that commit needed is gone: m_pendingPublishCallees subsumes it, and more cleanly, since an unpublished callee is simply absent rather than visible-but-half-baked. m_bbqCalleeLock has to stay because ThreadSafeWeakOrStrongPtr is a tagged union whose strong->weak transition and assignment are both non-atomic. The IPInt prologue reader deliberately stays on m_lock: it is only reached when IPInt cannot interpret the function at all, so it must find whichever JIT callee exists, and it is disabled in the default configuration. barrierCacheOnAllThreads() no longer "claims" to invalidate any instruction cache, only context-synchronizes, and is rewritten on top of a new Thread::barrierInstructionCache(). The old implementation passed crossModifyingCodeFence() to sendMessage, which runs the functor on the calling thread, not the target, so it only ever worked via the suspend/resume the message mechanism performs incidentally. On ARM64 Darwin the new primitive reads the target's register state instead, whose returning ERET is the context-synchronizing event. Elsewhere it stays as a thread suspend/resume sequence. OSR entry installation shares all of this rather than duplicating it, as it did before. An OMGOSREntryCallee is not the function's entrypoint even though it shares its index, so one predicate gates both the recursive-call resolution and the caller relinking; its reservation in m_osrEntryCallees also registers it, which is safe because that map is relinking bookkeeping and only BBQCallee::setOSREntryCallee, after the flush, makes the code enterable. Also removes EntryPlan::m_unlinkedWasmToWasmCalls and IPIntPlan's relink loop over it, which were dead (parseAndCompileMetadata never populated the vector). No new tests, no behavior change. Covered by existing tests. Canonical link: https://commits.webkit.org/318386@main
1 parent e7f11c8 commit 24527bb

23 files changed

Lines changed: 472 additions & 249 deletions

‎Source/JavaScriptCore/assembler/ARM64Assembler.h‎

Lines changed: 31 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3973,21 +3973,41 @@ class ARM64Assembler {
39733973
// and jump patching as they're modifying existing (linked) code,
39743974
// so the address being provided is correct for relative address
39753975
// computation.
3976+
template<RepatchingInfo repatch = jitMemcpyRepatchFlush>
39763977
static void relinkJump(void* from, void* to)
39773978
{
3978-
relinkJumpOrCall<BranchType_JMP>(reinterpret_cast<int*>(from), reinterpret_cast<const int*>(from), to);
3979-
cacheFlush(from, sizeof(int));
3979+
relinkJumpOrCall<BranchType_JMP, noFlush(repatch)>(reinterpret_cast<int*>(from), reinterpret_cast<const int*>(from), to);
3980+
if constexpr ((*repatch).contains(RepatchingFlag::Flush))
3981+
flushJump(from);
39803982
}
3981-
3983+
3984+
template<RepatchingInfo repatch = jitMemcpyRepatchFlush>
39823985
static void relinkCall(void* from, void* to)
39833986
{
3984-
relinkJumpOrCall<BranchType_CALL>(reinterpret_cast<int*>(from) - 1, reinterpret_cast<const int*>(from) - 1, to);
3985-
cacheFlush(reinterpret_cast<int*>(from) - 1, sizeof(int));
3987+
relinkJumpOrCall<BranchType_CALL, noFlush(repatch)>(reinterpret_cast<int*>(from) - 1, reinterpret_cast<const int*>(from) - 1, to);
3988+
if constexpr ((*repatch).contains(RepatchingFlag::Flush))
3989+
flushCall(from);
39863990
}
39873991

3992+
template<RepatchingInfo repatch = jitMemcpyRepatchFlush>
39883993
static void relinkTailCall(void* from, void* to)
39893994
{
3990-
relinkJump(from, to);
3995+
relinkJump<repatch>(from, to);
3996+
}
3997+
3998+
static void flushJump(void* from)
3999+
{
4000+
cacheFlush(from, sizeof(int));
4001+
}
4002+
4003+
static void flushCall(void* from)
4004+
{
4005+
cacheFlush(reinterpret_cast<int*>(from) - 1, sizeof(int));
4006+
}
4007+
4008+
static void flushTailCall(void* from)
4009+
{
4010+
flushJump(from);
39914011
}
39924012

39934013
#if ENABLE(JUMP_ISLANDS)
@@ -4336,7 +4356,7 @@ class ARM64Assembler {
43364356
}
43374357
}
43384358

4339-
template<BranchType type>
4359+
template<BranchType type, RepatchingInfo repatch = jitMemcpyRepatch>
43404360
static void relinkJumpOrCall(int* from, const int* fromInstruction, void* to)
43414361
{
43424362
static_assert(type == BranchType_JMP || type == BranchType_CALL);
@@ -4353,7 +4373,7 @@ class ARM64Assembler {
43534373
if (imm19 == 8)
43544374
condition = invert(condition);
43554375

4356-
linkConditionalBranch<IndirectBranch>(condition, from - 1, fromInstruction - 1, to);
4376+
linkConditionalBranch<IndirectBranch, repatch>(condition, from - 1, fromInstruction - 1, to);
43574377
return;
43584378
}
43594379

@@ -4366,7 +4386,7 @@ class ARM64Assembler {
43664386
if (imm19 == 8)
43674387
op = !op;
43684388

4369-
linkCompareAndBranch<IndirectBranch>(op ? ConditionNE : ConditionEQ, opSize == Datasize_64, rt, from - 1, fromInstruction - 1, to);
4389+
linkCompareAndBranch<IndirectBranch, repatch>(op ? ConditionNE : ConditionEQ, opSize == Datasize_64, rt, from - 1, fromInstruction - 1, to);
43704390
return;
43714391
}
43724392

@@ -4378,12 +4398,12 @@ class ARM64Assembler {
43784398
if (imm14 == 8)
43794399
op = !op;
43804400

4381-
linkTestAndBranch<IndirectBranch>(op ? ConditionNE : ConditionEQ, bitNumber, rt, from - 1, fromInstruction - 1, to);
4401+
linkTestAndBranch<IndirectBranch, repatch>(op ? ConditionNE : ConditionEQ, bitNumber, rt, from - 1, fromInstruction - 1, to);
43824402
return;
43834403
}
43844404
}
43854405

4386-
linkJumpOrCall<type>(from, fromInstruction, to);
4406+
linkJumpOrCall<type, repatch>(from, fromInstruction, to);
43874407
}
43884408

43894409
static int* addressOf(void* code, AssemblerLabel label)

‎Source/JavaScriptCore/assembler/ARMv7Assembler.h‎

Lines changed: 33 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2730,36 +2730,66 @@ class ARMv7Assembler {
27302730
// the write and executable address for call and jump patching
27312731
// as they're modifying existing (linked) code, so the address being
27322732
// provided is correct for relative address computation.
2733+
template<RepatchingInfo repatch = jitMemcpyRepatchFlush>
27332734
static void relinkJump(void* from, void* to)
27342735
{
27352736
ASSERT(!(reinterpret_cast<intptr_t>(from) & 1));
27362737
ASSERT(!(reinterpret_cast<intptr_t>(to) & 1));
27372738

27382739
linkJumpAbsolute(reinterpret_cast<uint16_t*>(from), reinterpret_cast<uint16_t*>(from), to);
27392740

2740-
cacheFlush(reinterpret_cast<uint16_t*>(from) - 5, 5 * sizeof(uint16_t));
2741+
if constexpr ((*repatch).contains(RepatchingFlag::Flush))
2742+
flushJump(from);
27412743
}
27422744

2745+
template<RepatchingInfo repatch = jitMemcpyRepatchFlush>
27432746
static void relinkCall(void* from, void* to)
27442747
{
27452748
ASSERT(isEven(from));
27462749

27472750
uint16_t* location = reinterpret_cast<uint16_t*>(from);
27482751
if (isBL(location - 2)) {
27492752
linkBranch(location, location, makeEven(to), BranchWithLink::Yes);
2750-
cacheFlush(location - 2, 2 * sizeof(uint16_t));
2753+
if constexpr ((*repatch).contains(RepatchingFlag::Flush))
2754+
cacheFlush(location - 2, 2 * sizeof(uint16_t));
27512755
return;
27522756
}
27532757

2754-
setPointer<jitMemcpyRepatchFlush>(location - 1, to);
2758+
setPointer<repatch>(location - 1, to);
27552759
}
27562760

2761+
template<RepatchingInfo repatch = jitMemcpyRepatchFlush>
27572762
static void relinkTailCall(void* from, void* to)
27582763
{
27592764
ASSERT(isEven(from));
27602765

27612766
uint16_t* location = reinterpret_cast<uint16_t*>(from);
27622767
linkBranch(location, location, to, BranchWithLink::No);
2768+
if constexpr ((*repatch).contains(RepatchingFlag::Flush))
2769+
flushTailCall(from);
2770+
}
2771+
2772+
// Unlike the other ports, relinkTailCall here writes a direct branch rather than
2773+
// delegating to relinkJump's absolute jump sequence, so flushJump and flushTailCall
2774+
// cover different ranges.
2775+
static void flushJump(void* from)
2776+
{
2777+
cacheFlush(reinterpret_cast<uint16_t*>(from) - 5, 5 * sizeof(uint16_t));
2778+
}
2779+
2780+
static void flushCall(void* from)
2781+
{
2782+
uint16_t* location = reinterpret_cast<uint16_t*>(from);
2783+
if (isBL(location - 2)) {
2784+
cacheFlush(location - 2, 2 * sizeof(uint16_t));
2785+
return;
2786+
}
2787+
cacheFlush(location - 5, 4 * sizeof(uint16_t));
2788+
}
2789+
2790+
static void flushTailCall(void* from)
2791+
{
2792+
uint16_t* location = reinterpret_cast<uint16_t*>(from);
27632793
cacheFlush(location - 2, 2 * sizeof(uint16_t));
27642794
}
27652795

‎Source/JavaScriptCore/assembler/AbstractMacroAssembler.h‎

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1021,15 +1021,29 @@ class AbstractMacroAssembler : public AbstractMacroAssemblerBase {
10211021
AssemblerType::relinkJump(jump.dataLocation(), destination.dataLocation());
10221022
}
10231023

1024-
template<PtrTag callTag, PtrTag destTag>
1024+
template<RepatchingInfo repatch = jitMemcpyRepatchFlush, PtrTag callTag, PtrTag destTag>
10251025
static void repatchNearCall(CodeLocationNearCall<callTag> nearCall, CodeLocationLabel<destTag> destination)
10261026
{
10271027
switch (nearCall.callMode()) {
10281028
case NearCallMode::Tail:
1029-
AssemblerType::relinkTailCall(nearCall.dataLocation(), destination.dataLocation());
1029+
AssemblerType::template relinkTailCall<repatch>(nearCall.dataLocation(), destination.dataLocation());
1030+
return;
1031+
case NearCallMode::Regular:
1032+
AssemblerType::template relinkCall<repatch>(nearCall.dataLocation(), destination.untaggedPtr());
1033+
return;
1034+
}
1035+
RELEASE_ASSERT_NOT_REACHED();
1036+
}
1037+
1038+
template<PtrTag callTag>
1039+
static void flushNearCall(CodeLocationNearCall<callTag> nearCall)
1040+
{
1041+
switch (nearCall.callMode()) {
1042+
case NearCallMode::Tail:
1043+
AssemblerType::flushTailCall(nearCall.dataLocation());
10301044
return;
10311045
case NearCallMode::Regular:
1032-
AssemblerType::relinkCall(nearCall.dataLocation(), destination.untaggedPtr());
1046+
AssemblerType::flushCall(nearCall.dataLocation());
10331047
return;
10341048
}
10351049
RELEASE_ASSERT_NOT_REACHED();

‎Source/JavaScriptCore/assembler/LinkBuffer.cpp‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -578,7 +578,8 @@ void LinkBuffer::performFinalization()
578578

579579
s_profileCummulativeLinkedSizes[static_cast<unsigned>(m_profile)] += m_size;
580580
s_profileCummulativeLinkedCounts[static_cast<unsigned>(m_profile)]++;
581-
MacroAssembler::cacheFlush(code(), m_size);
581+
if (m_cacheFlushOnFinalize == CacheFlushOnFinalize::Yes)
582+
MacroAssembler::cacheFlush(code(), m_size);
582583
}
583584

584585
#if DUMP_LINK_STATISTICS

‎Source/JavaScriptCore/assembler/LinkBuffer.h‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -165,8 +165,11 @@ class LinkBuffer {
165165
#undef COUNT_LINKBUFFER_PROFILE
166166
static constexpr unsigned numberOfProfilesExcludingTotal = numberOfProfiles - 1;
167167

168-
LinkBuffer(MacroAssembler& macroAssembler, void* ownerUID, Profile profile = Profile::Uncategorized, JITCompilationEffort effort = JITCompilationMustSucceed)
168+
enum class CacheFlushOnFinalize : bool { No, Yes };
169+
170+
LinkBuffer(MacroAssembler& macroAssembler, void* ownerUID, Profile profile = Profile::Uncategorized, JITCompilationEffort effort = JITCompilationMustSucceed, CacheFlushOnFinalize cacheFlushOnFinalize = CacheFlushOnFinalize::Yes)
169171
: m_ownerUID(ownerUID)
172+
, m_cacheFlushOnFinalize(cacheFlushOnFinalize)
170173
, m_profile(profile)
171174
{
172175
linkCode(macroAssembler, effort);
@@ -465,6 +468,7 @@ ALLOW_NONLITERAL_FORMAT_END
465468
#endif
466469
bool m_alreadyDisassembled { false };
467470
bool m_isThunk { false };
471+
CacheFlushOnFinalize m_cacheFlushOnFinalize { CacheFlushOnFinalize::Yes };
468472
bool m_isRewriting { false };
469473
Profile m_profile { Profile::Uncategorized };
470474
CodePtr<LinkBufferPtrTag> m_code;

‎Source/JavaScriptCore/assembler/RISCV64Assembler.h‎

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1632,23 +1632,43 @@ class RISCV64Assembler {
16321632
cacheFlush(location, sizeof(uint32_t) * 8);
16331633
}
16341634

1635+
template<RepatchingInfo repatch = jitMemcpyRepatchFlush>
16351636
static void relinkJump(void* from, void* to)
16361637
{
16371638
uint32_t* location = static_cast<uint32_t*>(from);
16381639
LinkJumpImpl::apply(location, to);
1639-
cacheFlush(location, sizeof(uint32_t) * 2);
1640+
if constexpr ((*repatch).contains(RepatchingFlag::Flush))
1641+
flushJump(from);
16401642
}
16411643

1644+
template<RepatchingInfo repatch = jitMemcpyRepatchFlush>
16421645
static void relinkCall(void* from, void* to)
16431646
{
16441647
uint32_t* location = static_cast<uint32_t*>(from);
16451648
LinkCallImpl::apply(location, to);
1646-
cacheFlush(location, sizeof(uint32_t) * 2);
1649+
if constexpr ((*repatch).contains(RepatchingFlag::Flush))
1650+
flushCall(from);
16471651
}
16481652

1653+
template<RepatchingInfo repatch = jitMemcpyRepatchFlush>
16491654
static void relinkTailCall(void* from, void* to)
16501655
{
1651-
relinkJump(from, to);
1656+
relinkJump<repatch>(from, to);
1657+
}
1658+
1659+
static void flushJump(void* from)
1660+
{
1661+
cacheFlush(static_cast<uint32_t*>(from), sizeof(uint32_t) * 2);
1662+
}
1663+
1664+
static void flushCall(void* from)
1665+
{
1666+
cacheFlush(static_cast<uint32_t*>(from), sizeof(uint32_t) * 2);
1667+
}
1668+
1669+
static void flushTailCall(void* from)
1670+
{
1671+
flushJump(from);
16521672
}
16531673

16541674
static void replaceWithVMHalt(void* where)

‎Source/JavaScriptCore/assembler/X86Assembler.h‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6407,21 +6407,30 @@ class X86Assembler {
64076407
setPointer(static_cast<char*>(code) + where.offset(), value);
64086408
}
64096409

6410+
template<RepatchingInfo repatch = jitMemcpyRepatchFlush>
64106411
static void relinkJump(void* from, void* to)
64116412
{
64126413
setRel32(from, to);
64136414
}
6414-
6415+
6416+
template<RepatchingInfo repatch = jitMemcpyRepatchFlush>
64156417
static void relinkCall(void* from, void* to)
64166418
{
64176419
setRel32(from, to);
64186420
}
64196421

6422+
template<RepatchingInfo repatch = jitMemcpyRepatchFlush>
64206423
static void relinkTailCall(void* from, void* to)
64216424
{
6422-
relinkJump(from, to);
6425+
relinkJump<repatch>(from, to);
64236426
}
64246427

6428+
// Flushing is a no-op on x86: instruction fetch is coherent with data writes, so the relink
6429+
// functions above never flush either.
6430+
static void flushJump(void*) { }
6431+
static void flushCall(void*) { }
6432+
static void flushTailCall(void*) { }
6433+
64256434
static void repatchPointer(void* where, void* value)
64266435
{
64276436
setPointer(where, value);

‎Source/JavaScriptCore/wasm/WasmBBQPlan.cpp‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,9 @@ void BBQPlan::work()
9999
Ref<BBQCallee> callee = BBQCallee::create(functionIndexSpace, m_moduleInformation->nameSection().get(functionIndexSpace), Ref { m_profiledCallee });
100100
std::unique_ptr<InternalFunction> function = compileFunction(m_functionIndex, callee.get(), context, unlinkedWasmToWasmCalls);
101101

102-
LinkBuffer linkBuffer(*context.wasmEntrypointJIT, callee.ptr(), LinkBuffer::Profile::WasmBBQ, JITCompilationCanFail);
102+
// The finished code is patched further (wasm call-site linking in installOptimizedCallee) and
103+
// flushed once afterward, so skip LinkBuffer's finalize instruction-cache flush.
104+
LinkBuffer linkBuffer(*context.wasmEntrypointJIT, callee.ptr(), LinkBuffer::Profile::WasmBBQ, JITCompilationCanFail, LinkBuffer::CacheFlushOnFinalize::No);
103105
if (linkBuffer.didFailToAllocate()) [[unlikely]] {
104106
fail(makeString("Out of executable memory while tiering up function at index "_s, m_functionIndex.rawIndex()), CompilationError::OutOfMemory);
105107
return;
@@ -131,10 +133,13 @@ void BBQPlan::work()
131133
callee->setPCToCodeOriginMap(WTF::move(context.pcToCodeOriginMap));
132134
}
133135

136+
CalleeGroup::CallerCallsiteFlushes deferredFlushes;
134137
{
135138
Locker locker { m_calleeGroup->m_lock };
136-
m_calleeGroup->installOptimizedCallee(locker, m_moduleInformation, m_functionIndex, callee.copyRef(), function->outgoingJITDirectCallees);
139+
m_calleeGroup->installOptimizedCallee(locker, m_moduleInformation, m_functionIndex, callee.copyRef(), function->outgoingJITDirectCallees, deferredFlushes);
137140
}
141+
// Flush the repatched callsites now that m_lock is released, keeping the icache flushes out of the critical section.
142+
deferredFlushes.flush();
138143
{
139144
Locker locker { m_profiledCallee->tierUpCounter().m_lock };
140145
m_profiledCallee->tierUpCounter().setCompilationStatus(mode(), IPIntTierUpCounter::CompilationStatus::Compiled);

0 commit comments

Comments
 (0)