vm: drop leaveFrame's unused *Frame parameter to unpin f from memory

[mparrett]

leaveFrame never read its *Frame. attrPopFrame pops the alloc-attribution shadow stack by position, so the parameter was inert. But runLoop passed it from a deferred closure, and f is also reassigned as the dispatch loop descends into callees and returns to parents, so capturing it forced the compiler to box the interpreter's hottest pointer:

$ go build -gcflags='-m -m' ./pkg/vm
pkg/vm/vm.go:827:7: (*Frame).runLoop capturing by ref: f (addr=false assign=true width=8)

assign=true is the operative half. Every f.ip, f.sp and f.code.code[f.ip] in the dispatch loop reads through an indirection, to supply an argument the callee discards.

Dropping the parameter lets the defer be a direct call. The capture is gone from escape analysis after the change, and no call site loses information, since none of the four was passing anything that got read.

Why the parameter is now documented as absent

defer leaveFrame(f) and defer func() { leaveFrame(f) }() are not equivalent: deferred arguments evaluate at defer time, which is the entry frame, while the closure evaluates at return time, which is whichever frame is current. Today that difference is unobservable because the argument is unused. It stops being unobservable the moment someone gives the parameter meaning, so both the defer and leaveFrame itself now carry a comment saying why there is no frame to pass.

On performance

This came out of measuring #700's entry-boundary cost, and the honest summary is that the effect is not demonstrated. Locally on an M2, (some #(> % 999998) (range 1000000)) under hyperfine (3 warmup, 20 runs) gave order-dependent results — main 1.03 ± 0.03x faster with A first, this change 1.07 ± 0.09x faster with B first — and the pkg/vm micros ran at ±20–30% σ with no family reaching significance. That is thermal, not signal.

So judge this one on the dead parameter and the removed capture; any number would be a bonus. I would run it through the perf-repeat EPYC lane to see whether the indirection shows up on quieter hardware, and will label it unless you would rather not spend the lane time.

One related note for #700: the ~2% previously attributed to gating this defer cannot have come from the capture. Gating a defer whose body still references f keeps the by-ref capture, so that measurement was moving something else.

Verification

• capturing by ref line absent from go build -gcflags='-m -m' ./pkg/vm
• go test ./pkg/vm ./pkg/rt ./test/... green, including TestGogenAOTDiff
• builds clean for linux/amd64, js/wasm, plan9/amd64, wasip1/wasm

[mparrett]

Repeat A/B results, run 31315465796. Posting by hand: perf-pr-repeat.yml uploads its evidence and writes a job summary but has no comment step, so nothing lands on the PR. Sending a patch for that separately.

Run quality is good. N=7 interleaved ABBA on an EPYC 7763, go1.26.5, 53 comparable families, none flaky, none missing, base 8a8222d0 vs head 7c56acfd. The register-only anchor sat at 1.245–1.247 ns across all 14 snapshots, so the runner itself was quiet.

The targeted families improve, slightly

family	median	cycles
FuncInvoke/Direct	−0.75%	7/7 negative
MultiArity/1Arg	−0.70%	6/7
FuncInvoke/Closure	−0.58%	7/7 negative

Directionally right and consistent, and about the size one removed indirection should buy. Nowhere near gateable.

Three families regress, also consistently

family	median	cycles
SeqIteration/List/01000	+8.30%	8.33–8.56, all positive
VectorConj/ArrayVector/10	+7.63%	the one confirm-k hit
SeqIteration/List/10	+7.20%	all positive
FrameDispatch	+4.97%	5.2, 5.3, 4.6, 4.6, 4.5, 5.1, 5.0

Four families would gate at 6%, two at 8%, none at 10%.

What I think this is, and what I cannot show

I am not going to call this noise. FrameDispatch at 7/7 positive with a 0.8-point spread is not the heavy tail the shadow phase is looking for.

But the change cannot alter behavior — leaveFrame ignored its *Frame argument, so deleting the parameter changes no control flow and no data. A 5–8% swing from a semantically inert edit points at code placement rather than logic. VectorConj splitting within one run supports that: +7.63% on ArrayVector/10 against −1.07% on PersistentVector/1000.

