-
Notifications
You must be signed in to change notification settings - Fork 55
[JSC] SamplingProfiler: attribute a sample taken in C code to the JIT frame and call site that made the call - #579
[JSC] SamplingProfiler: attribute a sample taken in C code to the JIT frame and call site that made the call #579robobun wants to merge 4 commits into
Conversation
... PC is in C code JIT code does not store vm.topCallFrame before it calls an operation that cannot throw (AssemblyHelpers::prepareCallOperation only does so when ASSERT_ENABLED). A sample taken while the PC is in such a callee, for example libm's sin reached from DFG code through sinDouble, used topCallFrame anyway. A dead topCallFrame made the walk bail and dropped the sample. An older one credited the sample to the caller. Walk the frame pointer chain from the machine frame to the first frame that is either topCallFrame or a live JS frame, and fall back to topCallFrame when neither is found within 32 frames.
...dress in the JIT frame that called it The FTL emits calls with no side effects (sinDouble, stdPowDouble, the operationToInt32 slow paths) without a call site index store, so the index in the frame is stale and the sample lands on whatever call site the frame stored last: with the callee inlined, that is the caller's. Report the address the C code returns to (from the C frame below the JS frame, or from the stack words above the stack pointer when the leaf has no frame) and let findPC map it to the call site and its inline stack.
...C-to-code-origin map Check for a live JS frame before the topCallFrame match, so that a frame that is both still gets its return address. Accept a stack word as the return address only when the frame's PC-to-code-origin map covers it. Stale return addresses into thunks or other code blocks below the live slot are rejected that way.
|
No actionable comments were generated in the recent review. 🎉 i️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 5 included reviews per hour; 1 remains after this review. WalkthroughThe sampling profiler now validates machine-stack frame pointers, walks C frames, maps JIT return addresses to call sites, and records the resulting attribution PC. Bun builds enable PC-to-code-origin mapping before profiler creation. ChangesSampling profiler call-site attribution
Merge Risk: ⚪ Minimal · up to This change improves sampling-profiler attribution for C code invoked by JIT code and records mapped call-site PCs. No current merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Findings marked 🟡 are optional suggestions and need no follow-up push.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 callFrameForSampleInCCode runs while the JS thread is suspended (between suspend at line 469 and resume at line 537), yet it calls PCToCodeOriginMap::findPC, which constructs a CodeOrigin on every decompression step and heap-allocates an OutOfLineCodeOrigin whenever the bytecode index bits exceed 1 << s_freeBitsAtTop; if the suspended JS thread holds the bmalloc/TZone lock the profiler thread deadlocks and the process hangs — the base branch only ever called findPC after resume. Fix: do not call anything that can allocate while the target is suspended — replace both findPC calls here with an allocation-free range check (e.g. compare against m_pcRangeStart/m_pcRangeEnd, or add a PCToCodeOriginMap::coversPC(void*) that returns bool without building a CodeOrigin).
Extended reasoning...
takeSample suspends the JS thread (line 469) and the comment at 471-472 states no malloc is allowed because the JS thread may hold the malloc lock. In the C-code branch it now calls callFrameForSampleInCCode (line 516), which at lines 145 and 153 invokes pcToCodeOriginMap->findPC(pc). PCToCodeOriginMap::findPC (PCToCodeOriginMap.cpp:287) builds CodeOrigin(currentBytecodeIndex, currentInlineCallFrame) on every loop iteration; CodeOrigin::buildCompositeValue (CodeOrigin.h:250-252) does new OutOfLineCodeOrigin(...) (WTF_MAKE_TZONE_ALLOCATED) when bytecodeIndex.asBits() >= 1 << s_freeBitsAtTop — with EFFECTIVE_ADDRESS_WIDTH=48 that is 65536, i.e. bytecode offset ≥ ~16K given the checkpoint shift, which real-world large hot functions reach. The temporary is then destroyed with delete. If the suspended JS thread was inside bmalloc/TZone allocation when the sample fired, the profiler thread blocks on that allocator lock forever and the JS thread is never resumed: full-process hang. On the base branch findPC is only reached from processUnverifiedStackTraces after...
Verification: normal — the new call happens inside the no-malloc window and can heap-allocate. SamplingProfiler.cpp:469 suspends the JS thread and the comment at 471–472 states "While the JSC thread is suspended, we can't do things like malloc because the JSC thread may be holding the malloc lock." Line 540 marks the end of that window ("We can now use data structures that malloc ... again."). The new call...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 (optional) On ARM64 (and other link-register CPUs) the leaf-case return PC is never on the stack: bl/blr puts it in LR only, and a leaf like libm sin never spills it, so this SP-to-fp scan finds nothing and returnPC stays null — the FTL pure-call site attribution this PR adds is x86_64-only for the very leaf math calls it targets. Fix: capture MachineContext::linkRegister(registers) in takeSample, pass it (untagged) into callFrameForSampleInCCode, and when previousFrame is null test it against pcToCodeOriginMap->findPC before falling back to the stack scan.
Extended reasoning...
previousFrame == nullptr means the very first frame (the sampled fp) is already the live JS frame — i.e. the C callee did not push its own frame. On x86_64, call pushed the return address at what is now between the callee's SP and the JIT fp, so the word scan at 149-157 recovers it. On ARM64 the JIT emits blr xN; the return-into-JIT address goes into x30 and nowhere else (the JIT frame's own saved LR at fp+8 is the JIT's caller, not this call). A leaf such as sin/cos/pow keeps it in x30 and never touches fp, so the sampled fp is the JIT CallFrame, previousFrame is null, and the words between SP and fp are only the JIT frame's spill slots — no return address to find. returnPC stays null, topPC stays the C machinePC, topCodeBlock->findPC(topPC) at line 762 returns nullopt, and processing falls back to the frame's stale callSiteIndex — exactly the mis-attribution the PR says it fixes for FTL callWithoutSideEffects (sinDouble, stdPowDouble). Base branch behaves the same on this path, so this is the fix not applying on ARM64/RISC-V rather than a regression,...
Verification: nit — The leaf-case return-PC recovery at SamplingProfiler.cpp:149-157 assumes the return address is on the stack between SP and fp, which is x86_64 call semantics; on ARM64 bl/blr writes the return address only to x30/LR, so a leaf C callee that neither pushes a frame nor spills LR leaves nothing on the stack for the scan to find, and returnPC stays null. Code path: `previousFrame ==...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 (optional) The return PC read from the C stack is used raw, but on arm64e it is PAC-signed, so pcToCodeOriginMap->findPC(pc) (a numeric range check) never matches and the new call-site attribution silently never fires on that platform, leaving the FTL-inline mis-attribution this change is meant to fix. Fix: strip pointer authentication before using the value, e.g. void* pc = removeCodePtrTag(reinterpret_cast<CallerFrameAndPC*>(previousFrame)->returnPC); and likewise removeCodePtrTag(*word) at line 153-154, matching how machinePC/llintPC are untagged at lines 492/495.
Extended reasoning...
CallerFrameAndPC::returnPC at fp[1] in a compiler-generated C frame on CPU(ARM64E) holds a B-key-signed LR (the C prologue does pacibsp before stp x29,x30). PCToCodeOriginMap::findPC (PCToCodeOriginMap.cpp:265-267) does uintptr_t pcAsInt = bit_cast<uintptr_t>(pc); if (!(m_pcRangeStart <= pcAsInt && pcAsInt <= m_pcRangeEnd)) return nullopt; — a signed pointer has high bits set and falls outside the range, so returnPC stays null on both the previous-frame path (line 144-146) and the stack-word scan (line 153-154). topPC then keeps the C machinePC, which topCodeBlock->findPC at line 762 also rejects, so startIndex stays 0 and the top frame is attributed via the stored call-site index — exactly the pre-change behaviour the PR says is wrong for FTL callWithoutSideEffects. The same function already strips tags for the other captured PCs (instructionPointer->untaggedPtr() line 492, removeCodePtrTag(...) line 495, assertIsNotTagged line 496); the new reads do not. Bun's current CI matrix (macOS arm64, not arm64e) is unaffected, but the code is not gated on...
Verification: normal — On CPU(ARM64E) the return-PC recovery added by this change never matches, so the FTL call-site/inline-stack attribution this PR is meant to add silently does not fire on that platform (the frame recovery still works; only returnPC stays null and topPC falls back to the C machinePC). At Source/JavaScriptCore/runtime/SamplingProfiler.cpp:144 the new code reads the saved LR from a...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 nit (optional): When !ENABLE(JIT), the stackPointer parameter, the maxStackWordsToScanForReturnPC constant, and previousFrame (assigned at line 165 but only read inside the #if ENABLE(JIT) block) are all unused, which can break -Werror cloop/no-JIT builds. Fix: add UNUSED_PARAM(stackPointer) and mark the two locals unused (or move them inside the #if), matching this file's existing convention at lines 624 and 768 for the same guard.
Extended reasoning...
callFrameForSampleInCCode wraps every use of stackPointer, maxStackWordsToScanForReturnPC, and every read of previousFrame in #if ENABLE(JIT) (lines 135-157). In a !ENABLE(JIT) configuration (e.g. the JSCOnly cloop port, which this file supports — it already has #if ENABLE(JIT) branches with UNUSED_PARAM fallbacks at lines 624 and 768), Clang/GCC emit -Wunused-parameter for stackPointer and -Wunused-but-set-variable for previousFrame, and under WebKit's -Werror those become build failures. Base branch had no such parameter or locals here.
Verification: nit — In callFrameForSampleInCCode at Source/JavaScriptCore/runtime/SamplingProfiler.cpp:122, when !ENABLE(JIT): - stackPointer (param, line 122) is used only at line 149, inside #if ENABLE(JIT) (lines 135-158) → -Wunused-parameter. - previousFrame (line 129) is written at line 166 (previousFrame = frame;, outside the guard) but read only at line 143 (inside the guard) →... | nit — In...
Preview Builds
|
... code compiled from now on Bun starts the sampling profiler from three places (--cpu-prof, the v8::CpuProfiler shim, bun:jsc). Set the flag here so all of them get the same attribution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review found no new issues
No new issues were found in this update; 4 findings from earlier reviews are still open above.
Uh oh!
There was an error while loading. Please reload this page.
Problem
sinreached from DFG code throughsinDouble. A loop overMath.sqrt(i) * Math.sin(i)for 1.5 s at a 1 ms interval gets 754 samples instead of about 1300, with gaps of 2.5 to 20 ms between samples, and the hot function gets 115 self samples instead of about 490. With an async caller, the samples land on the caller instead (main 298, hot 30).takeSample(SamplingProfiler.cpp:431) starts the walk atvm.topCallFramewhen the PC is outside JIT and LLInt code. JIT code does not storetopCallFramebefore an operation that cannot throw:AssemblyHelpers::prepareCallOperationstores it only whenASSERT_ENABLED, and the FTL'scallWithoutSideEffectscalls have nocallPreflight. SotopCallFrameis whatever the last call frame tracer or native thunk stored. A dead frame makes the walk bail (the sample is dropped). An older frame is credited with the sample.Fix
topCallFrame. A C leaf leaves the JIT frame in the frame pointer register, and a C function with a frame links back to it. Fall back totopCallFrameafter 32 frames, which is the previous behavior.PCToCodeOriginMapcovers.processUnverifiedStackTracesthen maps it throughfindPCto the call site and its inline stack. The stored call site index cannot do this: the FTL emits pure calls (sinDouble,stdPowDouble) without a call site store, so a callee inlined into its caller is credited to the caller.--cpu-proftest suite (13 tests) passes.Background
takeSamplesuspends the JS thread, reads its registers, and walks call frames withFrameWalker. Each frame records its CodeBlock and the call site index stored in the frame'sargumentCountIncludingThishigh word. The walk bails if a frame's CodeBlock is not in the CodeBlockSet.PCToCodeOriginMapmaps a PC in JIT code to aCodeOrigin, which includes the inline call frame for DFG and FTL. It is built whenVM::shouldBuilderPCToCodeOriginMapping()is set.VM::ensureSamplingProfilernow sets it underUSE(BUN_JSC_ADDITIONS), so every sampler Bun starts (--cpu-prof, thev8::CpuProfilershim,bun:jsc) gets the map for code compiled from then on. Upstream only sets it for--useSamplingProfiler.processUnverifiedStackTracesruns later on the JS thread. It usesfindPC(topPC)for the top frame when the PC is in that frame's code, otherwise the frame's call site index.