Repository navigation
Commit 9151e70
v2.7.5 "Tally": every audit claim closed with a measurement or a reason (#553)
* refactor(core): deprecate the dead bus interfaces, correct the drift
Core audit §4.2-§4.4 and §4.7, for v2.7.5. Removal of anything deprecated
here is decided at v2.9.0 (ADR 0041); this release only marks it.
§4.4, 18 rustynes_cpu::Bus methods. The audit named 22 methods "never called
by Cpu". I did not grep for that: a name like `cpu_read` is also an inherent
method on the core bus and on every mapper, so text search cannot tell a
trait call from anything else. Instead every trait method was temporarily
marked #[deprecated] and the compiler listed each real call site, for the
workspace (all targets) and for every feature set CI builds
(cpu-boot-trace, irq-timing-trace, ppu-state-trace, ppu-fetch-trace,
phi2-write-sweep, test-roms, commercial-roms + debug-hooks, full). Result:
- 16 methods have no call site at all.
- poll_irq and oam_dma_in_flight are called only from the default bodies
of two of those (poll_irq_at_phase, oam_dma_overlap_ready), so they are
dead too; the two calls carry #[allow(deprecated)] with that reason.
- 5 of the audit's 22 are LIVE and are not touched: cpu_read / cpu_write
(the trait's default read / write, and the core's own calls),
on_cpu_cycle (the default cpu_clock a stub bus relies on),
dmc_dma_defer_load_entry and oam_dma_overlap_cycle (the core calls both).
- The audit missed internal_data_bus (dead, deprecated here) and
trace_instr (live: cpu.rs calls it under cpu-instr-cycle-trace).
The report's own §1.4 scorecard says 18; its §4.4 list says 22 and is
wrong in the five places above. `-D warnings` across the workspace is now
the standing proof: a new call to any of the 18 fails the lint.
§4.3, ApuBus. Nothing implements or calls it; the bus drives the DMC
sample fetch through Apu::dmc_dma_pending / dmc_dma_addr /
complete_dmc_dma inside the CPU's unified DMA (bus.rs 3685-3693,
3832-3836). Deprecated; the crate re-export keeps working under
#[allow(deprecated)] until v2.9.0.
§4.2, naming. AGENTS.md said `rustynes-core::Bus` is borrowed by the CPU
during `tick()`: there is no type named Bus in rustynes-core and no tick().
The same section also still said the scheduler advances one PPU dot per
`tick_one_dot()` -- the dot-lockstep design v2.0.0 retired. Both
paragraphs are rewritten from the code (Cpu::step, start_cycle /
end_cycle, run_ppu_to, the 12:4 and 16:5 dividers), and docs/scheduler.md
likewise (it described ApuBus as live with methods it never had). The
suggested `pub type Bus = LockstepBus` alias is declined: a second name for
one type is the drift being reported.
§4.7, three doc strings, all confirmed: zapper_temporal_light documented
"default off" (on since v2.3.6) with a link to the wrong type;
RomError::FdsUnsupported still read "planned for v2.2.0" -- FDS shipped in
v2.2.0, and the message now says what a mobile user who opens a .fds needs
to know (the image loads through the disk loader with a BIOS, not as a
cartridge); the cpu.rs header described the retired on_cpu_cycle stepping
interface and now describes start_cycle / end_cycle in the order the code
runs them.
Also adds the v2.7.5 plan and records the verdicts in the core ledger.
Verified: cargo clippy --workspace --all-targets -D warnings; clippy for
rustynes-apu / rustynes-core; rustdoc -D warnings on rustynes-apu and
rustynes-cpu; markdownlint on the changed docs. No behaviour change: the
only runtime-visible string is the FDS error text, which no test matches.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj
* fix(hdpack): refund the pack budget when a PNG fails after its header
Carried into v2.7.5 from agy's round 4 on #551, where the per-pack decode
budget (SEC-01) was introduced.
decode_png checks a PNG's declared dimensions against what is left of the
pack's RGBA budget, then charges the budget, then decodes the pixels. The
charge is taken from the header alone so that no pixel buffer is allocated
for an image that would not fit. But three paths after the charge return
None without giving it back: output_buffer_size() failing, next_frame()
failing (truncated or corrupt IDAT), and an unexpanded Indexed image. A
corrupt image near the limit therefore spent budget that a valid image
later in the same pack needed, and that image was then refused as "over
budget". Bounded (nothing allocates past the budget) but wrong.
The pixel half moves into decode_png_pixels, and decode_png refunds the
charge whenever it returns None. The header checks and the charge are
unchanged, so the SEC-01 guarantee -- no allocation for an image that does
not fit -- still holds.
Test a_png_that_fails_after_its_header_refunds_the_budget builds a valid
64x64 grayscale PNG and cuts it two bytes into its IDAT data, so the header
parses (and is charged) and the decode fails. Red first: the budget read 0
of 16,384 after the failed decode, and the valid image at exactly the
remaining budget was then refused. The red run is also what proves the
truncated image reaches the charge rather than failing in read_info, which
would make the test vacuous. Mutation: deleting the refund line fails it.
Verified: cargo test -p rustynes-hdpack (74 passed, 1 ignored); clippy -D
warnings on rustynes-hdpack all targets and on rustynes-frontend with
scripting,hd-pack.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj
* docs(perf): measure the core audit's twelve hot-path proposals; adopt none
Core audit §3 and IMP-12 proposed twelve hot-path changes; v2.7.5 measured
them before building any. Nothing in the engine changes in this commit: every
probe was reverted, and the tree's code is exactly the previous commit's.
What lands is the record in docs/performance.md, the ledger verdicts, and a
correction to the plan.
Method. The project's standing record is heavily against micro-optimising
this core (the v2.3.1 campaign: ten items, none adopted), so the first run
was ONE combined ceiling probe: every proposal at its maximum in a single
tree, knowingly breaking correctness where the real change would be subtle --
CPU divider hard-coded to 12, take_dma_mc_consumed removed, sample_nmi_edge
removed from all three PPU catch-up loops, branchless set_nz, the four
redundant PPU stores deleted, BlipBuf::drain_all keeping its capacity, the
pulse mute check deleted, the cpu_read_unmapped call skipped. The result
bounds what the whole set could win, so a zero rejects all eight in one
measurement with no correctness risk.
scripts/perf/ab_check.sh, AB_MEASUREMENT_TIME=25, fat LTO, the four shipped
workloads, each with its A/B/A order-bias control:
run 1 +0.63 +1.29 +1.14 -0.54 % control +0.25 +0.39 +0.93 +2.48
run 2 void: control swung -5.5% .. +12.1% (indexers, Syncthing)
run 3 +0.14 -0.07 +0.13 +0.08 % control +0.16 +0.06 -0.16 -0.00
The ceiling is zero.
IMP-05 could not share that bound (inlining can move either way), and its
premise, unlike v2.3.1 G7's, is true: read1, write1, end_cycle, idle_tick and
adc survive as out-of-line symbols. Its candidate outlined read1's DMC-abort
and unified-DMA drains as #[cold] #[inline(never)] behind a guard of the
same two predicates (both pure &self, checked, so the double evaluation is
exact) and hinted read1 #[inline]. The outlining took (read1_slow_path is its
own symbol, confirmed with nm on the candidate build, not the reference the
harness re-benches last). Run 1 void (another session's pytest; control
+4.5%); run 2 clean: -0.15 -0.25 -0.11 +0.10 against control -0.18 -0.06 -0.17
+0.07. Zero.
IMP-06's branchless palette got its own ceiling (palette_index is on the hot
path, 61,440 calls a frame via emit_pixel -> read_palette): mirroring
deleted, +0.01 +0.04 +0.34 +0.32 against control -0.18 +0.06 +0.22 +0.11.
Zero. Its phase-match fold has no ceiling and was not pursued. IMP-12 is
v2.3.1's G10, already rejected.
Premises the measurement made moot but which were wrong anyway, recorded so
nobody re-derives them: §3.1 A's "integer division" is div / 2, a shift;
§3.1 B's accumulator is zero in production because the core overrides
cpu_clock (its only feeder is reached from unit tests), not for the reason
given; IMP-07's "eliminates per-frame allocations" cannot hold for a Vec
returned by value.
IMP-07's second half, a 16,384-sample cap on BlipBuf, is rejected as a
behaviour change with no user. Correction: the plan committed in cf377ed
said several tests and the movie round-trip drain after more than 16,384
samples. Checked, that is false -- every one drains each frame. The real
case is the probe engine's undrained framebuffer trials (about 24,000
samples over 30 frames, per rustynes-probe/tests/restore_audio_pin.rs),
which a cap would silently truncate. The plan now says so.
MOB-06 (the mobile framebuffer copy) is deferred as blocked upstream: the fix
needs UniFFI's mutable borrowed bytes (&mut [u8]), which exist only on its
unreleased main; 0.32.2, the newest release, has read-only borrowed bytes.
Checked in the registry sources of 0.32.1 and 0.32.2, not only the docs.
With these, the core and frontend ledgers have no UNTRIAGED row left.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj
* chore(release): cut v2.7.5 "Ledger"
The sixth and last release of the v2.7.x audit line: every row of the core
and frontend audit ledgers now carries a verdict with its evidence. The work
is the three commits before this one; this commit is the version bump and
the release documents.
- Workspace and rustynes-cosim to 2.7.5 via bump_release.py; the nested
"Built on" chains trimmed to one level (AGENTS, README, SUPPORT,
VERSION-PLAN, to-dos/ROADMAP) as at every cut; the to-dos/ROADMAP
release-line entry written by hand, as the script requires.
- CHANGELOG [2.7.5] under a kept [Unreleased]; a VERSION-PLAN row (v2.7.4
loses "(current)"); the ROADMAP status line; the release-notes override.
- The plan's As-built table and the line plan's v2.7.5 row.
Two corrections made while writing, recorded so the history carries them:
- The README and AGENTS.md paragraphs after the current-release anchor
still described v2.7.4, and the README said v2.7.4's mobile changes "are
checked on devices by the maintainer before release". v2.7.4 has merged
and nothing in this repository records a run of that checklist, so the
text now says exactly that instead.
- The release notes first said the twelfth performance proposal was
"already measured in v2.3.1". Only one of the last two was (IMP-12, as
G10); the other, the phase-match fold, was closed by reasoning and never
measured. Fixed before commit.
Verification on this tree:
cargo test --workspace --features test-roms
154 suites, 2,718 passed, 0 failed (v2.7.4: 2,716)
AccuracyCoin (RAM decoder): total=144 pass=133 pass_with_code=11 fail=0
not_run=0
nestest: pass
clippy -D warnings: workspace, scripting+hd-pack, retroachievements, full,
both wasm32, the harness trace features, test-roms+phi2-write-sweep,
and the excluded rustynes-cosim
rustdoc -D warnings; thumbv7em no_std; ios-host-typecheck; fmt
release_anchor / release_state_prose / release_notes_render audits pass;
pre-release.sh: nothing definitively wrong
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj
* docs(release): restore v2.7.3 in the README lineage, refresh two dates
Copilot round 1 on #553; all three findings held.
README: the release cut trims the current-release anchor's nested "Built
on" chain to one level, and the anchor now names v2.7.4 as its base. The
paragraph after it still opened its own chain at "Built on v2.7.2" -- which
was consistent at v2.7.4, when the anchor named v2.7.3, and skips v2.7.3
"Hearth" now. The paragraph names Hearth again. This is the second
release running where the paragraph after the anchor went stale because the
bump script rewrites only the anchor; worth remembering at the next cut.
OVERVIEW.md and ROADMAP.md carried "Last Updated: 2026-09-17" beside
content that states a 2026-09-24 release; both refreshed. bump_release.py
does not touch these fields.
Verified: release_anchor_audit and release_state_prose_audit pass;
markdownlint on the three files.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj
* perf(apu): drain_all keeps its capacity for the next frame (IMP-07)
Core audit IMP-07, adopted after review on #553 exposed that my first
measurement of it measured nothing.
The change: BlipBuf::drain_all returned the filled buffer with
mem::take, leaving a buffer of capacity zero that the next frame regrew by
doubling (about nine reallocations for a frame's ~800 samples). It now
replaces it with Vec::with_capacity(previous capacity): one allocation of
the size the last frame needed. A buffer returned by value has to be
replaced on every call, so the audit's "eliminates per-frame allocations"
cannot hold; what changes is how many.
The correction. v2.7.5's combined ceiling probe included this change and
the record said the probe bounded it at zero. It could not have: the stock
full_frame benches restore a snapshot per iteration (which replaces the
BlipBuf) and never drain audio, so drain_all never runs in them. CodeRabbit
questioned the combined probe's per-item bound on #553; checking the bench
for this item is how the gap surfaced.
The measurement it needed: a temporary (uncommitted) bench on the shape
Nes::drain_audio callers actually have -- one frame, then a drain, on the
same machine every iteration -- run as a manual A/B/A with criterion
baselines, pinned to cores 2-5, 25 s per measurement:
run reference candidate control (reference again)
1 3.958 ms 3.924 ms (-0.89%) 3.939 ms (-0.52%)
2 3.952 ms 3.916 ms (-0.89%) 3.959 ms (+0.22%)
p = 0.00 both runs; the candidate is the fastest measurement in both, with
the controls on either side of the reference. Under ab_check.sh's adoption
rule (the v2.3.1 maintainer decision: a consistent, reproduced, clean gain is
adoptable below 3%) this is adopted. AGENTS.md's hot-path note still says
">3%"; the conflict is flagged in the PR rather than decided here.
Who it affects: callers of Nes::drain_audio -- the mobile bridge and the wasm
build. The desktop and libretro drain with drain_audio_into into their own
buffer (BlipBuf::drain), which allocates nothing and is untouched. The
samples are not touched either, so audio is byte-identical by construction.
Test drain_all_keeps_room_for_the_next_frame, red first (capacity 0 against
717 drained samples); reverting to mem::take fails it. cargo test -p
rustynes-apu: 157 passed.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj
* docs(core): internal CPU cycles run both halves with no bus access
CodeRabbit on #553. The rewritten cpu.rs header (cf377ed), like the AGENTS.md paragraph
written with it, said every CPU cycle is a real bus access. Cpu::idle_tick calls
start_cycle and end_cycle with no access between them, so for internal
cycles that is false. The header now says what the code does: every cycle is
clocked in two halves; read1 / write1 perform their access between them;
idle_tick runs both halves with no access.
Comments only. The matching AGENTS.md paragraph lands with the release-document
commit that follows, since that file also carries the codename change.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj
* docs(release): rename v2.7.5 to "Tally", record IMP-07's adoption
CodeRabbit round 1 on #553 made five findings; four held and one is
declined. Acting on them changed the release in two ways.
1. The codename. v2.7.5 was "Ledger", which v2.3.4 already is. Multi-release
line names repeat by design ("Fathom", "Harbor"), but two unrelated
standalone releases sharing one would make the tags and release titles
ambiguous. Renamed to "Tally" everywhere v2.7.5 is named; the plan file
moves to to-dos/plans/v2.7.5-tally-plan.md. v2.3.4 "Ledger" is untouched,
as is the word ledger for the audit ledgers.
2. One performance proposal is adopted. CodeRabbit challenged the combined
ceiling as a per-item bound (offsets can hide an individual effect).
Checking each item for that found IMP-07 could never have been bounded
by it: the full_frame benches never drain audio. Measured on a
frame-then-drain bench it is -0.89% on two runs and was adopted in
8aba4e3. IMP-04, the one rewrite in the set, was run alone twice: zero.
IMP-05 got a third run and the palette ceiling a second: zero. So the
record now reads twelve proposals closed, eleven by measurement, one
adopted -- in docs/performance.md (the section rewritten: IMP-07 taken
out of the combined table with the reason, the new runs' tables, the
decision heading), the core ledger, the CHANGELOG, the release notes,
the plan, the line plan, and every current-release anchor.
3. "Twelve measured" was an overclaim even before that: the phase-match
fold was closed by reasoning. Every anchor now says eleven by
measurement.
4. AGENTS.md's architecture paragraph said every CPU cycle is a bus
access; idle_tick's are not. It now says what cpu.rs's header says since
bb2018e.
Declined: syncing the upstream libretro-super .info now. The plans put the
local metadata fix at v2.8.1 and the upstream sync at v3.0.0.
Also: the notes' verification rows name both red-first tests, and the
"two clean runs" wording is narrowed to what is true (two runs read against
their own controls; runs whose control swung by several percent reported as
void).
Verification on this tree (blip.rs changed in 8aba4e3, so all re-run):
cargo test --workspace --features test-roms: 154 suites, 2,719 passed,
0 failed (+1: the IMP-07 test)
AccuracyCoin (RAM): total=144 pass=133 pass_with_code=11 fail=0 not_run=0
fmt; clippy -D warnings workspace, full, both wasm32; rustdoc; no_std;
release_anchor 15/15, release_state_prose 2/2, release_notes_render 8/8
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj
* fix(apu): clamp the capacity drain_all keeps; finish the claim cleanup
agy and CodeRabbit on #553, after the IMP-07 adoption.
drain_all (agy). Keeping the previous capacity made it a high-water mark: a
caller that once let audio pile up would leave a buffer that size resident
for as long as the BlipBuf lives. The probe engine's undrained trials reach
~24,000 samples; the new test's one-second backlog kept 65,536 slots. The
kept capacity is now clamped at DRAIN_ALL_KEEP_CAPACITY = 4,096 samples,
four frames at 48 kHz. A per-frame drain keeps ~1,024 slots, below the
clamp, so the path measured at -0.89% is unchanged by it. Test
drain_all_does_not_keep_a_high_water_mark: red first ("kept 65536 after a
47982-sample backlog"); removing the clamp fails it.
Claims (CodeRabbit, second pass):
- AGENTS.md's Timebase bullet still said "every CPU cycle a real bus
access" -- the same overstatement bb2018e fixed in the architecture
paragraph, in text that predates this release. Now: every cycle clocked in
two halves, any access split between them.
- The VERSION-PLAN row said the twelve proposals "were measured before any
was built" with a probe covering "every proposal"; now ten measured here,
IMP-12 in v2.3.1, the phase-match fold by reasoning, and a probe over
seven.
- The CHANGELOG heading "the core audit's proposals, measured" drops the
last word; its bullets already give the breakdown.
Declined (agy): turning decode_png's Option into a Result. None for "an image
this loader cannot use" is the function's documented contract since v2.7.3;
this release only moved lines within it.
Verified: cargo test --workspace --features test-roms: 154 suites, 2,720
passed, 0 failed (+1); cargo test -p rustynes-apu 158 passed; clippy -D
warnings on rustynes-apu; fmt; release anchor and prose audits; markdownlint.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj
* perf(apu): an empty drain_all allocates nothing
agy on #553 (017173f). With the capacity kept between frames, a drain with
nothing queued allocated a replacement buffer and handed back the old,
empty one for the caller to free: one allocation and one free for no
samples. It now returns Vec::new(), which does not allocate, and leaves the
buffer as it is. Output is unchanged.
Test an_empty_drain_all_allocates_nothing, red first (the empty drain
handed out a 4,096-slot buffer); removing the early return fails it.
The same round's blocking finding, a u32 overflow in the HD-pack budget
arithmetic, is refuted: the quoted expression is not in the file, which
has widened both factors to u64 before multiplying since v2.7.3.
Verified: cargo test --workspace --features test-roms: 154 suites, 2,721
passed, 0 failed (+1); cargo test -p rustynes-apu 159 passed; clippy -D
warnings on rustynes-apu; fmt.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj
* docs: state the performance adoption rule as ab_check.sh records it
Maintainer instruction on #553: AGENTS.md carries the same language as
scripts/perf/ab_check.sh.
AGENTS.md said every optimization "must clear the project's >3% same-runner
A/B bar and stay byte-identical". ab_check.sh -- the tool that adjudicates
adoption -- records the v2.3.1 maintainer decision that the bar is evidence
quality, not effect size: a consistent, reproduced, statistically clean
gain is adoptable even below 3%, the second independent run is not
negotiable, and a mixed-sign result across workloads is a rejection. The
two disagreed, and v2.7.5's IMP-07 (-0.89%, reproduced) was adopted under
the script's rule with the conflict flagged in the PR. AGENTS.md now states
the script's rule, keeps byte-identity as a hard requirement, and notes
that older docs/performance.md records cite the bar in force when written.
Two other live statements of the rule are aligned: docs/audits/README.md
(the standing rules an audit fix is checked against) and the v2.7.x line
plan's v2.7.5 criterion, which said "kept only above 3%" beside an As-built
row that adopts a 0.89% gain; it carries a dated note rather than a silent
rewrite. Historical records in docs/performance.md, older plans, and the
separate pgo.yml promotion gate are left as written: they describe decisions
taken under the rule of their day.
Markdown only; markdownlint passes on the three files.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj
---------
Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>1 parent e02998b commit 9151e70
32 files changed
Lines changed: 717 additions & 103 deletions
File tree
- .github/release-notes
- crates
- rustynes-apu/src
- rustynes-core/src
- rustynes-cosim
- rustynes-cpu/src
- rustynes-hdpack/src
- rustynes-libretro
- rustynes-mappers/src
- docs
- audits
- to-dos
- plans
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
0 commit comments