I cannot separate those two hypotheses with this instrument, which is the same gap as the uncalibrated budget in #705. So: happy to hold this PR until a comment-only control has gone through the same lane. That control is worth running regardless, since it is what #705 asks for and this run is nearly it.

One incidental note for #700 and for whoever picks up the SeqIteration/List regression: that family has now moved several percent under three unrelated changes — the *lg-trace* migration, #645, and a no-op parameter deletion. That pattern reads as layout sensitivity in the family rather than three genuine regressions, and it would make the family a cheap first subject for the placement control.

[mparrett]

Retracting the reading in my results comment above. I said the regression pointed at code placement rather than logic, on the grounds that deleting an unread parameter cannot change behavior. An A/A run since then says otherwise.

#708 touches one workflow file and no Go source, so the lane built base and head from identical code on the same tier and settings. With code held identical, the families that moved here sat at zero:

family	this PR	A/A, identical code
SeqIteration/List/01000	+8.30%	+0.05%
SeqIteration/List/10	+7.20%	−0.10%
VectorConj/ArrayVector/10	+7.63%	−0.81%
FrameDispatch	+4.97%	−0.32%

Across all 53 families the A/A run's largest median was 1.69% and it was clean at every candidate budget. Numbers in #705.

So the deltas here are attributable to this branch's binary, not to the harness. Different runner draw, so it is not a perfectly controlled comparison, but a gap between 0.05% and 8.30% on the same family is not what runner variation looks like at N=7.

My "cannot add work" argument was the wrong shape. The change removes no semantic operation, but it does change what the compiler can do in runLoop — inlining, spilling, register allocation are all free to move once the closure and its captured f are gone. That is a real cost even with identical behavior, and it is the leading explanation now.

I would rather understand that than merge it. The escape-analysis win is real and the parameter is genuinely dead, but not at 8% on seq iteration, and a change this small should not need a footnote. Leaving the PR open and taking a look at what the compiler does differently at the leaveFrame call sites. Happy to drop it instead if you would rather not carry an open PR for a cleanup this size.

One correction to my own aside as well: I suggested SeqIteration/List might be layout-sensitive, since it had moved under several unrelated changes. It measured +0.05% on the A/A, so that theory does not hold in the form I put it.

[mparrett]

One number in my retraction above needs fixing, though the conclusion comes out stronger rather than weaker.

I cited a 1.69% A/A floor from a single run. Two more A/A runs since then show that was a lucky draw — one family hit 13.43% with identical code. Details on #705. The floor is per-family, not global.

That matters here because the families this PR moved are the stable ones, across all three A/A runs:

family	A/A medians (identical code)	max A/A	this PR
FrameDispatch	−0.3, +0.2, +0.3	0.3%	+4.97%
SeqIteration/List/01000	+0.1, +0.9, +1.0	1.0%	+8.30%
SeqIteration/List/100	−0.0, −1.1, −0.9	1.1%	+8.14%
VectorConj/ArrayVector/10	−0.8, +0.2, −0.4	0.8%	+7.63%
SeqIteration/List/10	−0.1, −0.4, +1.9	1.9%	+7.20%

FrameDispatch is sixteen times its own observed A/A range here. The memory-bound families that are noisy under A/A — VectorConj/*/1000, VectorCreation/*/10000 — barely moved on this PR at all, which is the opposite of what a noisy run would look like.

So the read stands: these deltas belong to the binary, and the likely mechanism is a changed inlining or register-allocation decision in runLoop once the closure and its captured f are gone. Still leaving the PR open and looking at what the compiler does differently rather than merging it.

[mparrett]

Chased the mechanism. It is not what I guessed, and the answer has a consequence beyond this PR.

Not inlining, not the defer

go build -gcflags='-m' ./pkg/vm, base against head, differs by three lines, all of which are the closure ceasing to exist:

< can inline (*Frame).runLoop.func1
< func literal does not escape
< inlining call to leaveFrame

