release: v2.3.1 "Plumb Line" — measurement apparatus, and ten measured rejections - #348
Merged
Conversation
Opens the v2.3.1 measurement release. Every optimization decision in this line depends on knowing where frame time actually goes, and the instrument the project had been profiling was lying about it by a consistent margin. The criterion `full_frame` bench is the right tool for adopt/reject verdicts — it does the statistics properly, and it remains what both CI gates and the PGO promotion gate consume. It is the wrong tool to PROFILE. A `perf record` of the bench binary attributes ~17% of samples to criterion itself: rayon plumbing for its parallel analysis, libm's `exp` from the distribution fitting, and its sorts. That overhead is not noise around the emulator's numbers, it is *mixed into* them — every per-function percentage is diluted by roughly a sixth. `frame_probe` runs the same workload with no criterion in the process image: load a ROM, discard warmup frames, then time steady-state `run_frame()` calls. Measured against the criterion figure it agrees to within 0.2% (nestest median 3.7752 ms here vs 3.7830 ms there), so it is measuring the same thing — but the profile is clean, and the corrected attribution is materially different: function criterion profile frame_probe profile Ppu::tick 24.94% 33.19% LockstepBus::cpu_clock 15.64% 19.07% Ppu::emit_pixel 7.27% 9.46% Cpu::end_cycle 7.20% 9.45% rayon / libm exp / sorts ~17% gone Every v2.3.2 core target is therefore worth about a third more than the numbers recorded during the v2.3.0 P1 campaign suggested. The probe also reports what a bare mean would hide. It prints median, p99, min, and a robust MAD-based coefficient of variation, then states plainly whether the host looked quiet enough to trust — because a figure measured on a contended machine is worse than no figure, since it still looks like data. This is not hypothetical: the v2.3.0 P1 campaign's first profile ran at 39% criterion outliers and its second, on a quiet host, at 2%, from the same binary. Running the probe during this very commit's CI correctly self-reported NOISY at 2.9% CV, with builds and checks competing for the machine. Implementation notes: percentiles use exact integer nearest-rank arithmetic (`rank = ceil(q_num * len / q_den)`) rather than a float `ceil`, so there is no lossy cast in either direction and no `allow` is needed under the workspace's pedantic+nursery lint set; elapsed time goes through `as_secs_f64()` for the same reason. Spread uses median-absolute-deviation rather than standard deviation so a handful of scheduler preemptions cannot dominate the estimate. Tooling only — no core or frontend source is touched, so the deterministic chip stack is byte-identical and AccuracyCoin holds 141/141 by construction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tended host
The same-runner A/B gate (`scripts/bench_relative_check.sh`) rests on one
assumption: that benching base and HEAD back to back on the same machine makes
runner-to-runner variance common-mode, so it cancels in the delta. That holds
only while both runs actually see a comparable machine. Under contention they do
not — whichever run lands next to the noisy neighbour is inflated, and the
reported delta stops measuring the code at all.
This is not a hypothetical failure mode in this repo. The v2.3.0 P1 sprite-eval
change profiled at +2% on a busy host and at -5.13% re-measured quiet. Same
commit, opposite sign. Until now the gate would have published either number
with equal confidence, because it had no way to tell the two situations apart.
It can now, from criterion's own artifacts. For each of the two saved baselines
the gate reads `sample.json` (per-sample iteration counts and elapsed times) and
`tukey.json` (the four fences criterion already computed), and derives:
* robust CV = 1.4826 * MAD / median. This is the trigger. Scaled that way the
MAD is a normal-consistent sigma estimate, and unlike stddev it is not itself
dragged around by the outliers being measured — which is precisely the
property needed on the contended runs the gate must recognize.
* outlier % = the fraction of per-sample averages outside the mild Tukey
fences, i.e. criterion's own "Found N outliers among M measurements"
recovered as a number. Reported as evidence; deliberately not the trigger.
Outlier % is the signal that first suggests itself, and measuring it against the
repo's existing saved baselines showed it to be actively misleading. Criterion's
fences are IQR-derived, so a benchmark whose bulk is unusually tight flags a
large outlier fraction from small absolute excursions:
nes_run_frame_flowing_palette_fast 30.0% outliers 0.19% CV
nes_run_frame_nestest 20.0% outliers 0.58% CV
nes_run_frame_flowing_palette 6.0% outliers 1.18% CV
nes_run_frame_nestest_fast 0.0% outliers 2.79% CV
The two axes do not merely disagree — they invert. The run with the most
outliers is the quietest in the set; the run with none is the noisiest. A gate
keyed on outlier % would have declined a verdict on the best measurement
available and accepted the worst. Hence CV as the trigger and outlier % as
reported context only.
The CV threshold is derived rather than chosen. A gate cannot adjudicate an
effect it cannot resolve, so the host counts as contended once the noise band
(3 * CV) grows wide enough to swallow the regression being tested for — that is,
once 3 * CV exceeds BENCH_MAX_REGRESSION_PCT. At the default 10% limit this is a
3.33% CV, overridable through the new BENCH_MAX_NOISE_CV_PCT knob. Tying the two
together means raising the regression limit relaxes the noise tolerance in step,
with no second constant to keep consistent by hand.
Verdict structure, in the same spirit as the existing "cannot resolve a base
commit" path that already exits 0 rather than inventing an answer:
* quiet host, delta within limit -> PASS (states the host was quiet)
* quiet host, delta over limit -> FAIL
* contended, delta beyond 3x the CV -> FAIL (contention inflates a
measurement; it does not invent a
40% one)
* contended, delta within 3x the CV -> NO VERDICT, exit 0, loudly
The last case is the substance of the change. A clean delta measured on a noisy
host is not evidence that nothing regressed, exactly as a dirty one is not
evidence that something did; emitting either would be manufacturing a conclusion
from data that cannot carry one. It exits 0 because the absolute ceiling in
`bench_regression_check.sh` still applies to every commit, so declining here
never leaves a branch ungated — it withholds a claim rather than withholding
enforcement.
Also recorded, as evidence in the log only: 1-minute load average and CPU count
where /proc/loadavg is readable. Nothing branches on it — it is a lagging figure
and the runner may not be Linux — but a reader diagnosing a NO VERDICT wants to
know what the machine was doing.
Verified by driving all four verdict paths against synthetic baselines built
from the on-disk criterion data, so no bench run is required to exercise the
logic: identical A/B -> PASS; forced-low CV limit -> NO VERDICT; +20% synthetic
regression -> FAIL on a quiet host and FAIL again on a nominally contended one
(20% exceeds 3 x 0.58%); +1.2% on a contended host -> NO VERDICT. `bash -n` and
`shellcheck` clean.
docs/performance.md gains the rationale, the measured outlier-vs-CV inversion
table, and the threshold derivation, so the trap is recorded rather than
rediscovered.
Part of v2.3.1 "Plumb Line" (measurement first). No source change to the
emulator core; AccuracyCoin and nestest are untouched by construction.
…es the APU
`scripts/perf/frame_breakdown.sh` profiles the harness-free `frame_probe` and
buckets samples into PPU / CPU / APU / mappers / bus-coupling, closing the
v2.3.1 "Plumb Line" item that asks for a per-subsystem cost breakdown of the
composed frame rather than the synthetic-bus figures the per-chip criterion
benches produce.
It exists because the obvious command is wrong about this codebase. Under
lto = "fat" + codegen-units = 1 the APU is inlined wholesale into
`<LockstepBus as Bus>::cpu_clock`, so `perf report --no-children -g none`
reports:
31.0% rustynes_ppu::ppu::Ppu::tick
18.3% <rustynes_core::bus::LockstepBus as rustynes_cpu::bus::Bus>::cpu_clock
10.0% rustynes_ppu::ppu::Ppu::emit_pixel
9.0% rustynes_cpu::cpu::Cpu::end_cycle
with no `rustynes_apu::` symbol appearing anywhere, at any percent limit. Read
literally that says the APU is free. Bucketing the same profile by source file
shows it is 18.7% of the frame -- apu.rs 8.4%, frame_counter.rs 2.1%, blip.rs
2.1%, pulse.rs, dmc.rs, noise.rs, mixer.rs, length.rs -- every bit of it hidden
inside that one cpu_clock line.
`perf report --inline` does not recover it. Measured, it produces output
byte-identical to the non-inline report: the inlined APU frames are not
recoverable as call frames at all. Source-file attribution is the only method
tried that works, and it is what the script uses.
The corrected picture, nestest at 1500 frames / 1500 Hz on a quiet host:
PPU (rustynes-ppu) 52.1%
APU (rustynes-apu) 18.7%
CPU (rustynes-cpu) 10.1%
Bus / scheduler coupling 9.9%
std inlined at emulator call sites 6.7%
Mappers 2.5%
------
accounted for 100.0%
This revises the working split the v2.3.x campaign was scoped against
("PPU ~53%, CPU+bus ~39%"). The PPU share holds; the CPU+bus share is really
CPU 10% + APU 19% + coupling 10%, and the CPU proper is about a third of what it
appeared to be. It does NOT reopen the §P4 conclusion: that experiment measured
the one remaining APU lever at a <=1.9% ceiling, and "the APU is large" and "the
APU is reducible" are separate claims -- only the first is established here.
Method and its limits, all recorded in the script header rather than left
implicit:
* Samples are bucketed by source file, so code inlined across a crate boundary
is credited to the crate that wrote it. perf emits basenames only, so the
basename -> subsystem map is built by scanning the tree at run time instead
of being hardcoded, and cannot drift as files are added.
* Four basenames exist in more than one emulation crate. bus.rs and
scheduler.rs are bucketed as coupling regardless of owner -- not a fudge:
each is the bus/scheduler abstraction, so the semantic bucket is identical
whichever crate the samples came from. This was verified rather than
assumed; a joint sym+srcfile view resolves the bus.rs samples to
LockstepBus::raw_cpu_read, Cpu::read1, cpu_clock, and Ppu::tick, i.e. all
three crates' bus files, all of them bus work. lib.rs and snapshot.rs carry
no such invariant and go to an explicit UNATTRIBUTED bucket rather than
being guessed at (both are far below 1% in practice).
* Inlined standard-library code is real emulator work performed at emulator
call sites but carries std's source path, so it gets its own reported line
and is deliberately NOT redistributed proportionally across the buckets --
that would invent precision the data does not contain.
Source attribution needs DWARF, which [profile.release] does not emit, so the
script rebuilds the probe with CARGO_PROFILE_RELEASE_DEBUG=2. Debuginfo does not
change codegen, and rather than assert that, the script prints the probe's own
frame cost so the claim is checkable against a stock release build.
The script skips with exit 0 when perf is absent or perf_event_paranoid > 2, so
it never becomes a hard dependency of any gate; it is a profiling instrument,
not a check. `--keep` retains the perf.data for hotspot.
docs/performance.md gains the measured table, the correction to the campaign's
working figures, and the reasoning above.
Part of v2.3.1 "Plumb Line". No source change to the emulator; AccuracyCoin and
nestest are untouched by construction.
Records the release plan and, per the project convention, the reasoning behind the items NOT taken as well as the ones landed. Done: the harness-free frame probe (f468e76), the source-file per-subsystem breakdown that recovers the APU the symbol profile hides (32fc007), and the contention-aware A/B gate whose first design real data falsified before it shipped (52cedcb). In flight: the BOLT measurement (run 31006334399), to be promoted only on the standing >3% + byte-identical bar and documented either way. Assessed and deliberately not run: the PGO corpus study, because a corpus A/B across two dispatches can only compare each run own PGO-vs-plain ratio, whose noise floor on a shared runner is around a percent or two -- and the measured profile says the corpus already covers the dominant PPU and APU paths, while mappers, where widening adds the most variety, are 2.5% of frame cost. Also cargo-nextest, which does not run doctests this workspace has, so adopting it requires a separate cargo test --doc step in the gate: a maintainer workflow decision rather than a drive-by. Flags for v2.3.2: its item ordering was scoped against the pre-correction "PPU ~53%, CPU+bus ~39%" figures and should be re-read against the measured 52 / 19 / 10 / 10 / 7 / 2.5 split before work starts.
Grain was scoped from the symbol profile, in which the APU is invisible. Now that source attribution exists (32fc007), every item has a measured ceiling instead of a call count -- call counts say how often code runs, only the profile says whether that costs anything. The consequential finding: cpu_clock is 86% inlined APU. Its 18.3% symbol time is apu.rs 6.19 + frame_counter 2.38 + blip 2.14 + pulse 2.01 + length 1.10 + noise 0.93 + mixer 0.74 + triangle 0.34 = 15.83%, against 1.79% of actual bus.rs code (Cpu::end_cycle is the same story: 2.53% of its 9.02% is apu.rs). Item 1 was ranked "highest expected value" precisely because cpu_clock looked like ~16% of bus code. Its premise is factually true -- 0 inline hints across 5,349 lines -- but run_ppu_to, apu_advance_one, and PpuBusAdapter emit no symbols at all, meaning LTO already inlines them, so the hints have less to do than assumed. Two items are dropped outright on evidence rather than deferred: item 3 (capability-gate bg_split_state) targets a symbol measured at 0.09% of frame and would need to beat the adoption bar by 30x, and item 4 (hoist PpuBusAdapter out of the per-dot loop) targets a construction that leaves no symbol behind because it is already optimized away. Promoted: item 9 (fast-dot coverage; Ppu::tick is 27.7% and its prologue line alone 1.84%), item 5 (the v2.3.0 P1 shape; tick_oam_bus 5.53%), item 7 (field layout -- cheap and byte-identical by construction, against a 51.7% ppu.rs). Three gaps the correction exposes, none of them in the original ten: the APU is 18.7% of frame with zero items against it (which does NOT contradict P4 -- that measured mixed-sample caching at a <=1.9% ceiling, not the per-cycle channel tick path); range.rs costs 1.52% inside Ppu::tick, a bigger single line item than four of the ten; and ppudata_sm_countdown (0.81%) is a per-dot decrement with exactly the shape item 6 targets for open-bus decay, so one deadline rewrite would serve both. Every figure is a ceiling, not a prediction, and several items cannot clear the >3% bar alone -- flagged to be bundled into one measured A/B rather than run as ten separate experiments.
… default-OFF
v2.3.2 "Grain" item 9(b). The campaign predicted the default-OFF
ppu-idle-line-fast path (P2: max -1.55%, below the bar) "becomes worthwhile if
per-dot dispatch gets cheaper", and v2.3.0 P1 delivered exactly that (-5.13%).
Re-measured on that basis; it still does not clear the bar.
Criterion change analysis, CPU-pinned (taskset -c 2-5), 2 s warm-up / 10 s
measurement, feature-OFF baseline vs feature-ON:
nes_run_frame_nestest -0.94% (p = 0.00) small win
nes_run_frame_flowing_palette +0.98% (p = 0.02) small REGRESSION
nes_run_frame_nestest_fast -0.36% (p = 0.29) no change
nes_run_frame_flowing_palette_fast +0.84% (p = 0.06) no change
Nothing approaches >3%, the two workloads disagree in sign, and decisively both
_fast variants report no change -- those being the shipped configuration since
fast_dotloop became the default in v2.2.3. The feature stays implemented and
default-OFF on exactly the terms P2 set.
The re-measurement disagrees in SIGN with P2 on flowing_palette (-1.31% then,
+0.98% now). Neither is wrong so much as both sit inside the noise for an effect
this size; the finding consistent across two independent sessions is that the
path moves the shipped configuration by under +-1.5% with an unstable sign,
which is what failing the bar looks like in practice.
Also records a method correction that cost a wrong intermediate read. The first
pass adjudicated from point-estimate ratios plus the v2.3.1 contention heuristic
(contended when 3 x robustCV exceeds the effect under test). That heuristic is
right for the CI regression gate, where the question is whether a single delta
could be noise. It is the wrong statistic for an adoption decision taken from
100-sample means, where the confidence interval governs and the standard error
falls as CV/sqrt(n) -- about 0.2% here, not the 2-3% raw CV. Applied to adoption
it would have demanded a quiet host no desktop provides and refused every
verdict in the campaign. Adoption decisions are adjudicated by criterion
--baseline change analysis, as P2/P3/P4 already did; the v2.3.1 gate keeps its
3xCV rule for the job it was built for.
No source change: the feature was already implemented and gated. AccuracyCoin
141/141 and nestest 0-diff untouched by construction.
…3% bar
The v2.3.2 sweep measures every campaign item, including the ones the profile
suggests are dead. Roughly a dozen A/Bs run the same way, so the method is worth
a script rather than a dozen bespoke command lines.
Deliberately a different tool from bench_relative_check.sh, answering a
different question with a different statistic:
bench_relative_check.sh CI gate: "did this commit regress beyond 10%?" One
delta, so point estimates plus the 3x-robust-CV
contention rule are correct there.
ab_check.sh Adoption: "is this worth keeping at the >3% bar?" A
question about the MEAN of ~100 samples, where the
confidence interval governs and the standard error
falls as CV/sqrt(n) -- about 0.2% here, not the 2-3%
raw CV.
Conflating the two produced a wrong intermediate read during G1 (the 3xCV rule
demanded a quiet host no desktop provides and would have refused every verdict
in the campaign). This script therefore defers to criterion --baseline change
analysis -- change interval plus p-value -- which is what P2/P3/P4 and G1 used.
The distinction is documented in the header so the mistake is not repeated.
Compares the working tree against a reference (default HEAD) back to back on one
host sharing one target dir. The reference builds in a throwaway git worktree,
never a git checkout, so uncommitted work survives even if the run dies. A
--features flag applies to the candidate side only, which is the shape a
default-OFF feature flag needs (G1 used exactly that).
Pins to a fixed CPU set via taskset when the host has the cores to spare:
measured here, pinning took robust CV from 2.73% to 1.95%, narrowing every
confidence interval at no cost.
The trailing note states the adoption rule the numbers must satisfy -- negative,
whole interval clearing -3%, p < 0.05, mixed signs being a rejection rather than
something to average -- and flags that the _fast workloads are the SHIPPED
configuration since v2.2.3, so a change that moves only the non-fast variants
moves nothing a user runs.
…exposed v2.3.2 "Grain" item 7. Two findings, the second more valuable than the first. The item asked to reorder Ppu\s 114 fields by access frequency, describing the hot ones as "scattered, with a 2 KiB rgba_lut sitting between the palette state and the framebuffer pointer", and called it "pure reordering". The premise is void: Ppu is repr(Rust), so declaration order does not determine memory layout. Probed offsets show rustc already packs every hot u16/i16 scalar contiguously into ONE cache line (v/dot/scanline/bg_shift_lo/bg_shift_hi/at_shift_lo/ at_shift_hi/flags_cached_scanline at 2570..2586) and places the 2 KiB LUT before that whole cluster -- the opposite of the description. Source reordering cannot move any of it. Measured anyway in the only form that changes layout -- #[repr(C)], which forces declaration order -- plus a variant hoisting the 256-byte oam_decay_cycles (dead unless OAM decay is enabled, default-off) out from between the scroll registers and the per-dot render state: run 1 repr(C) -1.84% .. -2.75%, p = 0.00 on all four run 2 repr(C) + cold field last no change on 3 of 4 (p >= 0.31) run 3 repr(C) again no change on all four (p >= 0.11) Run 1 was wrong. The identical candidate that produced a textbook -2% at p=0.00 on every workload produced nothing on re-measurement, with no code change between them. Chasing that number would have meant reordering 114 fields, and briefly it looked good enough to ask whether the >3% bar should be relaxed. Root cause is a systematic bias in ab_check.sh itself: the reference was always benched FIRST and the candidate SECOND, so anything making the host monotonically faster across a run -- page-cache warming, governor ramping, a background job finishing, boost/thermal settling -- is indistinguishable from "the candidate is faster". Run 1 followed heavy local activity (test runs, perf record, worktree builds); the machine was still settling during the reference and had settled by the candidate. Fixed with an A/B/A order-bias control: the reference is now re-benched a third time, LAST, against its own first run. Whatever that reports is pure position-in-the-run drift, and it is the noise floor the candidate must be read against -- printed before the adoption rule so it cannot be skipped. The script now also states that a single run is not a result and that anything under ~5% needs an independent second run, citing this experiment. Item rejected: no reproducible effect from any layout change tried. That is the physically sensible answer too -- Ppu is ~2,856 bytes and stays L1-resident across a frame, so layout has little left to buy. The finding is recorded on the oam_decay_cycles field itself so the next reader does not re-run the experiment. No behaviour change: ppu.rs carries only a documentation note; repr(C) and the field move are both reverted. AccuracyCoin 141/141 and nestest 0-diff untouched by construction.
v2.3.1 "Plumb Line" item 3 dispatched the BOLT measurement (run 31006334399).
The PGO stage cleared its >3% + byte-identical gate, then BOLT died with:
Cannot find llvm-bolt: cannot find binary path
The probe step was:
if command -v llvm-bolt; then have_bolt=true
elif sudo apt-get install -y --no-install-recommends bolt; then have_bolt=true
On Ubuntu the package named `bolt` is the **Thunderbolt 3 device manager** -- an
unrelated project that owns the name in Debian/Ubuntu. apt installed it happily,
exited 0, the probe concluded llvm-bolt was present, and the stage then failed
on the very tool it had just "confirmed". A job whose whole contract is
best-effort -- skip cleanly when the tool is missing -- instead failed the run.
The bug is not the package name; it is inferring a binary exists from a package
manager exit code. The probe now LOCATES THE BINARY and only reports success
when it can name a path:
* checks `llvm-bolt` on PATH, then `llvm-bolt-<N>` and
/usr/lib/llvm-<N>/bin/llvm-bolt for N in 21..16 (LLVM ships both the
unversioned form via apt.llvm.org bolt-<N> and versioned forms);
* symlinks a versioned hit to /usr/local/bin/llvm-bolt, because cargo-pgo
resolves the UNVERSIONED name, and puts that directory on GITHUB_PATH;
* on a miss, attempts several candidate packages and re-probes after EACH
one, since a successful install says nothing about what landed on disk;
* drops `set -e` deliberately, with a comment: this step probes for things
allowed to be absent, and a missing tool must skip the stage rather than
fail the run.
The likely outcome on ubuntu-latest is still a skip -- stock Ubuntu repos may
carry no LLVM BOLT at all -- but a skip is the DESIGNED behaviour and is now
reported honestly, with the found path echoed to the step summary when present.
No emulator code touched. The BOLT verdict for docs/performance.md remains
pending a run that gets far enough to produce a number.
…s it v2.3.2 "Grain" item 5, the campaign highest-ranked CODE item and the same transformation shape as the adopted v2.3.0 P1. Measured, rejected, reverted. Two sites compute values they then discard. tick_sprite_eval_per_dot derives next_line and sprite_height on entry but the match consumes them only in the 65..=256 arm -- dead on 149 of 341 dots. tick_oam_bus derives sprite_height and scan above the cycle < 65 secondary-OAM-clear path that discards both -- dead across a quarter of every visible line, P1 having already moved the cycle == 0 return above them. Both were sunk to their single point of use, in the sprite-eval case inside the !sprite_eval_done guard, tighter than the arm. Correctness was established before measuring: AccuracyCoin 100.00% over 141 assigned tests, visual_regression 9/9 (golden framebuffers, the direct byte-identity evidence), full --features test-roms suite green, clippy clean. The "framebuffer/RAM disagree by 21 cells" line the run prints was checked against a clean HEAD worktree and is byte-for-byte pre-existing. Two independent A/B runs. Run 1 showed nestest -0.56% at p = 0.00, which reads as a small genuine win. It is not, and the new A/B/A order-bias control proves it directly rather than by argument: run 2 control -- the reference benched against itself, no code difference at all -- reported nestest -0.59% at p = 0.00. The drift and the "effect" are the same size, same workload, same significance. Run 1 control had already flagged -0.39% (p = 0.03) on nestest_fast. Both shipped _fast variants are flat across both runs (p >= 0.48, intervals straddling zero). The generalizable finding: LLVM already sinks pure side-effect-free computations past branches that do not use them. At opt-level 3 with fat LTO, writing the sink by hand tells codegen nothing it had not already derived; the source change only made explicit what the optimizer was doing anyway. That reframes v2.3.0 P1, which bundled an #[inline] with a hoist of exactly this shape and measured -5.13% without separating them. G3 is evidence the hoist half contributes ~nothing, pointing at the #[inline] -- a change to the INLINER COST MODEL, which LLVM cannot infer -- as the real source of that win. Recorded as a hypothesis, not a conclusion: it was not re-measured in isolation. Reverted to the original code (the diff is comments only). Both sites keep a note so the attempt is not repeated. Also lowers the prior for items 2, 6 and 8, which are the same "stop computing something dead" shape -- they will still be measured, since todays predictions have been wrong in both directions.
v2.3.2 "Grain" items 8, 6 and 2, measured by CEILING PROBE: rather than engineer each optimization and then discover it was worthless, delete the work outright -- knowingly breaking correctness -- and measure the upper bound any real implementation could reach. Where the ceiling is zero the engineering is moot and no correctness hazard is ever introduced. Three multi-hour items became three benchmark runs. G4 (item 8) index_framebuffer store in emit_pixel 61,440 stores/frame zero G5 (item 6) open-bus decay loop in on_cpu_cycle ~29,780 calls/frame zero G6 (item 2) ALE/read fetch-address recomputation ~30,720 recomputes zero In every case the shipped _fast workloads were flat and the apparent movement on nestest was matched or exceeded by the run own A/B/A control: G4 candidate -0.82% (p=0.01) control -0.88% (p=0.01) G5 candidate -0.49% (p=0.06) control -0.51% (p=0.05) G6 run 1 candidate -0.89% (p=0.00) control -0.16% (p=0.37) G6 run 2 candidate -0.96% (p=0.00) control -1.17% (p=0.00) G6 is the instructive one and nearly became a false adoption. Run 1 showed -0.51% at p=0.00 on nestest_fast -- a SHIPPED configuration, with a clean control on that workload. Under the relaxed sub-3% bar that is an adopt. Run 2 measured the same probe at +0.01% (p=0.96), with a nestest control drifting -1.17%, larger than the candidate own -0.96%. The mandatory second run is the only thing that caught it. Also recorded: nestest is the FIRST workload criterion benches, absorbs the most warm-up, and is where drift appears most consistently across this whole campaign -- treat a nestest-only result with suspicion. Three mechanisms, one conclusion. G4: a line profile share is not its marginal cost -- perf charges ~0.78% to that store, but it is a sequential u16 write the store buffer absorbs off the critical path, so deleting it frees nothing and the samples redistribute onto neighbours. G5: ~29,780 calls/frame is three perfectly predicted compare-and-decrement steps on L1-resident data, hidden entirely under other latency. G6: the recomputation is real but equally off the critical path. G6 was additionally NOT adoptable at any speed, which the ceiling makes moot but is worth recording. The read half re-derives the address for observe_a12_addr; ale_splice takes the read address high bits from address_bus (latched at the ALE dot) and its low bits from octal_latch, so the recomputed value exists specifically to drive A12. On hardware only A7-A0 pass through the 74LS373, so the PPU drives the current full address during the read cycle and A12 follows it. Caching freezes A12 to the ALE dot and shifts MMC3 IRQ timing whenever a $2000/$2005/$2006 write lands between the two dots. The plan item saw two identical-looking expressions and inferred redundancy; they are identical only in the common case and are MEANT to be able to differ. All probes reverted -- the diff is comments only. Verified after revert: AccuracyCoin 100.00% over 141 assigned tests, clippy clean at -D warnings.
v2.3.2 "Grain" items 1, 10, 3 and 4, closing the campaign at ten measured and ten rejected. That result is the release finding, not a failure to find one. G7 (item 1) -- #[inline] on bus.rs. The plan called this "the highest expected value in the plan" because bus.rs carries zero #[inline] hints across 5,349 lines. True, but only THREE of its functions survive codegen as symbols: cpu_clock (18.32%), raw_cpu_read (2.45%), apply_genie (0.12%). The specifically-named run_ppu_to, apu_advance_one and the twelve PpuBusAdapter forwarders emit no symbol at all -- fat LTO already inlines every one. Hinting the two that genuinely are not inlined, measured separately as opposite bets: both together gave nestest +0.60% (p=0.02) against a CLEAN control (-0.10%, p=0.72), a real regression, because cpu_clock contains the whole inlined APU and duplicating it at every call site costs more in I-cache than the call saved -- the mechanism that made v2.2.3 P3 slower. raw_cpu_read alone gave -0.98% (p=0.00) against a -0.76% (p=0.01) control, i.e. drift. This weakens without disproving G3 hypothesis that v2.3.0 P1 -5.13% came from its #[inline]: P1 hint was on a small per-dot PPU function, structurally unlike either of these, so the hypothesis is untested rather than refuted -- but two attempts to find an inline-hint win have now failed and it must not be repeated as established. G8 (item 10) -- oam/ciram as fixed arrays. Both are Box<[u8]> indexed with & 0xFF / & 0x07FF, so the bounds check is provably dead but the type does not say so; [u8; 0x100] / [u8; 0x800] encode the length statically and elide it with no unsafe. Four-line swap; surrounding code coerces arrays to slices. nestest -0.61% (p=0.05) against a -0.78% (p=0.01) control, everything else flat. The checks really were removed; removing them bought nothing. Matches P3. G9 (item 3) -- capability-gate bg_split_state. Ceiling probe skipped the per-fetch mapper dispatch outright. Three workloads flat; flowing_palette_fast +0.54% (p=0.03) against a +0.81% (p=0.00) control on that same workload. Ceiling zero, consistent with the 0.09% the symbol carries. G10 (item 4) -- hoist PpuBusAdapter out of the dot loop. NOT IMPLEMENTABLE under this campaign constraints, and pointless if it were. The plan reads the per-dot construction as an oversight defeating vtable hoisting; it is forced. The adapter holds mapper: self.mapper.as_mut() and self.sample_nmi_edge() runs in the same loop taking &mut self, so hoisting would hold a mutable borrow of self.mapper across a call needing all of self. With no unsafe in the chip stack it cannot be done without restructuring sample_nmi_edge onto disjoint fields -- and no PpuBusAdapter symbol survives codegen anyway. Ten rejections via SIX distinct mechanisms, which is what makes this a finding rather than one bad assumption repeated: LLVM already does it (G3); the premise is false (G2, G7); the work is real but absorbed off the critical path (G4, G5, G6); the elision is real but buys nothing (G8); the target is too small (G9); the ownership model forbids it (G10). The per-dot loop has no incidental overhead left to reclaim -- its ~3.78 ms is work the accuracy model requires, and the core is issue-limited on that rather than on bookkeeping. This corroborates the existing record: P3 bounds-check elision measured slower, the v2.1.8 SIMD blitter measured slower, the P4 mixer lever capped at <=1.9%. Two methodological results outlast the items. The A/B/A order-bias control (added in G2) fired on nearly every subsequent run and is the only reason G6 was not adopted on a -0.51% (p=0.00) reading of a SHIPPED configuration that re-measured at +0.01% (p=0.96). And ceiling probes -- delete the work, knowingly breaking correctness, measure the bound before building anything -- settled G4, G5, G6 and G9 in one run each; G4 alone would otherwise have meant threading an opt-in flag through four consumers for a zero gain. Remaining levers are structural, not micro-architectural: v2.3.3 frontend copy chain (three full 720 KiB memcpys per displayed frame) and snapshot slimming (~250 KB per run-ahead frame) are whole-buffer costs. All probes reverted; the tree is byte-identical to HEAD outside documentation. Verified: workspace clippy clean at -D warnings, AccuracyCoin 100.00% over 141 assigned tests.
…frontend work
Maintainer decision. The core hot-path campaign ran to completion, measured ten
items and rejected all ten, and therefore had no shippable content of its own. Its
results are folded into v2.3.1 "Plumb Line" -- they are the answer the measurement
apparatus existed to produce, and shipping the tooling without the findings it
generated would split one piece of work across two releases for no reason.
The "Grain" name moves to the frontend / coupling / display work (formerly
"Conduit II"), where the campaign own evidence says the remaining wins are. The
line is now three releases, not four:
v2.3.1 "Plumb Line" measurement apparatus + the ten negative core results
v2.3.2 "Grain" frontend, coupling, display (was "Conduit II")
v2.3.3 "Lucid" the three novel features (was v2.3.4)
Mechanical: the ten experiment labels are re-attributed v2.3.2 G1-G10 ->
v2.3.1 G1-G10 across docs/performance.md, ppu.rs and ab_check.sh (the in-source
"do not re-attempt" notes cite them, so the labels have to stay resolvable), and
the adoption-rule attribution follows.
Substantive: the plan doc previously carried a forward-looking re-ranking of the
Grain items. That is replaced by the predicted-vs-measured table, which is the
more useful artifact -- the two promoted items were rejected, and the two items
downgraded on profile evidence (3, 4) were measured anyway at the maintainer
instruction and both confirmed. The gap between the ranking and the outcome is
the result, so the ranking history is kept rather than deleted.
Also closed out in the plan doc: the ppudata_sm_countdown lead is closed by G5
(the open-bus decay it mirrors has a ceiling of zero, so the same rewrite on the
same shape would too), leaving the APU (18.7%) and range.rs inside Ppu::tick
(1.52%) as the only unmeasured core leads -- both to be ceiling-probed before any
implementation. The three practices this campaign added (A/B/A order-bias
control, ceiling probes, mandatory second run) are recorded as the durable
outcome, each with the specific near-miss that earned it.
BOLT remains genuinely outstanding: run 31006334399 failed before producing a
number, and the probe fix is committed but unexercised.
No code changes; AccuracyCoin 141/141 and nestest 0-diff unaffected.
Scoped to this branch work only: the measurement apparatus, the ten-item hot-path campaign that produced ten rejections, and the first BOLT probe fix. The frontend hygiene items and the p99 gate work live on the Grain branch and get their own entry there. Leads with "no emulation-core changes" and names the six mechanisms behind the rejections, because a changelog that silently omitted a release worth of negative results would misrepresent what happened -- and the mechanisms are the transferable part. Calls out what each new tool actually found rather than just listing it: the frame probe removing criterion ~17% profile contamination, the per-subsystem breakdown recovering the APU at 18.7% of frame (invisible under perf report because fat LTO inlines it into cpu_clock), and the A/B/A order-bias control that is the only reason two near-misses were not adopted on a single reading.
There was a problem hiding this comment.
Pull request overview
This PR opens the v2.3.x performance line by adding repeatable measurement tooling (harness-free frame probing, subsystem cost attribution, and improved A/B gating), and documenting a completed hot-path campaign where ten candidates were measured and rejected—without changing emulation-core behavior.
Changes:
- Add new perf tooling (
frame_probe,frame_breakdown.sh,ab_check.sh) to measure steady-state frame cost and attribute time by subsystem even under fat-LTO inlining. - Improve the CI relative frame-time gate to detect/handle host contention by refusing to emit a verdict when noise is too high.
- Document the methodology + results in
docs/performance.md, and update release planning + changelog; harden the PGO BOLT probe to locatellvm-boltrather than trusting package names.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| VERSION-PLAN.md | Updates forward-path release plan to reflect the v2.3.x line and renames/scopes upcoming releases. |
| to-dos/plans/v2.3.1-plumb-line-plan.md | New detailed plan documenting the measurement-first approach and the ten measured rejections. |
| scripts/perf/frame_breakdown.sh | New perf-based subsystem attribution script (by source file) to recover inlined APU cost. |
| scripts/perf/ab_check.sh | New adoption-oriented A/B/A benchmarking helper using criterion’s baseline analysis plus order-bias control. |
| scripts/bench_relative_check.sh | Enhances CI perf regression gate with contention/noise detection using robust CV + criterion artifacts. |
| docs/performance.md | Records the new tooling, noise/host-contention rationale, and the G1–G10 experiment results and mechanisms. |
| crates/rustynes-test-harness/src/bin/frame_probe.rs | Adds a harness-free steady-state frame-cost probe intended for profiling without criterion overhead. |
| crates/rustynes-test-harness/Cargo.toml | Registers the new frame_probe binary with supporting documentation. |
| crates/rustynes-ppu/src/ppu.rs | Adds “do not re-attempt” notes tied to measured performance rejections for specific micro-optimizations. |
| CHANGELOG.md | Summarizes the measurement tooling additions, the “ten measured rejections,” and the BOLT probe fix. |
| .github/workflows/pgo.yml | Makes the BOLT probe locate an actual llvm-bolt binary and skip honestly when unavailable. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Warning Review limit reached
Next review available in: 44 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds frame timing, subsystem profiling, A/B benchmarking, and noise-aware benchmark verdict tools. It corrects BOLT detection in the PGO workflow and records rejected optimizations, measurement methods, release results, and the v2.3.x plan. ChangesPerformance measurement campaign
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant frame_breakdown
participant frame_probe
participant perf
participant attribution
frame_breakdown->>frame_probe: build with DWARF and run frames
frame_breakdown->>perf: sample frame execution
perf-->>frame_breakdown: return source samples
frame_breakdown->>attribution: map sources to subsystems
attribution-->>frame_breakdown: return aggregated costs
Suggested labels: 🚥 Pre-merge checks | ✅ 9✅ Passed checks (9 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 @.github/workflows/pgo.yml:
- Around line 294-304: Update the llvm-bolt discovery logic in the workflow’s
probe and installation loops to enumerate executable candidates matching
/usr/bin/llvm-bolt-* and /usr/lib/llvm-*/bin/llvm-bolt, rather than restricting
checks and package retries to fixed LLVM versions. Preserve the existing symlink
creation and have_bolt behavior once a candidate is found, while allowing both
newer and older installed versions to be detected.
- Around line 297-302: Update both llvm-bolt branches in find_bolt to verify
that sudo ln -sf succeeds before echoing /usr/local/bin and returning success.
If either symlink creation fails, do not report the directory or mark BOLT as
available; preserve probing so the workflow fails closed.
In `@CHANGELOG.md`:
- Around line 17-18: Finalize the v2.3.1 “Plumb Line” release metadata: in
CHANGELOG.md at lines 17-18, move the release notes into the versioned v2.3.1
section and leave [Unreleased] empty; in VERSION-PLAN.md at line 81, set v2.3.1
as the current release and v2.3.2 as the next planned release; in
to-dos/plans/v2.3.1-plumb-line-plan.md at lines 1-3, mark the plan complete or
clearly label it as historical planning material.
In `@crates/rustynes-test-harness/src/bin/frame_probe.rs`:
- Around line 167-168: Update the argument parsing in the frame_probe main flow
for “--frames” and “--warmup” so missing or malformed values produce a usage
error instead of falling back to defaults. Require “--frames” to parse as a
positive integer, rejecting zero, and preserve the existing defaults only when
the options are not provided.
In `@scripts/bench_relative_check.sh`:
- Around line 300-319: Update the contended branch in
scripts/bench_relative_check.sh to return the existing rc status before exiting
the no-verdict path. Emit the NO VERDICT message and exit successfully only when
rc indicates no prior hard failure, preserving failures recorded by the
benchmark checks.
In `@scripts/perf/ab_check.sh`:
- Around line 51-55: Restore the mandatory 3% adoption threshold in
scripts/perf/ab_check.sh at lines 51-55 by stating that evidence quality does
not replace the project’s 3% bar. Update the adoption conditions at lines
187-204 to require the optimization to be faster by more than 3%, and remove any
exception allowing adoption below 3%; both sites in the same file require
changes.
In `@scripts/perf/frame_breakdown.sh`:
- Around line 116-126: Update the frame_breakdown build flow around the
frame_probe commands to build and preserve a stock release probe before building
the CARGO_PROFILE_RELEASE_DEBUG=2 variant. Run both preserved binaries with
identical ROM and frame inputs, report their explicit same-runner cost
comparison, then continue with sampled attribution using the debuginfo probe.
In `@to-dos/plans/v2.3.1-plumb-line-plan.md`:
- Around line 235-246: Update the campaign table to identify it explicitly as a
pre-campaign recommendation table, or replace its entries with the final
rejected outcomes so its status labels agree with the outcome table. Correct the
index_framebuffer measurement from 61,440 B/frame to 61,440 entries or 122,880
B/frame, matching the Box<[u16]> implementation.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6735bc87-d8bf-4ad2-8c09-9b769ad8f6f9
📒 Files selected for processing (11)
.github/workflows/pgo.ymlCHANGELOG.mdVERSION-PLAN.mdcrates/rustynes-ppu/src/ppu.rscrates/rustynes-test-harness/Cargo.tomlcrates/rustynes-test-harness/src/bin/frame_probe.rsdocs/performance.mdscripts/bench_relative_check.shscripts/perf/ab_check.shscripts/perf/frame_breakdown.shto-dos/plans/v2.3.1-plumb-line-plan.md
Run 31067782333 is the first time BOLT ever worked end to end: the probe and
runtime-library fixes landed, `cargo pgo bolt build` instrumented the binary and
`cargo pgo bolt optimize` produced an optimized one, both succeeding for the
first time. That success is what exposed the two steps after them.
`cargo pgo bolt optimize` accepts NO cargo subcommand -- its usage is
`cargo pgo bolt optimize [OPTIONS] [-- <CARGO_ARGS>...]` -- unlike `cargo pgo
optimize` on the PGO side, which does take `bench`/`test`. Both BOLT steps were
written by analogy with the PGO stage, and both are rejected:
error: unexpected argument bench found (bench + gate step)
error: unexpected argument test found (determinism oracle step)
The determinism step failed loudly, which is how this was noticed. The bench step
did not: it swallowed the error with `|| cargo bench ...`, fell back to a PLAIN
non-BOLT build, computed a speedup against the plain baseline, and wrote it to
the job summary as "BOLT speedup vs plain release". It compared plain against
plain and REPORTED SUCCESS. Had that ratio landed above the 3% bar, the gate
would have promoted a BOLT binary on a measurement containing no BOLT.
Correcting the CLI would not fix it. BOLT optimizes the `rustynes` FRONTEND
binary, while the gate benches rustynes-core `full_frame` criterion bench -- a
separate binary BOLT never touched. Even spelled correctly, the step would
measure something unrelated to its subject. Measuring BOLT honestly needs a
harness running inside the optimized artifact (frame_probe built as part of it),
which is a design change, not a one-line fix.
Both steps are therefore DISABLED rather than patched, with the full reasoning
inline. A gate that cannot measure its subject is worse than no gate, and this
one could actively mislead. The instrument/optimize steps still run and still
prove BOLT works end to end, and the artifact is still uploaded; only the two
claims that were not true are withdrawn.
BOLT remains UNMEASURED. That is now an honest "not measured" rather than a
number that meant nothing.
Addresses the review on #348. Seven findings were valid and are fixed; three were false positives and are declined with evidence in the threads. RELEASE METADATA (CodeRabbit, major -- the substantive one). The PR was titled `release: v2.3.1` while every metadata surface still said unreleased/in-progress, and Cargo.toml still read 2.3.0 -- so release-auto.yml, which derives the tag from the workspace version, would never have produced a v2.3.1 tag. Checked against how v2.3.0 was cut (PR #347 bumped Cargo.toml in the same PR alongside CHANGELOG/README/STATUS) and matched that ceremony: workspace version 2.3.0 -> 2.3.1 (inherited by all 17 crates, Cargo.lock refreshed), the CHANGELOG [Unreleased] block cut to [2.3.1] - 2026-08-06 - "Plumb Line", VERSION-PLAN header + release table + forward path updated, docs/STATUS.md current-release banner rewritten, the plan doc marked COMPLETE, and .github/release-notes/ v2.3.1.md written for release-auto to consume. SCRIPT + CODE FIXES. * bench_relative_check.sh interpolated ${MAX_REGRESSION_PCT} into inline Python source to derive the CV threshold, so a non-numeric BENCH_MAX_REGRESSION_PCT would break parsing or execute as code. Replaced with awk, which treats the value as data; verified that garbage input now yields the 3.33 fallback rather than executing. * frame_probe accepted `--frames 0`, which produced an empty sample set and then reported a 0.00% CV ("host: QUIET"), a 0 ms median and an infinite realtime multiplier -- a confident-looking measurement of nothing, which is precisely what this probe exists to prevent. Both count flags now reject missing/non-numeric values (and --frames rejects zero) with exit code 2. Verified all four cases. * frame_breakdown.sh claimed the script "asserts" that debuginfo does not perturb the measurement by comparing against a stock build. It never builds a stock probe, so it asserts nothing. Reworded to state the figure is context, and to say how a reader can check the claim themselves. * The BOLT probe hard-coded an llvm-bolt-16..21 window, so any other version reported have_bolt=false -- indistinguishable from "not installed", the exact failure this probe exists to eliminate. Now enumerates /usr/bin/llvm-bolt-*, /usr/local/bin/llvm-bolt-* and /usr/lib/llvm-*/bin/llvm-bolt, with the install candidate list widened too. * find_bolt returned success even when its `ln -sf` failed, echoing a directory for a symlink that does not exist. Now propagates the failure and re-checks executability. DOC FIXES. Removed a paragraph duplicated verbatim in the plan doc; relabelled the campaign ranking table as a PRE-CAMPAIGN recommendation (its "promote to first" column is the prediction, and the outcome table above it records that all ten were rejected); corrected 61,440 B/frame to 61,440 u16 entries (122,880 B), index_framebuffer being Box<[u16]>. Verified: workspace clippy clean at -D warnings, cargo fmt clean, YAML and shellcheck clean, cargo check --workspace green on the bumped version.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
.github/workflows/pgo.yml (1)
358-365: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUpload the BOLT artifact instead of the PGO binary.
BOLT instrument + optimizerunscargo pgo bolt buildandcargo pgo bolt optimize, which createrustynes-bolt-instrumentedandrustynes-bolt-optimized. The upload path only usesactions/upload-artifactwithpath: target/**/release/rustynes, sorustynes-pgo-boltuploads the PGO binary fromscripts/pgo/run.sh, not a BOLT output. Select the intended artifact explicitly, or remove the false BOLT artifact if BOLT is not the released binary.🤖 Prompt for 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. In @.github/workflows/pgo.yml around lines 358 - 365, Update the BOLT artifact upload configuration associated with the “BOLT instrument + optimize” step to explicitly package the intended rustynes-bolt-optimized output (or remove the artifact if it is not released), rather than the generic target/**/release/rustynes PGO binary produced by scripts/pgo/run.sh. Keep the existing BOLT build and optimize commands unchanged.crates/rustynes-test-harness/src/bin/frame_probe.rs (1)
201-205: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject malformed options instead of running a different measurement.
When
--romhas no value, this parser silently falls back to the default corpus. The unknown-argument branch also ignores typos such as--frmaes 400. Exit with a usage error for missing--romvalues and unknown options.Suggested parser change
"--rom" => { - if let Some(p) = args.next() { - roms.push(PathBuf::from(p)); - } + let Some(p) = args.next() else { + eprintln!("frame_probe: --rom requires a value"); + std::process::exit(2); + }; + roms.push(PathBuf::from(p)); } - other => eprintln!("frame_probe: ignoring unknown argument {other:?}"), + other => { + eprintln!("frame_probe: unknown argument {other:?}"); + std::process::exit(2); + }Also applies to: 215-222
🤖 Prompt for 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. In `@crates/rustynes-test-harness/src/bin/frame_probe.rs` around lines 201 - 205, Update the argument parser handling around the "--rom" branch and the unknown-argument branch to return a usage error when "--rom" is missing its value, and when any unrecognized option such as "--frmaes" is encountered. Preserve normal parsing for valid options and ROM paths, but do not fall back to the default corpus after malformed input.scripts/perf/frame_breakdown.sh (3)
140-150: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not classify every unmapped source as
STD-INLINED.The source map omits
crates/rustynes-test-harness/src, includingframe_probe.rs, and it does not include dependency source trees. For any such source file,owners.get(fname)is empty andbucket_forreturnsSTD-INLINED. The report then labels that work as standard-library code. Use an explicit harness/dependency bucket, or classify unknown files asUNATTRIBUTEDunless their source path proves they are standard-library files.Also applies to: 180-186, 204-237
🤖 Prompt for 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. In `@scripts/perf/frame_breakdown.sh` around lines 140 - 150, The source-map fallback currently mislabels unmapped harness and dependency files as STD-INLINED. Update bucket_for and the related report paths around owners.get(fname) so unknown files use UNATTRIBUTED, while assigning explicit buckets only when the source path proves harness, dependency, or standard-library ownership; preserve existing crate mappings.
88-96: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle missing option values before
shift 2.
--rom,--frames, and--freqread$2whileset -uis active, soframe_breakdown.sh --freqexits with an unbound-variable error instead of a controlled usage error. Check that the argument exists before shifting, and validateFRAMESandFREQas positive integers.🤖 Prompt for 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. In `@scripts/perf/frame_breakdown.sh` around lines 88 - 96, Update the argument parsing case block in frame_breakdown.sh to verify that --rom, --frames, and --freq each have a following value before reading $2 or shifting two positions, emitting the existing controlled usage error for missing values. Also validate the assigned FRAMES and FREQ values as positive integers before continuing.
153-159: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve
perf reportfailures.The
|| truemasks the pipeline status, so a failedperf reportcan make{ work}/by_file.txtempty and triggerSKIP: perf produced no source-attributed sampleswith exit code 0. Runperf reportseparately and only ignoregrep’s no-match exit when samples exist.🤖 Prompt for 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. In `@scripts/perf/frame_breakdown.sh` around lines 153 - 159, Update the perf report pipeline in the frame breakdown script to preserve failures from perf report instead of masking them with “|| true”. Run perf report separately, capture its output/status, and only tolerate grep’s no-match status when report generation succeeded; retain the existing skip behavior for successfully generated output with no source-attributed samples.
🤖 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 @.github/release-notes/v2.3.1.md:
- Around line 1-3: Update the v2.3.1 release-note conclusion to limit its scope
to the ten measured hot-path candidates, stating that no shippable improvement
was found among them. Avoid claiming the entire emulation core has nothing left
to find, since the APU path and range.rs within Ppu::tick remain unmeasured.
In `@scripts/bench_relative_check.sh`:
- Around line 99-103: Validate the non-empty BENCH_MAX_NOISE_CV_PCT override
before assigning MAX_NOISE_CV_PCT, rejecting malformed values such as “3oops”
and allowing only a valid numeric limit. Preserve the existing default
calculation through awk when the override is unset, and ensure invalid input
fails before it reaches the Python verifier.
In `@scripts/perf/frame_breakdown.sh`:
- Around line 59-66: The frame breakdown script’s comments incorrectly imply
validation against a stock release build. Update the comments around the
debuginfo probe invocation to describe its output as contextual frame-cost
information only, or implement a same-runner A/B comparison using byte-identical
inputs and the project’s required >3% evidence; do not leave the current
unsubstantiated validation claim.
---
Outside diff comments:
In @.github/workflows/pgo.yml:
- Around line 358-365: Update the BOLT artifact upload configuration associated
with the “BOLT instrument + optimize” step to explicitly package the intended
rustynes-bolt-optimized output (or remove the artifact if it is not released),
rather than the generic target/**/release/rustynes PGO binary produced by
scripts/pgo/run.sh. Keep the existing BOLT build and optimize commands
unchanged.
In `@crates/rustynes-test-harness/src/bin/frame_probe.rs`:
- Around line 201-205: Update the argument parser handling around the "--rom"
branch and the unknown-argument branch to return a usage error when "--rom" is
missing its value, and when any unrecognized option such as "--frmaes" is
encountered. Preserve normal parsing for valid options and ROM paths, but do not
fall back to the default corpus after malformed input.
In `@scripts/perf/frame_breakdown.sh`:
- Around line 140-150: The source-map fallback currently mislabels unmapped
harness and dependency files as STD-INLINED. Update bucket_for and the related
report paths around owners.get(fname) so unknown files use UNATTRIBUTED, while
assigning explicit buckets only when the source path proves harness, dependency,
or standard-library ownership; preserve existing crate mappings.
- Around line 88-96: Update the argument parsing case block in
frame_breakdown.sh to verify that --rom, --frames, and --freq each have a
following value before reading $2 or shifting two positions, emitting the
existing controlled usage error for missing values. Also validate the assigned
FRAMES and FREQ values as positive integers before continuing.
- Around line 153-159: Update the perf report pipeline in the frame breakdown
script to preserve failures from perf report instead of masking them with “||
true”. Run perf report separately, capture its output/status, and only tolerate
grep’s no-match status when report generation succeeded; retain the existing
skip behavior for successfully generated output with no source-attributed
samples.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a78c6950-bc3b-4fa5-b77c-a7ce6bd1bffb
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!Cargo.lock
📒 Files selected for processing (10)
.github/release-notes/v2.3.1.md.github/workflows/pgo.ymlCHANGELOG.mdCargo.tomlVERSION-PLAN.mdcrates/rustynes-test-harness/src/bin/frame_probe.rsdocs/STATUS.mdscripts/bench_relative_check.shscripts/perf/frame_breakdown.shto-dos/plans/v2.3.1-plumb-line-plan.md
…robe root Addresses the Antigravity review on #348 (no blocking issues; these are its two actionable suggestions). find_bolt() mutated host state -- `ln -sf` into /usr/local/bin -- as a side effect of LOOKING for the tool, so a probe that should be idempotent and re-runnable modified system paths mid-discovery, and did so before its own executability checks had passed. Split into find_bolt_bin() (pure: echoes a path or nothing, touches nothing) and link_bolt() (explicit: creates the unversioned symlink cargo-pgo resolves, reporting failure rather than assuming success). find_bolt() now composes the two, so discovery can be re-run freely and the one mutating step is named as such. The already-unversioned case skips linking entirely rather than symlinking a path onto itself. frame_probe workspace_root() resolves the default ROM corpus from the COMPILE-TIME CARGO_MANIFEST_DIR, so a relocated binary cannot find them. That is not silent -- the corpus loop reports each missing ROM by path and exits non-zero with "no ROMs measured" -- but it was undocumented. Recorded on the function, with the `--rom` workaround, rather than adding a runtime search path that would guess. Not changed, with reasons given in the review reply: the GNU `find -printf` in frame_breakdown.sh is unreachable off Linux because the script skips earlier when `perf` is absent; the `if: false` BOLT steps are deliberate, since deleting them would delete the explanation of why the gate cannot measure its subject; and frame_probe nearest-rank percentiles are an intentional choice documented at the function, interpolation being the wrong default for latency tails. Verified: clippy clean at -D warnings, cargo fmt clean, YAML + shellcheck clean on the extracted probe body.
Owner
Author
Review closeoutAll 11 review threads replied to and resolved (0 unresolved), plus the Antigravity suggestions below. 8 findings fixed, 3 declined with evidence. Fixed
Finding 6 was the important one — Declined
AntigravityNo blocking issues. Both actionable suggestions applied: Verification
|
Addresses the CodeRabbit re-review on #348. Three findings, all valid. The release notes said the campaign found "nothing left to find in the emulation core". That overstates what was measured: the campaign itself surfaced two core leads it never measured -- the APU at 18.7% of frame and range.rs inlined inside Ppu::tick at 1.52% -- and the plan doc carries them forward explicitly. A release whose entire subject is not claiming more than the evidence supports should not open with an unsupported claim. Now scoped to "none of the ten hot-path candidates it measured yielded a shippable improvement", with the two unmeasured leads named. docs/STATUS.md carried the same overreach ("no incidental overhead left to reclaim") and is corrected the same way. bench_relative_check.sh validated BENCH_MAX_REGRESSION_PCT only implicitly, via the awk fallback, and copied BENCH_MAX_NOISE_CV_PCT verbatim -- so an override like `3oops` reached the python comparison and died with a traceback AFTER both benches had already been paid for. Both are now checked up front and exit 2 with a clear message. Verified for non-numeric and multi-dot input. frame_breakdown.sh still carried a stale validation claim at the probe invocation ("the check that the debuginfo build did not perturb the thing being measured") that contradicted the banner printed two lines later saying no stock build is made. The comment now matches the behaviour. Verified: shellcheck + bash -n clean, markdownlint clean.
Addresses the five "Outside diff range" findings on #348 -- the ones that live in the CodeRabbit review body rather than as resolvable threads, and would have been missed by only working the thread list. THE BOLT ARTIFACT CONTAINED NO BOLT. The upload step used `path: target/**/release/rustynes`, which matches the PGO binary that scripts/pgo/run.sh has already written to that path -- so an artifact published as `rustynes-pgo-bolt` was the PGO output. This is the same mislabelling as the bench gate disabled earlier in this PR, which reported a plain build as a "BOLT speedup vs plain release": both took whatever was lying in target/ and named it BOLT. Now uploads `rustynes-bolt-optimized`, the file `cargo pgo bolt optimize` actually writes, renames the artifact to match, and switches if-no-files-found from `warn` to `error` -- an absent BOLT binary should be a visible failure, not a quietly empty artifact that looks like a successful build. frame_probe accepted unknown arguments with a warning and then ran anyway, so `--frmaes 400` printed "ignoring unknown argument" and measured the DEFAULT 600-frame corpus -- a number for a run nobody requested, which is the same failure mode as the `--frames 0` case fixed earlier. Unknown flags and a value-less `--rom` now exit 2. Verified. frame_breakdown.sh bucketed anything outside the five emulation crates as "std inlined at emulator call sites", which also swept up inlined third-party crates AND the harness own frame_probe.rs driver loop. The bucket is renamed NONWORKSPACE-INLINED ("std + deps inlined at call sites") and rustynes-test- harness is added to the source-map scan under a new HARNESS bucket, so the probe driver is reported as "probe driver (not emulator work)" instead of masquerading as standard-library cost inside the emulator profile. Verified: workspace clippy clean at -D warnings, cargo fmt clean, YAML and shellcheck clean.
Owner
Author
Outside-diff-range + latest Antigravity closeoutFive outside-diff findings and a second Antigravity pass, addressed in The important one: the BOLT artifact contained no BOLT
This is the same defect as the bench gate disabled earlier in this PR, which reported a plain build as Also fixed
Antigravity's blocking issue — declined, verified false
It is set, unconditionally, before any use: Both functions are defined after line 167 and called later still, so contention detection cannot silently disable itself this way. The stated contrast with Its remaining suggestions: VerificationWorkspace clippy clean at |
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 `@scripts/bench_relative_check.sh`:
- Around line 107-119: Update the validation patterns for MAX_NOISE_CV_PCT and
MAX_REGRESSION_PCT to explicitly reject a lone "." alongside the existing
invalid numeric forms. Ensure both overrides fail with the current error
handling before benchmarks run.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2f46896a-39d0-41cf-970c-c219873178fa
📒 Files selected for processing (6)
.github/release-notes/v2.3.1.md.github/workflows/pgo.ymlcrates/rustynes-test-harness/src/bin/frame_probe.rsdocs/STATUS.mdscripts/bench_relative_check.shscripts/perf/frame_breakdown.sh
The validation added earlier in this PR rejected empty, non-numeric and
multi-dot values, but a bare `.` slipped through: it contains no character
outside [0-9.] and has only one dot, so both patterns missed it -- and
`float(".")` raises, so the script would still have died in the python
comparison AFTER paying for both benchmark runs, which is the exact failure the
validation was added to prevent.
A character check alone cannot express "is a number"; a digit must also be
required. Folded both variables into one `require_number` helper that applies
the character/dot patterns and then requires at least one digit.
Verified against the full matrix, and cross-checked that every accepted value is
one python can parse:
REJECT . .. 3oops 1.2.3 <empty> abc 1e5
accept 10 3.33 .5 5. 0
(`1e5` is rejected deliberately: these are percentages typed by a human at a
shell, and accepting scientific notation to match float() exactly would widen
the surface for no practical gain.)
Antigravity review (Gemini via Ultra)This release adds performance measurement tooling ( Blocking issues
Suggestions
Nitpicks
Automated first-pass review by |
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.
Closes nothing; opens the v2.3.x performance line.
v2.3.1 builds the measurement apparatus and then uses it. The core hot-path
campaign originally planned as a separate v2.3.2 "Grain" ran to completion,
measured ten items and rejected all ten, and so had no shippable content of
its own — it is folded in here, because these results are the answer this
tooling existed to produce. The "Grain" name moves to the frontend work.
No emulation-core changes. AccuracyCoin holds at exactly 141/141 and nestest
is 0-diff, verified after every experimental probe was reverted rather than
merely by construction — this branch did land and remove real edits.
Why the tooling came first
Two failures motivated it, both from the immediately preceding release:
+2%on a contended host and−5.13%re-measuredquiet — the same commit, opposite sign. Nothing in the tooling noticed the
host.
perf reportshows zerorustynes_apu::symbols at any percent limit,because fat LTO inlines the APU wholesale into
cpu_clock. The working split"PPU ~53%, CPU+bus ~39%" silently folded ~19% of the frame into the wrong
bucket.
What the tooling found
frame_probeexp/sort work was ~17% of every profileframe_breakdown.sh--inlinedoes not recover it)ab_check.shA/B/A controlCorrected split: PPU 52.1% / APU 18.7% / CPU 10.1% / coupling 9.9% /
std-inlined 6.7% / mappers 2.5%. The CPU proper is about a third of what the
symbol profile implied.
Ten items, ten rejections, six mechanisms
repr(Rust)ignores source order; G7 already inlinedThe diversity is the point: this is not one bad assumption repeated. The
per-dot loop has no incidental overhead left to reclaim — its ~3.78 ms is work
the accuracy model requires. That corroborates the existing record, where
emit_pixelbounds-check elision and the SIMD blitter both measured slower.Two near-misses worth reading
workloads — from an order-bias artifact. It measured as exactly zero on
re-run. This is what prompted the A/B/A control.
nestest_fast, a shippedconfiguration, with a clean control on that workload. Re-run: +0.01%
(p = 0.96). Under the relaxed sub-3% adoption bar, run 1 alone was an adopt.
Both were caught only by the mandatory second run. Method notes are recorded in
docs/performance.mdalongside the numbers.Also here
apt-get install bolt— on Ubuntu that packageis the Thunderbolt 3 device manager, so the stage failed on the tool it had
just "confirmed" instead of skipping. (BOLT itself remains unmeasured; a
follow-up fix for its runtime library is on the Grain branch.)
Verification
cargo test --workspace --features test-romsgreen; AccuracyCoin 141/141,visual_regression9/9.-D warnings;cargo fmt --all --checkclean.bash -n+shellcheckclean on all touched scripts.Summary by CodeRabbit
New Features
Bug Fixes
Documentation