docs(perf): -C target-cpu=native measures neutral, and the return leg is what proves it - #228
Conversation
… is what proves it The structural-waste search was one axis; how the compiler EMITS the code is a separate one, and it was unexamined. Nothing in the workspace sets target-cpu, so every build targets baseline x86-64 while this host offers AVX2 and BMI2 -- which looks like free headroom. It is not. A-B-A, frame_cost_probe on Super Mario 64, --release --features fast-scheduler: A baseline (x86-64) 93.777 / 93.689 ms B RUSTFLAGS="-C target-cpu=native" 93.399 / 93.056 ms A baseline, again 92.068 ms THE THIRD A LEG CAME IN BELOW BOTH B LEGS. A two-leg comparison would have reported a ~0.5% win; the return leg shows it was drift. Mean A 93.178 against mean B 93.227 -- 0.9995x, neutral. Unsurprising in hindsight: the hot code is scalar integer interpretation and pointer chasing, not the vectorizable arithmetic AVX2 would help. The portability cost never had to be argued, since a target-cpu=native binary does not run on older hosts and that would be a real price for a measured nothing. Added to the ruled-out list as 5b so it is not retried. PGO is named as the one codegen lever still untested, with the explicit warning NOT to assume it is neutral because this was: it works on branch layout and inlining driven by a real profile rather than on instruction selection, and interpreters are the workload it classically helps most. It needs two full LTO rebuilds plus an instrumented run and changes the release process rather than a config line, so it is recorded as an open avenue rather than attempted in passing. Refs #55.
|
Warning Review limit reached
Next review available in: 22 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughUpdates performance documentation with neutral ChangesCode-generation documentation
Estimated code review effort: 1 (Trivial) | ~3 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 8 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (8 passed)
Comment |
![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 `@docs/performance.md`:
- Around line 629-631: Revise the conclusion around the “-C target-cpu=native”
result and the corresponding explanation near the codegen-levers discussion to
scope the finding only to this benchmark workload. State that A-B-A showed no
measurable gain here, and label claims about instruction-selection binding or
pointer chasing as hypotheses unless supported by differential profiling
evidence; do not present aggregate results as proof of mechanism.
- Around line 858-860: Update the portability statement in the target-cpu=native
discussion to say that binaries may not run on older hosts, rather than
asserting they always do not run. Preserve the explanation about host-specific
instructions and potential illegal-instruction failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e29c0406-d57b-4f59-8c9e-f62364fb39de
📒 Files selected for processing (1)
docs/performance.md
…or it was not Review round one on #228. Both adopted, and the first is a repeat of a mistake this session had already named. I explained the null result with a mechanism -- "the hot code is scalar integer interpretation and pointer chasing, not the vectorizable arithmetic AVX2 would help". The A-B-A establishes no measurable gain ON THIS BENCHMARK. It does not establish that the emulator is generally not instruction-selection-bound, and an aggregate frame time conflates every cost in the frame, so it cannot isolate a cause at all. That is the same shape as the earlier "the profile over-predicted" claim in the VI work: an untested story attached to a measurement, which is how a plausible mechanism turns into a repeated citation. The explanation is kept -- it is consistent with the per-line survey, where no vectorizable inner loop appears anywhere -- but it is now labelled an INFERENCE, with what would actually settle it (differential profiling of the two builds rather than one number from each). The conclusion is scoped to the benchmark in both the ruled-out entry and the section. Second: "does not run on older hosts" overstated it. target-cpu=native MAY emit instructions an older host does not implement, and such a binary MAY fail there with an illegal instruction -- it depends on which features LLVM selects and what the host supports. Reworded, and the portability point stands on being a real risk rather than a certainty. Gates: fmt, clippy -D warnings, test --workspace, rustdoc -D warnings, check_en_us.sh, markdownlint -- ALL-GATES-OK.
|
Round one. Both adopted — and the first is a repeat of a mistake this session had already named, which is worth saying plainly. 1. "Limit the conclusion to this workload; mark the explanation as inference" — ADOPTED, and correct. I wrote that the null result was "unsurprising in hindsight: the hot code is scalar integer interpretation and pointer chasing, not the vectorizable arithmetic AVX2 would help." The A-B-A establishes no measurable gain on this benchmark. It does not establish that the emulator is generally not instruction-selection-bound, and an aggregate frame time conflates every cost in the frame, so it cannot isolate a cause at all. That is the same shape as the "the profile over-predicted" claim I had to retract in the VI work two PRs ago: an untested story attached to a real measurement, which is precisely how a plausible mechanism becomes a repeated citation. Catching it twice in one session says the reflex is the problem, not the individual sentence. The explanation is kept — it is consistent with the per-line survey, where no vectorizable inner loop appears anywhere — but it is now labelled an inference, with what would actually settle it stated: differential profiling of the two builds, not one number from each. The conclusion is scoped to the benchmark in both the ruled-out entry and the section body. 2. "Qualify the portability claim" — ADOPTED. "Does not run on older hosts" overstated it. Gates: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/performance.md`:
- Around line 629-631: Update the accuracy-ledger entry corresponding to the
target-cpu=native finding described in the performance documentation. Record the
benchmark, A-B-A comparison method, measured result and drift interpretation,
and explicitly capture the portability and PGO limitations while noting the
ruled-out alternatives required by the ledger’s evidence rules.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f9e0460c-0dfb-46b5-beae-4dcb843f11dc
📒 Files selected for processing (1)
docs/performance.md
…ps asking Review round two on #228. The request was to file the target-cpu=native A-B-A in docs/accuracy-ledger.md. Declined -- for the third time in this series, which is the actual problem and is what this commit fixes. The ledger states its own scope: "Three things, and nothing else" -- constants the hardware documentation does not supply, known-wrong emulated behavior, and ruled-out approaches. All three are about the emulated machine. A compiler flag that changed no emulated behavior is none of them, and filing it there would blur what the file is for. The underlying concern is right, though: the evidence must be recorded in a ruled-out list with its method. It already is -- docs/performance.md carries its own "Ruled out by measurement -- do not retry" list, and target-cpu=native is item 5b with the benchmark, the A-B-A table, the drift interpretation, the portability qualification and the PGO caveat. The finding was in a ruled-out list all along; only the file was in question. So rather than reject a third time and wait for a fourth, the ledger now states the boundary explicitly and points at docs/performance.md. The boundary was previously discoverable only by reading both files and noticing that one of them explains the split -- which is not discoverable at all. Gates: fmt, clippy -D warnings, test --workspace, rustdoc -D warnings, check_en_us.sh, markdownlint -- ALL-GATES-OK.
|
Round two. Rejected as a move — adopted as a discoverability fix, because this is the third time the same request has come and that is the real signal. Why not the ledger
All three are about the emulated machine. Why the underlying concern is right anywayThe substance of the request — this must be recorded in a ruled-out list, with benchmark, method, result, drift interpretation, and limitations — is correct, and it already is.
So the finding was in a ruled-out list the whole time. Only the file was ever in question. What I changed insteadRejecting a third time and waiting for a fourth would be the wrong response to a repeated signal. The ledger now states the boundary explicitly and points at
Previously the split was explained only inside Gates: |
Antigravity review (Gemini via Ultra)This PR documents performance benchmark results showing Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
…easure it PGO was the last untested codegen lever, named in #228 as the most promising one because interpreters are the workload it classically helps most. It was tested. It is a 4.96% regression. FIRST IT NEEDED A HARNESS, and that is a finding in itself. frame_cost_probe is a #[test], and a test cannot be built with -Cprofile-use in this workspace: tests require panic = "unwind" while the release profile is panic = "abort", and RUSTFLAGS applies the flag to every crate in the graph, so they collide with "requires panic strategy abort which is incompatible with this crate's strategy of unwind". That survives cargo clean --release, so it is structural rather than stale artifacts, and the same command with RUSTFLAGS unset builds and runs fine. The binary path has no such requirement, so this adds crates/rustyn64-frontend/examples/frame_bench.rs -- the same measurement shaped so it can be instrumented and rebuilt. An example rather than a [[bin]] so a plain cargo build does not carry a benchmark into every developer's build. Full cycle: instrument, run, llvm-profdata merge (18.6 MB from 50 raw files), rebuild with -Cprofile-use, A-B-A in one sitting over 120 frames of Super Mario 64: A baseline 98.323 ms B PGO 104.060 / 102.918 ms A baseline, again 98.764 / 98.704 ms Mean A 98.597, mean B 103.489 -- PGO IS 4.96% SLOWER. The worst baseline leg is still faster than the best PGO leg, so the legs do not overlap and this is not the drift that caught target-cpu=native in #228. Two integrity checks: the instrumented run cost 141.6 ms/frame, the expected ~44% overhead, and all three builds retired an identical 219,075,686 instructions, so they executed exactly the same work. WHY is inference, not measurement, and is labelled as such: this workspace already builds with lto = "fat" and codegen-units = 1, giving LLVM whole-program visibility, and PGO's profile-driven inlining and block layout override choices fat LTO had already made globally. Consistent with the result and with PGO's usual caveat about aggressive LTO, but nothing here isolates it -- confirming it needs a comparison at thin or no LTO. Recorded as a hypothesis so it is not later cited as a finding. Not adopted; the ruled-out list gains item 5c. What this does NOT say: PGO is not useless in general, and a different build configuration could change the answer. It says PGO is a regression for THIS configuration on THIS workload, which is all three paired runs can support. Review improved the harness in two ways. The completion witness counted cumulative retired instructions, which include the 36 warm-up frames -- a timed loop that stalled entirely would still have shown ~47M and reported a very fast frame, so the witness would have been witnessing the warm-up. It is now a delta across the timed window (171,471,972). And the scan-out's presence in the timed loop is now stated where the question arises: run_frame is run_until + produce_frame (which calls scanout_scaled) + produce_audio, so presentation is inside every timed frame, while the warm-up's extra call is only the VI-liveness probe. The A-B-A legs are marked as taken in one sitting: a verification run of the same binary an hour later, after heavy building, read 101.6 ms. That is the drift the protocol controls for, and the return legs are why the comparison holds -- but the legs are only comparable to each other. With this, every reachable codegen lever is measured: instruction selection neutral, PGO negative, and the fast-scheduler's 1.054x already merged. Refs #55.
1 participant
The structural-waste search (#227) was one axis. How the compiler emits the code is a separate one, and it was unexamined until now.
Nothing in the workspace sets
target-cpu, so every build targets baselinex86-64while this host offers AVX2 and BMI2. That looks like free headroom.It is not
A-B-A,
frame_cost_probeon Super Mario 64,--release --features fast-scheduler:x86-64)RUSTFLAGS="-C target-cpu=native"The third A leg came in below both B legs.
A two-leg comparison would have reported a ~0.5% win. The return leg shows it was drift — mean A 93.178 against mean B 93.227, 0.9995×, neutral. This is the third time in this project that A-B-A has overturned a two-leg result, after the
#[inline]phantom regression and the release-guard phantom improvement.Unsurprising in hindsight: the hot code is scalar integer interpretation and pointer chasing, not the vectorizable arithmetic AVX2 would help.
The portability cost never had to be argued — a
target-cpu=nativebinary does not run on older hosts, which would be a real price for a measured nothing. Added to the ruled-out list as 5b so it is not retried.PGO is the one codegen lever still untested
Named explicitly, with the warning not to assume it is neutral because this was. The mechanisms do not overlap: PGO drives branch layout and inlining from a real profile rather than instruction selection, and interpreters are the workload it classically helps most.
It needs two full LTO rebuilds plus an instrumented run, and it changes the release process rather than a config line — so it is recorded as an open avenue rather than attempted in passing.
Refs #55.