Nothing new became inlinable. And -gcflags='-d=defer' reports a stack-allocated defer on both sides, so the defer kind did not change either.

It is register allocation, and the function grew

go tool nm -size on the two pkg/vm test binaries, the only meaningful change:

(*Frame).runLoop   25072 -> 25232   (+160 bytes)

Which is the change working as intended. Pinning f to memory was the thing the capture was doing; removing the capture lets the allocator keep the dispatch loop's hottest pointer in a register, and that re-solves allocation across a 25 KB function. The new solution is 160 bytes larger and, on the evidence, slower on dispatch-bound work — FrameDispatch +4.97%, SeqIteration/List +7 to +8%, against per-family A/A noise of 0.3% and 1.0% respectively (#705).

So the closure was not merely wasteful. It was holding a large hot loop in an equilibrium that happened to be good, and the escape-analysis "fix" trades a real indirection for a worse global allocation. Removing an indirection from a 25 KB loop is not a local change.

Consequence for folding runLoop into Run

I raised that fold elsewhere as the cheap win against the entry-boundary cost: the two functions exist so the leaveFrame defer covers ~46 error returns, and folding removes one of two defers per entry.

This result argues against it. The fold makes the hot function bigger, and +160 bytes on this loop was apparently enough to cost 5 to 8%. Anyone attempting it should expect the defer saving to be swamped by whatever allocation does with a larger region, and should measure before assuming otherwise.

If the direction is right at all it may be the opposite one — a smaller runLoop, with the cold error paths lifted out so allocation has less to solve. That is a real refactor rather than a cleanup, and I have not tried it.

Where that leaves this PR

Happy to close it. The escape-analysis win is verified and the parameter is genuinely dead, but the net effect is negative on this workload and the finding is worth more written down than merged. Leaving it open a few days in case you want it kept as the reference for the effect, then closing unless you say otherwise.

[mparrett]

Converting to draft to signal the intent in the last finding.

[nnunley]

Read this one carefully because it touches the 2021 core, and the change itself is safe: leaveFrame's body is only if allocAttrEnabled { attrPopFrame() } (vm.go:747-751), attrPopFrame takes no arguments and pops by position (allocattr.go:101), and leaveFrame is unexported with no callers outside pkg/vm. defer-with-arg versus defer-closure is unobservable precisely because the parameter is dead.

I also traced the enter/leave balance, since runLoop does if entering { enterFrame(f) } but defers leaveFrame() unconditionally (vm.go:829-833). It balances: on the error path releaseFailedFrames returns the resumed parent before calling leaveFrame on it (vm.go:765-775), so the re-entry with entering=false rides the still-live push and its defer pops it. No new imbalance. One doc nit if it ever revives: "Balances the enterFrame above" is slightly loose, since it may balance a push from a previous runLoop invocation.

Agree with holding it, on your own evidence — FrameDispatch +4.97%, SeqIteration/List/10 +7.20%, /01000 +8.30%, VectorConj/ArrayVector/10 +7.63%, against A/A noise of 0.3-1.9%. The mechanism you identified is the part worth keeping regardless of what happens to the diff: (*Frame).runLoop grew 25072 → 25232 bytes and register allocation re-solved worse once the captured f was freed. Removing an indirection from a 25 KB dispatch loop is not a local change, and "do not fold runLoop into Run expecting a win" is a durable finding.

If it stays as a documented reference rather than a merge, that seems right to me. I have not re-run the benchmarks — the numbers are yours, but they are A/A-controlled and self-consistent.

[mparrett]

#719 removes runLoop's leaveFrame defer statement outright (along with Run's cleanup defer), after profiling showed the dominant cost is not the capture but deferreturn scaffolding: since Go 1.21 a function that contains a defer statement pays the _panic.nextDefer walk on every return, even when the defer is never registered. With the statement gone there is no capture left to unpin, so this PR has nothing to apply to if #719 lands. That strengthens the close recommendation already on this thread; the mechanism write-up here (regalloc sensitivity of the ~25 KB loop) stays useful as the reference #719's perf-repeat gate exists to check against.