perf(vm): keep defer scaffolding out of the dispatch hot path - #719
Conversation
Self-reviewNo actionable findings. The defer-wrapper split preserves normal, error, and panic cleanup ordering. Verified with:
All required GitHub checks pass; only the repeat benchmark remains pending. Non-blocking suggestion: commit a regression test for the attribution-enabled panic path, since existing tests do not exercise it directly. |
|
The repeat A/B came back (run 31540713790, N=7 interleaved, sign verified against the raw base/head samples in
The trade also shows end to end: a pure Taking that trade and marking ready for review: the shapes #700 tracks are the consumer-visible ones, and they are the ones that improve. If the dispatch loss matters more than expected, the follow-up lever is shrinking |
|
Consumer-shaped follow-up on the dispatch-vs-invoke trade: xsofy's headless native engine bench (
Noise is high on this box (one main-leg round caught a load spike), but the interleaved pattern is consistent: the branch stack is never worse and typically 15–30% better per game turn. The |
Since Go 1.21 deferreturn walks the unified _panic.nextDefer machinery, so a function that merely contains a defer statement pays return-path scaffolding even when the defer is never registered. Run and runLoop each carried one on every host->VM entry: Run's panic-cleanup defer and runLoop's leaveFrame defer. Both only preserve allocation-attribution balance and frame-pool hygiene on a Go panic — the panic itself is re-raised either way. Hoist them into wrappers taken only when allocation attribution is on (runChainProtected, runLoopAttr), leaving Run/runChain/runLoopInner with no defer statements at all. Without attribution, a Go panic now leaks the chain's pooled frames to GC — exactly what pre-#645 did when a panic unwound past runBytecodeCallTarget's ReleaseFrame. leaveFrame ignores its argument (attribution keeps its own frame stack), so runLoopAttr can leave on the loop's behalf without tracking its final frame. On the #700 some-over-range workload (1e6 elements, interleaved medians, darwin/arm64) this recovers roughly half of the tip-vs-v1.12.2 gap; micro families need a perf-repeat pass (FrameDispatch/SeqIteration regalloc sensitivity, cf. #706 mechanism notes). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
runLoopInner's error arms inlined their constructors (fmt varargs setup, struct literals, source-map lookups) into the ~25KB hot function, adding code the register allocator must work around (#706/#719 FrameDispatch fragility). Extract them into //go:noinline helpers: same messages, same handleError routing, same control flow — only the construction bodies move out of line. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
64f95ba to
af272ff
Compare
nooga
left a comment
There was a problem hiding this comment.
Reviewed the dispatch-path changes. No correctness bugs found — three low-severity notes inline, none blocking.
Verification: go build, go vet, go test ./... -count=1, go test -race ./pkg/vm/, and LG_ALLOC_ATTR=1 go test ./pkg/vm/ ./pkg/rt/ all pass. Traced attribution push/pop balance by hand across every exit of runLoopInner (root return, descend via OP_INVOKE/OP_TAIL_CALL, OP_RETURN unwind, error return, resume-after-handler, panic, nested Run re-entry) — the runChainProtected + runLoopAttr split is balance-equivalent to the old unconditional defers in every case. frameRunState still stays on Run's stack in both paths, so no new heap alloc from passing &state into the wrappers.
The perf win reproduces: (some (fn [x] (< 9999999 x)) (range 1000000)), 3 interleaved reps on darwin/arm64 — main 277/324/254 ms vs PR 212/212/202 ms.
|
All three review notes are addressed in #730, stacked on this branch — the runChain fold ( |
runLoopInner's error arms inlined their constructors (fmt varargs setup, struct literals, source-map lookups) into the ~25KB hot function, adding code the register allocator must work around (#706/#719 FrameDispatch fragility). Extract them into //go:noinline helpers: same messages, same handleError routing, same control flow — only the construction bodies move out of line. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-free Two structural notes from #719's review, together because they meet at runLoopAttr: runLoop was a dispatcher too big to inline (cost 144 vs budget 80), so the entry path paid Run -> runChain -> runLoop -> runLoopInner — one real call more than needed on every host->VM entry and error-resume iteration. Hoist the allocAttrEnabled gate into runChain's loop and call runLoopAttr/runLoopInner directly; runLoop goes away. leaveFrame took a *Frame it never read, and the doc comment leaned on that: in OP_RETURN there is a window after ReleaseFrame(child) where state.current still points at the pooled frame, so the first real use of the parameter would be a use-after-release. Drop the parameter — the invariant is now structural, and runLoopAttr's defer no longer needs a closure. Interleaved go-bench medians (N=6, vs #719 head): FrameDispatch 0.94x, FuncInvoke/Closure 0.97x, Direct 1.00x — flat to slightly better. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
runLoopInner's error arms inlined their constructors (fmt varargs setup, struct literals, source-map lookups) into the ~25KB hot function, adding code the register allocator must work around (#706/#719 FrameDispatch fragility). Extract them into //go:noinline helpers: same messages, same handleError routing, same control flow — only the construction bodies move out of line. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Since Go 1.21,
deferreturnwalks the same_panic.nextDefermachinery the panic path uses, so a function that merely contains a defer statement pays return-path scaffolding on every call — even when the defer is never registered at runtime. CPU profiles of the #700 workload showruntime.(*_panic).nextDeferandruntime.(*_panic).startattributed toFrame.Run's normal returns, on both current main and a #645 revert build.#645 put two such defers on every host-to-VM entry:
Run's panic-cleanup defer andrunLoop'sleaveFramedefer. Both exist only to keep allocation-attribution balance and frame-pool hygiene when a Go panic unwinds through the VM; the panic itself is re-raised either way. Native functions that invoke a bytecode callback per element (some,reducewith a closure) pay the entry cost once per element, which is where most of #700's consumer-visible regression comes from.This change hoists both defers into wrappers that are entered only when allocation attribution is on (
runChainProtected,runLoopAttr), leavingRun,runChain, andrunLoopInnerwith no defer statements at all.leaveFrameignores its argument (attribution keeps its own frame stack), so the wrapper can leave on the loop's behalf without tracking which frame the loop ends on.Behavior note: without attribution enabled, a Go panic now leaks the chain's pooled frames to GC instead of returning them to the pool. That matches pre-#645 behavior exactly — a panic skipped
runBytecodeCallTarget'sReleaseFramethe same way. The attribution tests and #645's frame-lifecycle tests cover the gated path.Measured on the #700 shape,
(some (fn [x] (< 9999999 x)) (range 1000000)), darwin/arm64, interleaved runs: main 89.9 ms → this branch 72.4 ms, against v1.12.2's 60.1 ms — about 59% of the gap on that workload.Draft on purpose: the local box could not adjudicate the micro families.
FuncInvoke/DirectandClosureimproved consistently across runs, butFrameDispatch/SeqIterationswung both ways inside local noise, and the #706 investigation showedrunLooprestructures can shift register allocation in either direction. Theperf-repeatlabel is the point of this draft — if those families hold, this is ready for review.Verified:
pkg/vm,pkg/rt, andtest/suites; linux, js/wasm, and plan9 builds.