Repository navigation
fix(callgrind): end a call that returns with SP below its frame - #63
Draft
not-matthias wants to merge 1 commit into
Draft
not-matthias wants to merge 1 commit into
not-matthias wants to merge 1 commit into
Conversation
Callgrind only treated a ret as a return when SP moved back above the frame of the top call, or stayed equal and hit the recorded return address. A callee that returns with SP lower than it was called with, because it builds a stack frame for its caller, was turned into a 'RET w/o CALL' jump and then emulated as a call back into its caller. Everything the caller did afterwards was recorded under the callee. V8's Sparkplug prologue (Builtins_BaselineOutOfLinePrologue) works this way. With the node binary instrumented, every JS function's callees ended up under the shared prologue, which tied all JS functions into one cycle and showed non-recursive functions as recursive. A ret to the return address recorded for the top call now pops that call even when SP is still below its frame. Instruction totals are unchanged. Tests: ret_below_frame reproduces the pattern with a hand-written amd64 prologue, ret_below_frame_node checks that the Sparkplug prologue never calls into JIT code under node --always-sparkplug.
Merging this PR will not alter performance
Comparing 
Footnotes
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TLDR: A callee that returns with SP lower than it was called with (it builds a frame for its caller) was recorded as calling back into its caller, and the caller's work showed up under it. Callgrind now pops the call when the
retgoes to its recorded return address. (amd64 tests; arm64 uses the same path but isn't covered by CI.)Builtins_BaselineOutOfLinePrologue) works this way. Once the node binary is no longer obj-skipped, every JS function's callees end up under the shared prologue, which ties all JS functions into one cycle and makes non-recursive functions look recursive.setup_bbccgets a third "real return" rule: SP below the top frame and target == that frame'sret_addr→ pop that one frame. Equal and higher SP cases are unchanged.ret_below_framegives 291,775 Ir onmasterand with this change.ret_below_frame: hand-written amd64 prologue. Onmaster:frame_prologue -> with_prologue_a'2 -> leaf_a. Now:with_prologue_a -> leaf_a.ret_below_frame_node:node --always-sparkplug; the prologue must not call into JIT code. Onmaster: 66 such callees. Now: 0. Skipped whennodeisn't installed.callgrind/testssuite: 30/30 pass locally.--separate-callers=4): false JS→JS edges 3 → 0. Without--separate-callers, cross-function false edges remain because shared V8 builtins (Call*,CompileLazy,Execution::Call) merge callers. A stacked PR will address that.Part of COD-4155. Related: CodSpeedHQ/codspeed-node#121, CodSpeedHQ/codspeed#617