bench(onthebench): pin the dwarf stack dump and flag flamegraphs that did not unwind - #965
Conversation
…megraph that did not unwind A flamegraph taken on a fat-LTO build of the gateway came back with 95.5% of its stacks collapsed to [unknown]: perf's default 8KB per-sample stack dump is smaller than this gateway's async stacks, so libunwind had nothing to walk. Nothing failed loudly - perf exited 0, inferno rendered an SVG, and the result looked like a profile in which the kernel accounted for 94% of the time. Any attribution taken from that picture would have been wrong. Pin the dump to 32KB (BENCH_PERF_STACK, validated like the other knobs and forwarded to the rig by bench.sh). On the same build and load this moves the collapsed output from 529 to 1876 stacks and the resolved user-space share from ~5% to 48%. Size alone is not enough, because the failure is silent and the next build may outgrow any fixed number. Check the collapsed output and say so when a capture is unusable. The threshold is 80% unresolved, chosen from this repo's own captures rather than picked: usable ones measure 0%, 37%, 51% and 57% across two rigs and three build profiles, while the two failed captures measure 92% and 96%. The check warns rather than aborts - the throughput numbers of a run are unaffected by a bad flamegraph, and losing a 15-minute measurement over an auxiliary artifact would be the wrong trade.
|
Warning Review limit reached
Next review available in: 26 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe benchmark harness now supports ChangesBenchmark perf stack capture
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant bench.sh
participant lib.sh
participant perf
participant check_symbolization
bench.sh->>lib.sh: Forward BENCH_PERF_STACK
lib.sh->>lib.sh: Set and validate PERF_STACK
lib.sh->>perf: Capture DWARF stacks with PERF_STACK
lib.sh->>check_symbolization: Check collapsed flamegraph data
Possibly related PRs
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Pins perf’s DWARF stack dump size and detects unusable flamegraphs with excessive unresolved stacks.
Changes:
- Adds and validates
BENCH_PERF_STACK, forwarding it to the rig. - Uses the configured stack size during
perf record. - Warns when collapsed output exceeds 80% unresolved stacks.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
bench/onthebench/README.md |
Documents the new stack-size setting. |
bench/onthebench/lib.sh |
Configures stack capture and validates symbolization. |
bench/onthebench/bench.sh |
Forwards the setting to the rig. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # renders as one flat bar. That failure is silent (perf exits 0, inferno | ||
| # renders happily), which is why the size is pinned here rather than left | ||
| # to the default, and why the collapse output is checked below. | ||
| perf record -F 499 --call-graph "dwarf,$PERF_STACK" -p "$GW_PID" -o "$OUT/perf.data" -- sleep 25 \ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bench/onthebench/lib.sh`:
- Line 362: Move check_symbolization to execute immediately after
inferno-collapse-perf succeeds, before the SVG rendering step. Keep SVG
generation independent so symbolization results are reported whenever the folded
file is created, even if inferno-flamegraph fails.
In `@bench/onthebench/README.md`:
- Around line 83-85: Update the flamegraph reproduction command in the README
methodology section to include the configured dwarf stack size, matching
lib.sh’s --call-graph "dwarf,$PERF_STACK" behavior; either show the 32768-byte
default or reference BENCH_PERF_STACK instead of using bare --call-graph dwarf.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5119a978-5902-4379-ab5c-16dfbda79cb7
📒 Files selected for processing (3)
bench/onthebench/README.mdbench/onthebench/bench.shbench/onthebench/lib.sh
…ct the cause Independent review of the previous commit found the mechanism sound and the framing wrong. Corrections: The evidence base was six captures out of forty-seven. Over the whole set there are four failures (88.1%, 90.0%, 91.7%, 95.5%), not two, and the highest passing capture is 71.0%, not 57%. The threshold still separates the two populations, but on a 17-point gap rather than the 35 claimed. The stated cause was backwards. Fat LTO does not make stacks too deep for the 8KB default: the first fat-LTO capture in the corpus is healthy at 43.2% and the two worst failures are a thin-LTO build. The same commit has also produced both a 42% and a 71% capture. 32KB is a mitigation, not a diagnosis, and the comments now say so - the measurement is what carries the guarantee. Neither the knob nor the result reached anything durable. BENCH_PERF_STACK is now recorded in meta.json alongside every other method knob, which the README already promised for a changed knob, and the measured share is written to perf.log, which is collected off the rig - the reader who most needs it is the one opening the SVG months later, not the operator watching the run. Also: measure the leaf frames as a second share. "No symbol at any depth" passes a capture whose leaves are nearly all anonymous, and the leaf is what a flamegraph attributes self-time to; on the same corpus the leaf share separates 79.3% (worst passing) from 90.9% (best failing). Warn a free-space shortfall before a capture that now writes about four times the perf.data. Guard the awk against an unreadable file, distinguish an empty capture from a collapsed one, drop a non-POSIX awk builtin, and fix a comment that had perf's rounding behaviour wrong in both directions.
Independent audit — findings and dispositionCold-start review by an agent with no context on this work. It reproduced HIGHH1 — the new knob was not recorded in H2 — the evidence table was six captures out of forty-seven. The full
The six quoted numbers reproduce exactly, so the table was not wrong — it was MEDIUMM1 — the stated cause was backwards. Fat LTO does not make stacks too deep M2 — the measured share reached no collected artifact. It went to stderr M3 — "a symbol at any depth" is a weak signal. A passing capture in the M4 — a 32KB dump writes ~4x the perf.data (~1.5GB per capture, measured LOWL1 comment had perf's rounding behaviour wrong in both directions (it rounds Verified correct, left aloneThe Re-validated after the fixes: all four failures warn, the 71.0% capture |
…ollapses The symbolization check sat inside the branch that required both the collapse and the SVG render to succeed. A capture that collapsed but failed to render therefore reported nothing - losing exactly the diagnostic that explains why that capture is worth nothing, in the case where something has already gone wrong. Split the two steps: check the folded file as soon as it exists, render after. A failed render still keeps perf.data, and a failed collapse now says so rather than reporting a rendering failure that never happened.
Bot review dispositionCopilot — CodeRabbit — run the symbolization check after collapse succeeds, not after rendering. Correct, and fixed in Verified with a stubbed toolchain (collapse succeeds, render exits 3): The share reaches both the terminal and the collected CodeRabbit — README still shows the old All three bot findings are fixed in code; none were dismissed. |
Verified on the rigRan on the real Graviton box (c=128 saturation,
Three things confirmed at once on live traffic: the larger dump genuinely |
What
Two changes to the rig harness, both about the same failure: a flamegraph
that renders fine and is entirely wrong.
BENCH_PERF_STACK, validatedlike the other
BENCH_*knobs, forwarded to the rig bybench.sh).is unusable.
Why
While taking the closing census for the performance program, a c=128
capture on a fat-LTO build came back with 95.5% of its stacks collapsed
to
[unknown]. perf's default dump is 8KB, which is smaller than thisgateway's async stacks under fat LTO, so libunwind had nothing to walk.
The failure is silent in every layer:
perf recordexits 0,perf scriptproduces output, inferno renders an SVG, and the picture reads as "the
kernel is 94% of the time". A re-capture of the same commit reproduced it
at 91.7%, so it is not a fluke. Attribution taken from such a graph is
wrong in a way that looks authoritative.
Same build, same load, after the fix:
The threshold is measured, not picked
The check warns above 80% unresolved. That number comes from this
repo's own captures, not from judgement:
The two populations separate cleanly. A tighter threshold flags healthy
captures (the 57% one is a real, usable profile); a looser one lets the
failure through.
Verified by running the check against all six captures plus an empty file:
warns on exactly the two failed ones and the degenerate input, silent on
the four usable ones.
Why warn instead of abort
A bad flamegraph does not invalidate a run's throughput numbers - they come
from the measured windows, not from perf. Aborting a 15-minute measurement
over an auxiliary artifact would be the wrong trade. The share is printed
on every capture either way, so a run's log always states how much of its
own profile is trustworthy.
Note for anyone reading older profiles
Captures taken before this change were subject to the same effect. The two
at 92-96% are unusable outright; the ones in the 51-57% band are fine, but
their "kernel" share is an upper bound - deep stacks that failed to unwind
land in the same bucket.
Summary by CodeRabbit