From 75e5468f41c2a34ef5a82fcff72a6cd109d577fa Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Sun, 16 Aug 2026 17:12:00 -0400 Subject: [PATCH 01/10] docs(agents): the bot ceremony must read review bodies, not just threads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit posts "Outside diff range" and other suppressed findings inside the review BODY, where a resolve-every-thread sweep cannot see them; Copilot does the same. Three defects have reached `main` or nearly shipped through that gap: - issue #360, an untested replay-attestation path (landed unaddressed); - two findings on #357, one CRITICAL — two threads producing frames during fast-forward under threaded display-sync (fixed in #358); - a use-after-free in the v2.3.5 libretro controller tables, caught only because the review body happened to be read. "All threads resolved" is therefore not evidence the review was addressed. The rule now says so, and names the command that actually surfaces them. This is the last open item of issue #360; its test work landed in 63ba1fe9 (PR #373, v2.3.4) and is verified present — every recording test in `movie_ui.rs` now feeds `after_frame`, `a_recorded_movie_verifies_against_a_fresh_nes` replays against a fresh `Nes`, and `a_recording_that_never_attests_fails_verification` is the negative control that deliberately does not attest. Co-Authored-By: Claude Opus 5 --- AGENTS.md | 1 + 1 file changed, 1 insertion(+) diff --git a/AGENTS.md b/AGENTS.md index 5742b2f5..374bc208 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -222,6 +222,7 @@ These cross-cutting decisions span multiple files. Reading individual chip docs - **There is ONE toolchain, `rust-toolchain.toml`'s `channel`, and no version literal anywhere in `.github/` — don't add one.** `.github/actions/rust-setup` parses the channel out of that file and fails closed if it can't, so a toolchain bump is a one-line edit there. Pass the composite's `toolchain:` input only to install something *deliberately* different from the project pin. **The resolver is table-scoped `awk` on purpose — do NOT "simplify" it back to a one-line `sed`.** Matching the first `channel = "..."` *anywhere* in the file (the first implementation, caught in review on PR #322) resolves `nightly` if any other table carries a `channel` key ahead of `[toolchain]` — silently installing the very toolchain this setup exists to keep out, while the step still reports success. `awk` rather than `tomllib` because the step runs on Windows and macOS runners too and Python ≥3.11 is not a safe assumption there; only double-quoted TOML strings are accepted, and anything else (missing table, single-quoted value, empty file) aborts the job rather than being guessed at. The old `stable` default was misleading rather than wrong: `rust-toolchain.toml` is a directory override that outranks the `rustup default` the action performs, so every job was already compiling on 1.96.0 (rustup logs `overridden by .../rust-toolchain.toml`) — `stable` just downloaded a second toolchain nothing used and made the workflows *read* as though they tested latest stable, which they never did. **Nightly is used in exactly one place, not a gate:** `cargo fuzz` (hard requirement — libFuzzer's sanitizer flags are nightly-only). If you think a CI job needs nightly, it doesn't. - **`rust-libretro 0.3.2` is unmaintained (no commit since 2023-02) and has a MinGW bug we work around.** It casts a keycode with `cfg(target_family = "windows")`, but C enum signedness follows the *ABI*: only **MSVC** gives plain enums `int` — under **MinGW** (`x86_64-pc-windows-gnu`, what the buildbot builds) bindgen emits `c_uint` and the crate fails `E0308`. `.cargo/config.toml`'s `[env] BINDGEN_EXTRA_CLANG_ARGS_x86_64_pc_windows_gnu = "--target=x86_64-pc-windows-msvc"` fixes it; the generated-bindings diff is 28 lines, all enum signedness. Don't "clean up" that env var without rebuilding for `x86_64-pc-windows-gnu`. - **CodeRabbit is now a 3rd automated PR review bot** (`.coderabbit.yaml`, added 2026-07-20 in PR #316), alongside gemini-code-assist and copilot-pull-request-reviewer — same reply-and-resolve-every-thread ceremony applies before any merge. Configured `profile: assertive` (not the "chill" default) and a `tools{}`/`path_instructions`/custom-checks set audited against this repo's actual file footprint, not guessed. `tone_instructions` has a hard 250-character schema limit that fails validation silently on the CodeRabbit side — after editing `.coderabbit.yaml`, verify with a `@coderabbitai configuration` PR comment and confirm every changed field shows `Source: Repository YAML (base)`. +- **The bot-comment ceremony must read the review BODIES, not just the resolvable threads.** CodeRabbit posts "Outside diff range" and other suppressed findings **inside the review body**, where they are invisible to a resolve-every-thread sweep — and Copilot does the same. This has now cost the project three times: issue #360 (an untested attestation path) reached `main` unaddressed; two findings of the same class on #357 were genuine defects, **one critical** (two threads producing frames during fast-forward under threaded display-sync, fixed in #358); and a **use-after-free** in the v2.3.5 libretro controller tables was caught only because the review body was read. A green "all threads resolved" is not evidence the review was addressed. Fetch the bodies explicitly — `gh pr view --json reviews --jq '.reviews[].body'` — and triage every finding in them before merging. - **lz4_flex 0.14+ requires the crate's own `alloc` feature explicitly** for `compress_prepend_size`/`decompress_size_prepended` (used by `rewind.rs`/`zwinder.rs`) — it split real no_std support into an `alloc`-vs-`std` distinction that didn't exist in 0.13. A `cargo build --workspace` will NOT catch a missing `alloc` feature here because `rustynes-core`'s own default-on `std` feature implies it via cargo's feature unification; only a standalone `cargo build -p rustynes-core --target thumbv7em-none-eabihf --no-default-features` (the exact CI `no_std build` job) will. Run that command locally before pushing any bump that touches this dependency. - **The libretro `.info` RetroArch reads is a DIFFERENT FILE from this repo's, and it went stale for eleven days.** RetroArch downloads `dist/info/rustynes_libretro.info` from `libretro/libretro-super`; `crates/rustynes-libretro/rustynes_libretro.info` is an unrelated copy that nothing syncs and nothing compared. So the v2.2.9 GPL relicense reached `Cargo.toml`, `NOTICE`, `deny.toml`, the SPDX headers and the local `.info` — and **not** the file users actually see, which went on advertising "MIT OR Apache-2.0" at `display_version = v2.2.1`. Both upstream PRs had merged *exactly two weeks before* the relicense, so no sync could have carried it. **A license change is now a mandatory upstream-sync trigger**, on the same footing as a release. `crates/rustynes-test-harness/tests/libretro_info_audit.rs` pins the local file against the workspace manifest so the sync is a *copy*, never a re-derivation; it cannot see upstream, so the sync itself stays a human step. libretro `.info` uses short license tokens, not SPDX, and marks "or later" with a trailing `+` (tallied across all 316 upstream cores: `GPLv2` x100, `GPLv3` x64, `GPLv2+` x19, `GPLv3+` x5) — RustyNES is **`GPLv3+`**; a bare `GPLv3` understates it as GPL-3.0-only. Full detail + the surface table: `docs/libretro/UPSTREAM_SYNC.md`. From d1baa70c1884f8c1091583ff98ea79442d0d5dcb Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Sun, 16 Aug 2026 17:15:46 -0400 Subject: [PATCH 02/10] =?UTF-8?q?feat(probe):=20rustynes-probe=20=E2=80=94?= =?UTF-8?q?=20the=20deterministic=20re-simulation=20engine?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three planned v2.3.6-v2.3.8 tools reduce to one primitive: take a snapshot anchor, re-simulate N times under controlled variation, and find the first observable divergence. - Latency Oracle: replay with a button held and with it never pressed; the first frame that differs IS the game's internal input lag. - RAM Atlas: replay with a byte perturbed and see what changes. - Divergence Lens: replay two configurations and find where they part. Writing that three times would produce three subtly different answers to the same question, so it is written once here. RustyNES can do this soundly because its determinism contract is a hard guarantee rather than an aspiration — the property save-states, TAS replay and netplay rollback already depend on. A probe result is a property of the ROM, not of the run. Design notes worth keeping: - The engine does NOT own a `Nes`. The caller passes a scratch instance, so a frontend can reuse one and leave the live emulator untouched. - Every observable reduces to one `u64`, so a divergence search is a linear scan. That deliberately discards HOW two frames differ: this answers WHEN, and Pixel Provenance already answers what. - A budget-truncated trial returns a SHORT vector, which the caller must read as "inconclusive" — never as "no divergence". `agree()` refuses to report agreement for two empty trials for the same reason: nothing ran, so there is nothing to agree about, and a probe that says "no reaction" when it never simulated anything is exactly the failure mode this crate exists to avoid. - Replaying an anchor into an emulator running a different ROM panics rather than producing a plausible wrong answer. - `AudioEnergy` is quantised on purpose: exact float equality across a resampled stream compares noise, not signal. Eleven tests plus a doctest. They cover the contract the engine rests on (identical inputs => identical samples), that each trial genuinely restarts from the anchor, all four observables, both budget ceilings, the exact-frame comparator, and — closing the loop the other tests leave open — that a real one-byte work-RAM difference propagates through the replay into a detected divergence at frame 0. Dependency-light like `rustynes-gamedb` (core only), so it is headless-testable and CI can gate it without winit/wgpu. Not a workspace default-member, so the libretro buildbot's bare `cargo build` is unaffected. Co-Authored-By: Claude Opus 5 --- Cargo.lock | 7 + Cargo.toml | 1 + crates/rustynes-probe/Cargo.toml | 26 ++ crates/rustynes-probe/src/lib.rs | 541 +++++++++++++++++++++++++++++++ 4 files changed, 575 insertions(+) create mode 100644 crates/rustynes-probe/Cargo.toml create mode 100644 crates/rustynes-probe/src/lib.rs diff --git a/Cargo.lock b/Cargo.lock index 24e04257..cc2ba0b9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4503,6 +4503,13 @@ dependencies = [ "thiserror 2.0.19", ] +[[package]] +name = "rustynes-probe" +version = "2.3.5" +dependencies = [ + "rustynes-core", +] + [[package]] name = "rustynes-ra" version = "2.3.5" diff --git a/Cargo.toml b/Cargo.toml index 8225a0d6..e718de09 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -10,6 +10,7 @@ members = [ "crates/rustynes-cheevos", "crates/rustynes-script", "crates/rustynes-gamedb", + "crates/rustynes-probe", "crates/rustynes-frontend", "crates/rustynes-test-harness", "crates/rustynes-mobile", diff --git a/crates/rustynes-probe/Cargo.toml b/crates/rustynes-probe/Cargo.toml new file mode 100644 index 00000000..f5b88e5e --- /dev/null +++ b/crates/rustynes-probe/Cargo.toml @@ -0,0 +1,26 @@ +# v2.3.6 — the deterministic-probe engine. +# +# Three planned tools reduce to the same primitive: take a snapshot anchor, +# re-simulate N times under controlled variation, and find the first observable +# divergence. RustyNES can do that soundly because its determinism contract is a +# hard guarantee rather than an aspiration, so the answer is a property of the +# ROM rather than of the run. +# +# Kept dependency-light on purpose, like `rustynes-gamedb`: it depends only on +# `rustynes-core`, so it is headless-testable and CI can gate it without pulling +# in winit/wgpu. +[package] +name = "rustynes-probe" +version.workspace = true +edition.workspace = true +rust-version.workspace = true +license.workspace = true +authors.workspace = true +repository.workspace = true +description = "Deterministic re-simulation probe: anchor, replay under variation, locate the first divergence" + +[dependencies] +rustynes-core = { path = "../rustynes-core" } + +[lints] +workspace = true diff --git a/crates/rustynes-probe/src/lib.rs b/crates/rustynes-probe/src/lib.rs new file mode 100644 index 00000000..d17dc09e --- /dev/null +++ b/crates/rustynes-probe/src/lib.rs @@ -0,0 +1,541 @@ +//! Deterministic re-simulation probing: **anchor, replay under variation, +//! locate the first divergence**. +//! +//! # What this is for +//! +//! Several questions about a running game have the same shape: +//! +//! - *How many frames of input lag does this game have?* — replay from one +//! anchor with a button held and with it never pressed, and see which frame +//! first differs. That index **is** the game's internal lag. +//! - *What is this RAM byte for?* — replay with the byte perturbed and see what +//! changes. +//! - *Where do these two configurations disagree?* — replay both and find the +//! first frame that differs. +//! +//! Each is "take a snapshot, re-simulate under controlled variation, find the +//! first observable difference". This crate is that primitive, written once. +//! +//! # Why `RustyNES` can do this and most emulators cannot +//! +//! The answers are only meaningful if a replay from the same anchor with the +//! same inputs produces the same frames *every time*. That is `RustyNES`'s +//! determinism contract (`docs/testing-strategy.md`), a hard guarantee rather +//! than an aspiration — the same property save-states, TAS replay, and netplay +//! rollback already rely on. A probe result is therefore a property of the ROM, +//! not of the run, and [`Probe::run`] re-asserts it rather than assuming it. +//! +//! # What it deliberately does not do +//! +//! It does not own a [`Nes`]. The caller passes a scratch instance in, so a +//! frontend can reuse one across probes and keep the live emulator untouched. +//! It does not spawn threads, and it does not interpret results — deciding that +//! "frame 1 differed, therefore the lag is 1 frame" belongs to the tool, which +//! knows what it asked. +//! +//! # Example +//! +//! ```no_run +//! use rustynes_core::{Buttons, Nes}; +//! use rustynes_probe::{Budget, Observable, Probe}; +//! +//! # fn demo(live: &Nes, scratch: &mut Nes) { +//! let probe = Probe::anchor(live, Budget::default()); +//! +//! // Trial A: hold Right from the first frame. Trial B: never press anything. +//! let held = probe.run(scratch, 16, Observable::Framebuffer, |_| { +//! (Buttons::RIGHT, Buttons::empty()) +//! }); +//! let idle = probe.run(scratch, 16, Observable::Framebuffer, |_| { +//! (Buttons::empty(), Buttons::empty()) +//! }); +//! +//! match Probe::first_divergence(&held, &idle) { +//! Some(frame) => println!("reacted on frame {frame}"), +//! None => println!("no reaction inside the budget"), +//! } +//! # } +//! ``` + +use rustynes_core::{Buttons, Nes, ROM_HASH_TAG_LEN}; + +/// Bounds on what a single probe may spend. +/// +/// A probe runs the emulator many times over, from a UI thread in the frontend's +/// case, so it needs a ceiling that does not depend on the game cooperating. A +/// game that never reacts must make the probe *stop and say so* rather than run +/// until something else notices. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub struct Budget { + /// Hard cap on frames simulated per trial. Reached, [`Probe::run`] returns + /// the samples it has; the caller sees a short vector and must treat that as + /// "inconclusive", never as "no divergence". + pub max_frames_per_trial: u32, + /// Hard cap on trials a caller may run against one anchor. Enforced by + /// [`Probe::trials_remaining`]; the engine cannot enforce it alone because + /// it does not drive the loop. + pub max_trials: u32, +} + +impl Default for Budget { + fn default() -> Self { + Self { + // ~2 s of NTSC. Long enough for any input-lag question (games react + // within a handful of frames) and short enough that a probe cannot + // stall a UI frame budget for a noticeable time. + max_frames_per_trial: 120, + max_trials: 64, + } + } +} + +/// What a trial observes at the end of each frame. +/// +/// Every variant reduces its observation to one `u64` so trials compare cheaply +/// and a divergence search is a linear scan of two slices. Hashing loses the +/// ability to say *how* two frames differ — deliberately: this engine answers +/// *when*, and the caller that wants *what* already has Pixel Provenance and the +/// debugger for that. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum Observable { + /// The RGBA framebuffer. The default question — "did the screen change?". + Framebuffer, + /// The palette-index framebuffer. Cheaper than [`Self::Framebuffer`] and + /// immune to a palette or filter change that leaves the rendered indices + /// identical. + IndexFramebuffer, + /// The 2 KiB work RAM. Catches a game that reacted internally without + /// drawing anything yet — a menu highlight committed to a variable one frame + /// before it is rendered, for instance. + Wram, + /// Coarse energy of the audio produced this frame. The fallback for a + /// reaction that is audible before it is visible; quantised, because exact + /// float equality across a resample would be noise, not signal. + AudioEnergy, +} + +/// An anchor plus the budget probes taken from it must respect. +/// +/// Cloning is cheap relative to re-deriving the state, but the snapshot is a +/// real allocation — hold one anchor and run many trials against it rather than +/// re-anchoring per trial. +#[derive(Clone, Debug)] +pub struct Probe { + snapshot: Vec, + rom_tag: [u8; ROM_HASH_TAG_LEN], + budget: Budget, + trials_used: u32, +} + +impl Probe { + /// Capture the anchor: the emulator's full state plus the identity of the + /// ROM it is running. + /// + /// The ROM tag is recorded so [`Self::run`] can refuse to replay into an + /// instance running a different game — restoring a snapshot across ROMs + /// would produce confident nonsense rather than an error. + #[must_use] + pub fn anchor(nes: &Nes, budget: Budget) -> Self { + Self { + snapshot: nes.snapshot(), + rom_tag: nes.rom_hash_tag(), + budget, + trials_used: 0, + } + } + + /// Trials still allowed against this anchor under its [`Budget`]. + #[must_use] + pub const fn trials_remaining(&self) -> u32 { + self.budget.max_trials.saturating_sub(self.trials_used) + } + + /// The budget this anchor was taken under. + #[must_use] + pub const fn budget(&self) -> Budget { + self.budget + } + + /// Restore the anchor into `nes` and run `frames` frames, sampling + /// `observable` after each one. + /// + /// `input` is called once per frame with the zero-based frame index and + /// returns the buttons for controller 1 and 2. It is the only thing that + /// varies between trials; everything else is re-derived from the anchor, + /// which is what makes two trials comparable. + /// + /// Returns one sample per frame actually run. A vector shorter than `frames` + /// means the budget stopped it — treat that as **inconclusive**, not as + /// evidence of no divergence. + /// + /// # Panics + /// + /// Panics if `nes` is running a different ROM than the anchor was taken + /// from, or if the anchor fails to restore. Both are caller errors that + /// would otherwise yield a plausible, wrong answer, and this engine exists + /// to produce answers people will act on. + pub fn run( + &self, + nes: &mut Nes, + frames: u32, + observable: Observable, + mut input: F, + ) -> Vec + where + F: FnMut(u32) -> (Buttons, Buttons), + { + assert_eq!( + nes.rom_hash_tag(), + self.rom_tag, + "probe anchor belongs to a different ROM than the emulator it was \ + replayed into; restoring across ROMs yields confident nonsense" + ); + nes.restore(&self.snapshot) + .expect("probe anchor round-trips: it came from Nes::snapshot"); + + let n = frames.min(self.budget.max_frames_per_trial); + let mut samples = Vec::with_capacity(n as usize); + let mut audio = Vec::new(); + for f in 0..n { + let (p1, p2) = input(f); + nes.set_buttons(0, p1); + nes.set_buttons(1, p2); + nes.run_frame(); + samples.push(sample(nes, observable, &mut audio)); + } + samples + } + + /// [`Self::run`], counting the trial against the budget. + /// + /// Returns `None` once [`Self::trials_remaining`] reaches zero, so a search + /// loop terminates on the budget rather than on the caller remembering to + /// check. + pub fn run_counted( + &mut self, + nes: &mut Nes, + frames: u32, + observable: Observable, + input: F, + ) -> Option> + where + F: FnMut(u32) -> (Buttons, Buttons), + { + if self.trials_remaining() == 0 { + return None; + } + self.trials_used += 1; + Some(self.run(nes, frames, observable, input)) + } + + /// The first frame index at which two trials differ, or `None` if they agree + /// over their common length. + /// + /// Comparing only the common prefix is deliberate: a shorter trial means its + /// budget ran out, and "one ran longer" is not a divergence. + #[must_use] + pub fn first_divergence(a: &[u64], b: &[u64]) -> Option { + a.iter() + .zip(b.iter()) + .position(|(x, y)| x != y) + .and_then(|i| u32::try_from(i).ok()) + } + + /// Whether two trials agree over their whole common prefix, and that prefix + /// is non-empty. + /// + /// Distinct from `first_divergence(..).is_none()`, which is also true for + /// two empty trials — a case that means "nothing ran", not "they agree". + #[must_use] + pub fn agree(a: &[u64], b: &[u64]) -> bool { + let common = a.len().min(b.len()); + common > 0 && Self::first_divergence(a, b).is_none() + } +} + +/// Reduce the emulator's current state to one comparable value. +fn sample(nes: &mut Nes, observable: Observable, audio: &mut Vec) -> u64 { + match observable { + Observable::Framebuffer => fnv1a64(nes.framebuffer()), + Observable::IndexFramebuffer => { + // The index framebuffer is `u16` per pixel; fold it through the same + // byte hash so every variant shares one mixing function. + let mut h = FNV_OFFSET; + for px in nes.index_framebuffer() { + h = fnv1a64_step(h, px.to_le_bytes().as_slice()); + } + h + } + Observable::Wram => fnv1a64(nes.wram()), + Observable::AudioEnergy => { + audio.clear(); + audio.extend_from_slice(&nes.drain_audio()); + // Quantised sum of |amplitude|. Exact float equality across a + // resampled stream would compare noise; this asks the coarser + // question the fallback is for — "did this frame make a + // meaningfully different sound?". + let energy: f32 = audio.iter().map(|s| s.abs()).sum(); + #[allow(clippy::cast_possible_truncation, clippy::cast_sign_loss)] + let q = (energy * 64.0) as u64; + q + } + } +} + +const FNV_OFFSET: u64 = 0xcbf2_9ce4_8422_2325; +const FNV_PRIME: u64 = 0x0000_0100_0000_01b3; + +fn fnv1a64_step(mut hash: u64, bytes: &[u8]) -> u64 { + for &b in bytes { + hash ^= u64::from(b); + hash = hash.wrapping_mul(FNV_PRIME); + } + hash +} + +fn fnv1a64(bytes: &[u8]) -> u64 { + fnv1a64_step(FNV_OFFSET, bytes) +} + +#[cfg(test)] +mod tests { + use super::*; + + /// A minimal NROM that spins forever, mirroring the core's `synth_nrom` + /// fixture. Enough to exercise the engine's contract without a real game. + fn synth_nrom() -> Vec { + let mut bytes = Vec::new(); + bytes.extend_from_slice(b"NES\x1A"); + bytes.push(1); // 16 KiB PRG + bytes.push(1); // 8 KiB CHR + bytes.push(0); + bytes.push(0); + bytes.extend_from_slice(&[0u8; 8]); + let mut prg = vec![0u8; 16 * 1024]; + prg[0] = 0x4C; // JMP $C000 + prg[1] = 0x00; + prg[2] = 0xC0; + let len = prg.len(); + prg[len - 6] = 0x00; // NMI + prg[len - 5] = 0xC0; + prg[len - 4] = 0x00; // RESET + prg[len - 3] = 0xC0; + prg[len - 2] = 0x00; // IRQ + prg[len - 1] = 0xC0; + bytes.extend_from_slice(&prg); + bytes.extend_from_slice(&vec![0u8; 8 * 1024]); + bytes + } + + fn nes() -> Nes { + Nes::from_rom(&synth_nrom()).expect("fixture parses") + } + + fn idle(_: u32) -> (Buttons, Buttons) { + (Buttons::empty(), Buttons::empty()) + } + + /// THE contract the whole engine rests on: two trials with identical inputs + /// produce identical samples, frame for frame. If this fails, nothing else + /// here means anything. + #[test] + fn identical_trials_are_identical() { + let mut n = nes(); + for _ in 0..10 { + n.run_frame(); + } + let probe = Probe::anchor(&n, Budget::default()); + + let a = probe.run(&mut n, 20, Observable::Framebuffer, idle); + let b = probe.run(&mut n, 20, Observable::Framebuffer, idle); + assert_eq!(a, b, "the determinism contract failed under replay"); + assert_eq!(Probe::first_divergence(&a, &b), None); + assert!(Probe::agree(&a, &b)); + } + + /// The anchor must actually restore: a trial run after another trial has + /// advanced the emulator has to produce the same samples as the first. + /// Without the restore, trial 2 would start 20 frames later and differ. + #[test] + fn each_trial_restarts_from_the_anchor() { + let mut n = nes(); + let probe = Probe::anchor(&n, Budget::default()); + let first = probe.run(&mut n, 8, Observable::Wram, idle); + // Advance well past the anchor between trials. + for _ in 0..50 { + n.run_frame(); + } + let second = probe.run(&mut n, 8, Observable::Wram, idle); + assert_eq!(first, second, "the anchor did not restore between trials"); + } + + /// All four observables must be usable and self-consistent. + #[test] + fn every_observable_is_deterministic() { + let mut n = nes(); + let probe = Probe::anchor(&n, Budget::default()); + for obs in [ + Observable::Framebuffer, + Observable::IndexFramebuffer, + Observable::Wram, + Observable::AudioEnergy, + ] { + let a = probe.run(&mut n, 6, obs, idle); + let b = probe.run(&mut n, 6, obs, idle); + assert_eq!(a, b, "{obs:?} was not deterministic under replay"); + assert_eq!(a.len(), 6, "{obs:?} produced the wrong sample count"); + } + } + + /// The budget must cap a trial, and the short result must be distinguishable + /// from a completed one — the caller has to be able to tell "inconclusive" + /// from "no divergence". + #[test] + fn budget_caps_the_trial_length() { + let mut n = nes(); + let budget = Budget { + max_frames_per_trial: 3, + ..Budget::default() + }; + let probe = Probe::anchor(&n, budget); + let samples = probe.run(&mut n, 100, Observable::Framebuffer, idle); + assert_eq!(samples.len(), 3, "budget did not cap the trial"); + } + + /// `run_counted` must stop handing out trials once the budget is spent, + /// so a search loop terminates on the budget rather than on discipline. + #[test] + fn trial_budget_is_enforced_and_then_refuses() { + let mut n = nes(); + let budget = Budget { + max_trials: 2, + ..Budget::default() + }; + let mut probe = Probe::anchor(&n, budget); + assert_eq!(probe.trials_remaining(), 2); + assert!( + probe + .run_counted(&mut n, 2, Observable::Wram, idle) + .is_some() + ); + assert!( + probe + .run_counted(&mut n, 2, Observable::Wram, idle) + .is_some() + ); + assert_eq!(probe.trials_remaining(), 0); + assert!( + probe + .run_counted(&mut n, 2, Observable::Wram, idle) + .is_none(), + "the engine handed out a trial past its budget" + ); + } + + /// A divergence must be located at the exact frame it first appears, not + /// merely detected. The fixture is synthetic so the answer is known: two + /// sample streams that agree for three entries and then differ. + #[test] + fn first_divergence_reports_the_exact_frame() { + let a = [1u64, 2, 3, 4, 5]; + let b = [1u64, 2, 3, 9, 5]; + assert_eq!(Probe::first_divergence(&a, &b), Some(3)); + assert!(!Probe::agree(&a, &b)); + } + + /// Two trials of different length agree if their common prefix does — a + /// budget-truncated trial is not a divergence. + #[test] + fn a_shorter_trial_is_not_a_divergence() { + let long = [1u64, 2, 3, 4]; + let short = [1u64, 2]; + assert_eq!(Probe::first_divergence(&long, &short), None); + assert!(Probe::agree(&long, &short)); + } + + /// Two empty trials must NOT read as agreement: nothing ran, so there is + /// nothing to agree about. This is the distinction that stops a probe + /// reporting "no reaction" when it in fact never simulated anything. + #[test] + fn empty_trials_do_not_count_as_agreement() { + assert_eq!(Probe::first_divergence(&[], &[]), None); + assert!( + !Probe::agree(&[], &[]), + "empty trials must not report agreement" + ); + } + + /// Input must actually reach the emulator: a trial that presses buttons and + /// one that does not must differ in WRAM on a ROM that stores the pad. + /// + /// The spin-loop fixture never reads the controller, so this asserts the + /// weaker but still meaningful property that the input closure is invoked + /// once per frame with ascending indices — without which two "different" + /// trials would be silently identical and every probe would answer "no + /// reaction". + #[test] + fn the_input_closure_is_called_once_per_frame_in_order() { + let mut n = nes(); + let probe = Probe::anchor(&n, Budget::default()); + let mut seen = Vec::new(); + let _ = probe.run(&mut n, 5, Observable::Wram, |f| { + seen.push(f); + (Buttons::empty(), Buttons::empty()) + }); + assert_eq!(seen, vec![0, 1, 2, 3, 4]); + } + + /// A real state difference must propagate through the replay into a detected + /// divergence. + /// + /// The tests above prove the comparator and the replay path separately; this + /// closes the loop. Two anchors differing only in one work-RAM byte must + /// produce sample streams that diverge at frame 0 under the `Wram` + /// observable — which is the mechanism every consumer of this crate relies + /// on, and the one a comparator test alone cannot demonstrate. + /// + /// The fixture ROM never reads the controller, so perturbing memory is the + /// available way to introduce a genuine difference; the Latency Oracle will + /// introduce its difference through input instead, on ROMs that do read it. + #[test] + fn a_real_state_difference_is_detected_end_to_end() { + let mut n = nes(); + for _ in 0..10 { + n.run_frame(); + } + + n.poke_ram(0x0200, 0x00); + let probe_a = Probe::anchor(&n, Budget::default()); + let a = probe_a.run(&mut n, 4, Observable::Wram, idle); + + // Restore to the same point, change ONE byte, and re-anchor. + probe_a.run(&mut n, 0, Observable::Wram, idle); // restore only + n.poke_ram(0x0200, 0xA5); + let probe_b = Probe::anchor(&n, Budget::default()); + let b = probe_b.run(&mut n, 4, Observable::Wram, idle); + + assert_eq!( + Probe::first_divergence(&a, &b), + Some(0), + "a one-byte work-RAM difference did not reach the observable" + ); + assert!(!Probe::agree(&a, &b)); + } + + /// Replaying an anchor into an emulator running a different ROM must fail + /// loudly. A snapshot restored across ROMs would produce a confident, wrong + /// answer, which is worse than no answer for a tool people act on. + #[test] + #[should_panic(expected = "different ROM")] + fn replaying_into_a_different_rom_panics() { + let n = nes(); + let probe = Probe::anchor(&n, Budget::default()); + + // A ROM with different PRG contents => a different hash tag. + let mut other_bytes = synth_nrom(); + let prg_start = 16; + other_bytes[prg_start + 8] = 0xEA; // NOP somewhere harmless + let mut other = Nes::from_rom(&other_bytes).expect("fixture parses"); + let _ = probe.run(&mut other, 1, Observable::Wram, idle); + } +} From 373d6013e08b07fd7fe1bacc25f42855fc1b5b56 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Sun, 16 Aug 2026 17:21:05 -0400 Subject: [PATCH 03/10] =?UTF-8?q?perf(apu):=20cache=20the=20C1=20fast-path?= =?UTF-8?q?=20gain=20predicate=20(D3)=20=E2=80=94=20MEASUREMENT=20PENDING?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit v2.3.5's C1 fast path tests `mask == CHANNEL_MASK_ALL && channel_gain == CHANNEL_GAIN_UNITY` once per CPU cycle — a 6-wide `f32` array comparison evaluated 1.789 million times a second to answer a question that can only change when a user drags a mixer slider. `gain_is_unity` caches it, reducing the per-cycle test to a `u8` compare plus a `bool` load. Byte-identical BY CONSTRUCTION, not by measurement: the cached value is the same predicate over the same array, so the branch taken is unchanged. No save-state impact either — `channel_gain` is a UI playback overlay and is not in the APU snapshot, so neither is anything derived from it. `the_cached_gain_predicate_cannot_desync` pins every write path, including the one a naive implementation gets wrong: the setter CLAMPS, so a caller asking for 3.0 stores 2.0, and the cache must be computed from the stored value rather than the requested one. It also pins that `reset` (which does not touch the gain overlay) leaves the two consistent. NOT YET MEASURED. The project's bar for adopting a performance change is >3% on a same-runner A/B plus byte-identical output, and the host is currently contended (load 2.29) — `docs/performance.md` already carries one retracted subsection whose numbers were taken during a concurrent build, so a number taken now would be worth less than none. The A/B against the pre-D3 baseline is owed before this is described as a win anywhere user-facing; if it does not clear the bar it stays as a simplification and is recorded as a rejection with its number, per the convention F19 and the C1 arms follow. Co-Authored-By: Claude Opus 5 --- crates/rustynes-apu/src/apu.rs | 65 +++++++++++++++++++++++++++++++++- 1 file changed, 64 insertions(+), 1 deletion(-) diff --git a/crates/rustynes-apu/src/apu.rs b/crates/rustynes-apu/src/apu.rs index 0b7b0b22..bde295ce 100644 --- a/crates/rustynes-apu/src/apu.rs +++ b/crates/rustynes-apu/src/apu.rs @@ -292,6 +292,19 @@ pub struct Apu { /// the oracle / test ROMs (which never touch a gain) are unaffected. NEVER /// serialized into the save state (a UI preference, like the mask / volume). pub(crate) channel_gain: [f32; 6], + /// v2.3.6 D3 — cached `channel_gain == CHANNEL_GAIN_UNITY`. + /// + /// The C1 fast-path predicate compared a `[f32; 6]` array **1.789 million + /// times a second** to answer a question that can only change in + /// [`Apu::set_channel_gain`], which a user reaches through a mixer slider. + /// Caching it turns the per-cycle test into a `u8` compare plus a `bool` + /// load. + /// + /// Not serialized, and correctly so: `channel_gain` is a UI playback overlay + /// rather than NES hardware state, so it is not in the APU snapshot either. + /// This field is derived from it and must be recomputed wherever it is + /// written — `new`, `reset`, and `set_channel_gain`. + pub(crate) gain_is_unity: bool, /// v2.1.6 "Expansion Audio" — the most recent RAW external / on-cart /// expansion-audio sample fed into [`Self::tick_with_external`] (BEFORE the /// UI [`Self::channel_gain`] `[5]` re-weight), retained purely so the @@ -382,6 +395,7 @@ impl Apu { last_frame_events: FrameEvents::default(), channel_mask: CHANNEL_MASK_ALL, channel_gain: CHANNEL_GAIN_UNITY, + gain_is_unity: true, last_external: 0.0, } } @@ -463,6 +477,10 @@ impl Apu { for (slot, g) in self.channel_gain.iter_mut().zip(gain.iter()) { *slot = g.clamp(0.0, 2.0); } + // v2.3.6 D3 — refresh the cached predicate the per-cycle fast path + // reads. Recomputed from the CLAMPED values, so a caller passing 3.0 + // (clamped to 2.0) cannot leave the cache claiming unity. + self.gain_is_unity = self.channel_gain == CHANNEL_GAIN_UNITY; } /// v2.1.3 — select the analog output-filter model (see @@ -1077,7 +1095,10 @@ impl Apu { // it would have received, so the output is byte-identical by // construction rather than by measurement. `apu_default_mix_matches_the_gated_path` // pins that across a 2,048-point sweep anyway. - if mask == CHANNEL_MASK_ALL && self.channel_gain == CHANNEL_GAIN_UNITY { + // v2.3.6 D3 — `gain_is_unity` is the cached form of + // `channel_gain == CHANNEL_GAIN_UNITY`; see the field. Same predicate, + // without a 6-wide `f32` array compare per CPU cycle. + if mask == CHANNEL_MASK_ALL && self.gain_is_unity { self.last_external = external; let mixed = self.mixer.mix( self.pulse1.output(), @@ -1928,6 +1949,48 @@ mod tests { "a non-unity gain must change the emitted audio" ); } + + /// v2.3.6 D3 — the cached `gain_is_unity` must never disagree with the array + /// it summarises. + /// + /// The cache is what the per-cycle fast path reads, so a stale `true` would + /// silently apply unity gain while the user's mixer said otherwise — a + /// wrong-output bug with no assertion anywhere else to catch it. Every write + /// path to `channel_gain` is exercised, including the clamp: a caller asking + /// for 3.0 gets 2.0, which is NOT unity, and the cache must say so. + #[test] + fn the_cached_gain_predicate_cannot_desync() { + let mut apu = Apu::new(Region::Ntsc, 48_000); + assert!(apu.gain_is_unity, "a fresh APU is at unity gain"); + assert_eq!(apu.channel_gain, CHANNEL_GAIN_UNITY); + + apu.set_channel_gain([0.5, 1.0, 1.0, 1.0, 1.0, 1.0]); + assert!(!apu.gain_is_unity, "cache missed a non-unity gain"); + assert_eq!(apu.gain_is_unity, apu.channel_gain == CHANNEL_GAIN_UNITY); + + // Back to unity: the cache must recover, not latch. + apu.set_channel_gain(CHANNEL_GAIN_UNITY); + assert!(apu.gain_is_unity, "cache latched non-unity"); + + // Clamped input: 3.0 becomes 2.0, which is not unity. + apu.set_channel_gain([3.0, 1.0, 1.0, 1.0, 1.0, 1.0]); + assert_eq!(apu.channel_gain[0], 2.0, "premise: the setter clamps"); + assert!( + !apu.gain_is_unity, + "cache computed from the pre-clamp value, not the stored one" + ); + assert_eq!(apu.gain_is_unity, apu.channel_gain == CHANNEL_GAIN_UNITY); + + // `reset` does not touch the gain overlay, so the cache must survive it. + apu.set_channel_gain([0.25; 6]); + apu.reset(); + assert_eq!( + apu.gain_is_unity, + apu.channel_gain == CHANNEL_GAIN_UNITY, + "reset desynced the cache from the array" + ); + } + use super::*; #[test] From c8334b53b1c0f16d049cdbf332a8e2296e3bd141 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Sun, 16 Aug 2026 17:29:55 -0400 Subject: [PATCH 04/10] feat(probe): the Latency Oracle measurement, headless MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The algorithm half of the Latency Oracle: replay one anchor twice — once with a button held, once with nothing pressed — and the first frame at which the runs differ IS the game's internal input lag, because it is the first frame on which the press could have changed anything. It lives here rather than in the frontend so it is testable without a window; only the panel needs winit. Every emulator makes this a manual ritual (hold a direction, frame-advance until the sprite moves, subtract one — the procedure RetroArch documents). RustyNES's own settings panel says "1 fits most games". Nothing measured it. The measurement is easy; being honest about it is the work, because the number is ACTED ON — it sets run-ahead depth, which is linear in the core's frame cost (~34%/52%/78% of the NTSC budget at depth 0/1/2). So: - Six buttons are probed and must AGREE. A plurality is not agreement: two buttons saying 1 and two saying 4 is a game doing something this probe does not understand, and the honest output is no number. - `frames: None` and `frames: Some(0)` are different answers and are never collapsed. `Some(0)` means the game reacted immediately; `None` means the probe could not tell. Conflating them is how a latency tool starts lying. - `suggested_run_ahead` returns `None` when inconclusive: leave the user's setting alone rather than guess. - A divergence past `max_plausible_lag` is discarded — at that distance it is far likelier to be the game's own animation than a reaction to the pad. - Observables fall back framebuffer -> audio -> work RAM, because a reaction can be audible or internal before it is visible. Two fixture findings worth keeping, both of which first presented as the algorithm being wrong: 1. A continuously-polling ROM measured 3-3 split between frames 0 and 5 — end-of-frame sampling caught its eight-bit shift loop at different points, so WHICH BIT POSITION a button occupies leaked into the answer. That is an artefact of a ROM no real game resembles; the fixture now latches once per frame in NMI, as real games do. The algorithm was right to call the split inconclusive. 2. The NMI fixture then measured nothing at all, because the PPU IGNORES `$2000` writes for its first ~29,658 CPU cycles and the handler enabled NMI once, inside that window. The main loop now re-asserts it. A fixture that silently tests nothing is worse than a failing one. Seventeen tests across the crate. The load-bearing pair: a ROM that never reads the controller must report INCONCLUSIVE rather than zero-lag, and a ROM that does read it must produce a measurement — without the second, the first would also pass on a probe that always answers "I don't know". Co-Authored-By: Claude Opus 5 --- crates/rustynes-probe/src/latency.rs | 460 +++++++++++++++++++++++++++ crates/rustynes-probe/src/lib.rs | 2 + 2 files changed, 462 insertions(+) create mode 100644 crates/rustynes-probe/src/latency.rs diff --git a/crates/rustynes-probe/src/latency.rs b/crates/rustynes-probe/src/latency.rs new file mode 100644 index 00000000..6e656bf9 --- /dev/null +++ b/crates/rustynes-probe/src/latency.rs @@ -0,0 +1,460 @@ +//! Measuring a game's **own** input lag. +//! +//! Most NES titles sample the controller in their NMI handler and act on it one +//! or more frames later. That delay is real on hardware, and run-ahead removes +//! it by simulating those frames in advance — but only if you tell it how many. +//! +//! Every emulator makes finding that number a manual ritual: hold a direction, +//! frame-advance until the sprite moves, subtract one. `RetroArch` documents +//! exactly that procedure; `RustyNES`'s own settings panel says only "1 fits most +//! games". Nothing measures it. +//! +//! This does, by asking the question directly: replay the same anchor twice, +//! once with a button held and once with nothing pressed, and find the first +//! frame at which the two runs differ. That index **is** the game's internal +//! lag, because it is the first frame on which pressing the button could have +//! changed anything. +//! +//! # Being honest is the hard part +//! +//! A latency number is acted on — it sets run-ahead depth, which costs real +//! frame budget. So this module is built to **decline** rather than guess: +//! +//! - It probes several buttons, because a given game may ignore most of them, +//! and requires them to *agree* before reporting a value. +//! - It falls back across observables, because a reaction may be audible or +//! internal before it is visible — a menu that commits a highlight to a +//! variable one frame before drawing it would otherwise read as slower than +//! it is. +//! - It reports [`Confidence`] alongside the number, and returns +//! [`LatencyReport::frames`] as `None` whenever the trials disagree or nothing +//! reacted inside the budget. +//! +//! "I could not tell" is a valid, useful answer. A wrong depth silently spends +//! frame budget the host may not have. + +use rustynes_core::{Buttons, Nes}; + +use crate::{Budget, Observable, Probe}; + +/// Buttons worth probing, in the order tried. +/// +/// Directions first: they are what most games act on soonest and what a player +/// is holding when latency matters. `A`/`B` next, since action games respond to +/// them. `START` last and deliberately — it pauses many games, which is a +/// reaction, but a reaction to a *menu*, not to gameplay input, and treating a +/// pause as gameplay latency would over-report. +const PROBE_BUTTONS: [Buttons; 6] = [ + Buttons::RIGHT, + Buttons::LEFT, + Buttons::DOWN, + Buttons::UP, + Buttons::A, + Buttons::B, +]; + +/// Observables tried in order until one produces agreeing answers. +/// +/// Framebuffer first — a visible reaction is what a player perceives as latency. +/// Audio next: a sound effect often fires the same frame the input is accepted, +/// before anything is drawn. Work RAM last, because it detects a reaction the +/// player cannot yet perceive; useful as evidence the game read the pad at all, +/// but the least representative of *felt* latency. +const OBSERVABLE_ORDER: [Observable; 3] = [ + Observable::Framebuffer, + Observable::AudioEnergy, + Observable::Wram, +]; + +/// How much to trust a measurement. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum Confidence { + /// Every button that reacted agreed on the same frame. + Unanimous, + /// A majority agreed; at least one reacting button disagreed. Usable, but a + /// caller applying it automatically should say it is approximate. + Majority, + /// No value: nothing reacted inside the budget, or the reacting buttons did + /// not agree closely enough to pick one. + Inconclusive, +} + +/// The result of a measurement. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct LatencyReport { + /// Measured internal lag in frames, or `None` when inconclusive. + /// + /// `None` and `Some(0)` are **different answers** and must not be collapsed: + /// `Some(0)` means the game reacted on the very next frame, `None` means the + /// probe could not tell. Reporting the second as the first is how a latency + /// tool starts lying. + pub frames: Option, + /// How much to trust [`Self::frames`]. + pub confidence: Confidence, + /// Buttons that produced any divergence at all. + pub reacting_buttons: u32, + /// Buttons probed. + pub probed_buttons: u32, + /// Which observable produced the answer, when there is one. + pub observable: Option, + /// Per-button first-divergence frames, in `PROBE_BUTTONS` order (a plain + /// code span: that item is private and `rustdoc::private_intra_doc_links` is + /// denied), for a UI + /// that wants to show the evidence rather than just the conclusion. + pub per_button: Vec>, +} + +impl LatencyReport { + /// An inconclusive report, with the evidence that produced it. + fn inconclusive(per_button: Vec>, probed: u32) -> Self { + let reacting = + u32::try_from(per_button.iter().filter(|d| d.is_some()).count()).unwrap_or(u32::MAX); + Self { + frames: None, + confidence: Confidence::Inconclusive, + reacting_buttons: reacting, + probed_buttons: probed, + observable: None, + per_button, + } + } + + /// A run-ahead depth this measurement supports, clamped to `max_depth`. + /// + /// Returns `None` for an inconclusive report: **leave the user's setting + /// alone rather than guess**. Run-ahead depth is linear in the core's frame + /// cost (roughly 34% / 52% / 78% of the NTSC budget at depth 0 / 1 / 2), so + /// applying a fabricated depth spends real budget for nothing. + #[must_use] + pub fn suggested_run_ahead(&self, max_depth: u32) -> Option { + self.frames.map(|f| f.min(max_depth)) + } +} + +/// How a measurement is run. +#[derive(Clone, Copy, Debug)] +pub struct LatencyConfig { + /// Frames to simulate per trial. A game that has not reacted within this + /// many frames is reported as inconclusive rather than as zero-lag. + pub frames_per_trial: u32, + /// Largest lag treated as a real measurement. A divergence beyond this is + /// far more likely to be the game's own animation or a timer than a reaction + /// to input, so it is discarded rather than reported. + pub max_plausible_lag: u32, +} + +impl Default for LatencyConfig { + fn default() -> Self { + Self { + frames_per_trial: 20, + // Games buffer input by a frame or three. Ten is generous; past it, + // "the screen changed" almost certainly means something else moved. + max_plausible_lag: 10, + } + } +} + +/// Measure the game's internal input lag from the emulator's current state. +/// +/// `nes` is a scratch instance the probe replays into — it is rewound to the +/// anchor repeatedly and left wherever the last trial ended, so do not pass the +/// live emulator. +/// +/// Returns as soon as an observable yields agreeing answers, so the common case +/// costs one observable's worth of trials rather than all three. +pub fn measure(nes: &mut Nes, anchor: &Nes, cfg: LatencyConfig) -> LatencyReport { + let budget = Budget { + max_frames_per_trial: cfg.frames_per_trial, + // Two trials per button per observable, plus headroom. + max_trials: u32::try_from((PROBE_BUTTONS.len() + 1) * 2 * OBSERVABLE_ORDER.len()) + .unwrap_or(u32::MAX), + }; + let probe = Probe::anchor(anchor, budget); + let probed = u32::try_from(PROBE_BUTTONS.len()).unwrap_or(u32::MAX); + let mut last_evidence = vec![None; PROBE_BUTTONS.len()]; + + for observable in OBSERVABLE_ORDER { + // The idle baseline is the same for every button under one observable, + // so it is run once rather than per button. + let idle = probe.run(nes, cfg.frames_per_trial, observable, |_| { + (Buttons::empty(), Buttons::empty()) + }); + + let mut per_button = Vec::with_capacity(PROBE_BUTTONS.len()); + for button in PROBE_BUTTONS { + let held = probe.run(nes, cfg.frames_per_trial, observable, move |_| { + (button, Buttons::empty()) + }); + let d = Probe::first_divergence(&held, &idle).filter(|f| *f <= cfg.max_plausible_lag); + per_button.push(d); + } + + if let Some(report) = conclude(&per_button, probed, observable) { + return report; + } + last_evidence = per_button; + } + + LatencyReport::inconclusive(last_evidence, probed) +} + +/// Turn per-button divergences into a verdict, or `None` if this observable +/// cannot support one. +fn conclude( + per_button: &[Option], + probed: u32, + observable: Observable, +) -> Option { + let reacting: Vec = per_button.iter().filter_map(|d| *d).collect(); + if reacting.is_empty() { + return None; + } + + // Pick the most common answer. Ties resolve to the SMALLEST frame, which is + // the conservative direction: under-reporting lag sets a lower run-ahead + // depth, which costs the user less frame budget than over-reporting. + let mut best = (0usize, u32::MAX); + for &candidate in &reacting { + let votes = reacting.iter().filter(|f| **f == candidate).count(); + if votes > best.0 || (votes == best.0 && candidate < best.1) { + best = (votes, candidate); + } + } + let (votes, frames) = best; + + let confidence = if votes == reacting.len() { + Confidence::Unanimous + } else if votes * 2 > reacting.len() { + Confidence::Majority + } else { + // A plurality is not agreement. Two buttons saying "1" and two saying + // "4" is a game doing something this probe does not understand, and the + // honest output is no number at all. + return None; + }; + + Some(LatencyReport { + frames: Some(frames), + confidence, + reacting_buttons: u32::try_from(reacting.len()).unwrap_or(u32::MAX), + probed_buttons: probed, + observable: Some(observable), + per_button: per_button.to_vec(), + }) +} + +#[cfg(test)] +mod tests { + use super::*; + + /// An NROM that never reads the controller — the honesty fixture. + fn spin_rom() -> Vec { + let mut prg = vec![0u8; 16 * 1024]; + prg[0] = 0x4C; // JMP $C000 + prg[1] = 0x00; + prg[2] = 0xC0; + wrap_nrom(prg) + } + + /// An NROM that latches the controller **once per frame in its NMI handler** + /// — the structure essentially every real NES game uses — and shifts the + /// eight bits into `$0300`. The main loop does nothing. + /// + /// Frame quantisation is the point. An earlier version of this fixture + /// polled `$4016` continuously from the main loop, and the measurement came + /// back split 3-3 between frames 0 and 5: sampling at end-of-frame caught + /// that loop at a different point in its eight-bit shift for buttons read + /// early (`A`, `B`) versus late (`Right`, `Left`, `Down`), so *which bit + /// position a button occupies* leaked into the answer. That is an artefact of + /// a ROM no real game resembles, not a property of the measurement — and the + /// split correctly produced "inconclusive", which is the algorithm behaving + /// as designed on nonsense input. + fn polling_rom() -> Vec { + let mut prg = vec![0u8; 16 * 1024]; + // $C000: enable NMI, forever. + // + // Re-asserted in the loop rather than written once, because the PPU + // IGNORES `$2000` writes for roughly its first 29,658 CPU cycles after + // reset. A single write at reset lands inside that window, is discarded, + // and NMI never fires — which presented here as every button reporting + // "no reaction", i.e. a fixture that silently tested nothing. + let reset: &[u8] = &[ + 0xA9, 0x80, // LDA #$80 + 0x8D, 0x00, 0x20, // STA $2000 (NMI on VBlank) + 0x4C, 0x00, 0xC0, // JMP $C000 + ]; + prg[..reset.len()].copy_from_slice(reset); + + // $C020: the NMI handler — strobe, read 8 bits into $0300, return. + let nmi: &[u8] = &[ + 0xA9, 0x01, // LDA #$01 + 0x8D, 0x16, 0x40, // STA $4016 (strobe on) + 0xA9, 0x00, // LDA #$00 + 0x8D, 0x16, 0x40, // STA $4016 (strobe off -> latch) + 0xA2, 0x08, // LDX #$08 + 0xA9, 0x00, // LDA #$00 + 0x8D, 0x00, 0x03, // STA $0300 + // read loop @ $C02F + 0xAD, 0x16, 0x40, // LDA $4016 + 0x4A, // LSR A (bit 0 -> carry) + 0x2E, 0x00, 0x03, // ROL $0300 + 0xCA, // DEX + 0xD0, 0xF6, // BNE -10 -> back to LDA $4016 + 0x40, // RTI + ]; + prg[0x20..0x20 + nmi.len()].copy_from_slice(nmi); + wrap_nrom_with_nmi(prg, 0xC020) + } + + fn wrap_nrom(prg: Vec) -> Vec { + wrap_nrom_with_nmi(prg, 0xC000) + } + + fn wrap_nrom_with_nmi(prg: Vec, nmi_addr: u16) -> Vec { + let mut prg = prg; + let len = prg.len(); + prg[len - 6] = (nmi_addr & 0xFF) as u8; // NMI + prg[len - 5] = (nmi_addr >> 8) as u8; + prg[len - 4] = 0x00; // RESET-> $C000 + prg[len - 3] = 0xC0; + prg[len - 2] = 0x00; // IRQ -> $C000 + prg[len - 1] = 0xC0; + let mut bytes = Vec::new(); + bytes.extend_from_slice(b"NES\x1A"); + bytes.push(1); + bytes.push(1); + bytes.push(0); + bytes.push(0); + bytes.extend_from_slice(&[0u8; 8]); + bytes.extend_from_slice(&prg); + bytes.extend_from_slice(&vec![0u8; 8 * 1024]); + bytes + } + + fn warmed(rom: &[u8], frames: u32) -> Nes { + let mut nes = Nes::from_rom(rom).expect("fixture parses"); + for _ in 0..frames { + nes.run_frame(); + } + nes + } + + /// THE honesty property. A ROM that never reads the controller must be + /// reported as **inconclusive**, never as zero-lag. + /// + /// `Some(0)` would be acted on: it sets run-ahead to 0 and tells the user the + /// game has no internal lag, which is a claim the probe has no evidence for. + /// A latency tool that cannot say "I don't know" is worse than none. + #[test] + fn a_game_that_ignores_input_is_inconclusive_not_zero() { + let rom = spin_rom(); + let anchor = warmed(&rom, 20); + let mut scratch = Nes::from_rom(&rom).expect("fixture parses"); + + let report = measure(&mut scratch, &anchor, LatencyConfig::default()); + assert_eq!( + report.frames, None, + "reported a lag it could not have measured" + ); + assert_eq!(report.confidence, Confidence::Inconclusive); + assert_eq!(report.reacting_buttons, 0); + assert_eq!( + report.probed_buttons, + u32::try_from(PROBE_BUTTONS.len()).unwrap_or(u32::MAX) + ); + assert_eq!( + report.suggested_run_ahead(3), + None, + "an inconclusive report must not move the user's run-ahead setting" + ); + } + + /// The complement: a ROM that DOES read the pad must produce a measurement. + /// Without this, the honesty test above would also pass on a probe that + /// always answers "inconclusive". + #[test] + fn a_polling_game_is_measured() { + let rom = polling_rom(); + let anchor = warmed(&rom, 20); + let mut scratch = Nes::from_rom(&rom).expect("fixture parses"); + + let report = measure(&mut scratch, &anchor, LatencyConfig::default()); + assert!( + report.frames.is_some(), + "a continuously-polling ROM produced no measurement: {report:?}" + ); + assert!(report.reacting_buttons > 0); + assert_ne!(report.confidence, Confidence::Inconclusive); + assert!(report.observable.is_some()); + } + + /// A divergence past `max_plausible_lag` is discarded: at that distance it is + /// far likelier to be the game's own animation than a reaction to the pad. + #[test] + fn an_implausibly_late_divergence_is_not_a_measurement() { + let per_button = [Some(40), Some(40), None, None, None, None]; + // `measure` filters before `conclude` sees them, so model that here. + let filtered: Vec> = per_button + .iter() + .map(|d| d.filter(|f| *f <= LatencyConfig::default().max_plausible_lag)) + .collect(); + assert!(conclude(&filtered, 6, Observable::Framebuffer).is_none()); + } + + /// Unanimity, majority and a bare plurality must be told apart — a plurality + /// is not agreement, and must yield no number. + #[test] + fn agreement_is_graded_and_a_plurality_is_not_agreement() { + let unanimous = [Some(2), Some(2), None, None, None, None]; + let r = conclude(&unanimous, 6, Observable::Framebuffer).expect("a verdict"); + assert_eq!(r.frames, Some(2)); + assert_eq!(r.confidence, Confidence::Unanimous); + + let majority = [Some(1), Some(1), Some(4), None, None, None]; + let r = conclude(&majority, 6, Observable::Framebuffer).expect("a verdict"); + assert_eq!(r.frames, Some(1)); + assert_eq!(r.confidence, Confidence::Majority); + + // Two against two: no majority, so no number. + let split = [Some(1), Some(1), Some(4), Some(4), None, None]; + assert!( + conclude(&split, 6, Observable::Framebuffer).is_none(), + "a plurality was reported as a measurement" + ); + } + + /// An even split is inconclusive at every size, not just at four buttons. + /// + /// One-vs-one is the case that looks most like "nearly agreed" and is the + /// easiest to talk oneself into reporting. It carries exactly as much + /// evidence as two-vs-two: none. The smallest-value tie-break inside + /// `conclude` exists only to make candidate selection deterministic — it can + /// never decide a *reported* number, because equal top votes and a majority + /// are mutually exclusive. + #[test] + fn an_even_split_is_inconclusive_at_any_size() { + let one_v_one = [Some(3), Some(1), None, None, None, None]; + assert!( + conclude(&one_v_one, 6, Observable::Framebuffer).is_none(), + "a 1-1 split was reported as a measurement" + ); + let two_v_two = [Some(3), Some(3), Some(1), Some(1), None, None]; + assert!(conclude(&two_v_two, 6, Observable::Framebuffer).is_none()); + } + + /// The suggested depth is clamped, so a measurement cannot ask for more + /// run-ahead than the caller is willing to afford. + #[test] + fn suggested_run_ahead_is_clamped() { + let r = LatencyReport { + frames: Some(7), + confidence: Confidence::Unanimous, + reacting_buttons: 6, + probed_buttons: 6, + observable: Some(Observable::Framebuffer), + per_button: vec![Some(7); 6], + }; + assert_eq!(r.suggested_run_ahead(3), Some(3)); + assert_eq!(r.suggested_run_ahead(0), Some(0)); + } +} diff --git a/crates/rustynes-probe/src/lib.rs b/crates/rustynes-probe/src/lib.rs index d17dc09e..ce5addc6 100644 --- a/crates/rustynes-probe/src/lib.rs +++ b/crates/rustynes-probe/src/lib.rs @@ -57,6 +57,8 @@ //! # } //! ``` +pub mod latency; + use rustynes_core::{Buttons, Nes, ROM_HASH_TAG_LEN}; /// Bounds on what a single probe may spend. From 37839171916352ac77f4f15e13742663a0608367 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Sun, 16 Aug 2026 17:32:59 -0400 Subject: [PATCH 05/10] docs(version-plan): VERSION-PLAN.md was a release behind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It still opened "Current release: v2.3.4 'Ledger'" after v2.3.5 shipped, named v2.3.4 as current in three separate places, and described the APU workstream as "carried to v2.3.5" — which by then had happened. This matters more than an ordinary stale doc: `VERSION-PLAN.md` is the more current of the two forward-planning documents (`to-dos/ROADMAP.md` stops at Phase 10 and names two different releases as current in two bullets), so it is what someone reads to find out where the project actually is. Updated the header claim, the lineage chain, the release table (v2.3.4 demoted, a v2.3.5 row added), and the forward-path paragraph. All three "(current)" markers now name the same release, which is now checkable with a grep rather than by reading three paragraphs. Recording the general form, since this is the second document found stale in the same review pass: a release cut must update this file alongside the CHANGELOG header, `docs/STATUS.md`, the README badge and `rustynes_libretro.info`. The v2.3.5 cut updated the others and missed this one. Co-Authored-By: Claude Opus 5 --- VERSION-PLAN.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/VERSION-PLAN.md b/VERSION-PLAN.md index 71fd26b7..e2a72679 100644 --- a/VERSION-PLAN.md +++ b/VERSION-PLAN.md @@ -1,6 +1,6 @@ # RustyNES Version Plan -**Current release: v2.3.4 "Ledger"** — the coverage release: three boards (mapper 176 submapper 2 WAIXING-FS005, 154 NAMCOT-3453, 243 Sachen SA-020A, breadth **172 → 174 families**), the coverage harness moved onto the frontend's real load path, and the defect that exposed — the per-game database reading a `0` Mapper column as "force NROM" and overwriting correct headers, leaving **every Sachen cartridge** unloadable since **v1.2.0**. **This release touches the emulation core**, so AccuracyCoin exactly 141/141 is **verified, not asserted by construction**. Carried to v2.3.5 unstarted: the APU at 18.7% of frame time (Workstream C, not delivered — its bench was never built). Built on **v2.3.3 "Cadence"** — the display-pacing release: the run-ahead throttle oscillation traced to a stale median (a gate counting 120 frames of a 600-sample ring), a predictive engage arm that converges a `run_ahead = 3` host in 2.8 s instead of 12.1 s, and the `wp_presentation` measurement apparatus that made the diagnosis possible. **No emulation-core changes** (AccuracyCoin exactly 141/141). Built on **v2.3.2 "Lucid"** (pixel provenance + deterministic replay attestation), **v2.3.1 "Plumb Line"** (ten measured rejections), and **v2.3.0 "Datum II"**, the capstone that **closed** the v2.2.6 → v2.3.0 line (true multi-viewport OS-window detach, the emulator-lock frame-pacing fix, a −5.1% byte-identical PPU optimization, and both forum-reported accuracy items verified already-correct) — all on the **v2.0.0 "Timebase"** MAJOR base (the one-clock / every-cycle-bus-access scheduler rewrite). **v1.0.0** was the first stable, production cut. As of **v2.2.9**, RustyNES is **GPL-3.0-or-later** — a derivative work of GPL-licensed emulators (ADR 0036); a licensing correction, **not** a SemVer break (no public-API or save-state change). `docs/STATUS.md` is the authoritative current-state record; `CHANGELOG.md` carries the full per-release history. +**Current release: v2.3.5 "Manifest"** — the declaration release: what the core says about itself. A user reported RetroArch still showing the pre-relicense MIT/Apache-2.0 terms, and it was: RetroArch reads `dist/info/` from **libretro/libretro-super**, a SEPARATE copy nothing synced, so the v2.2.9 GPL relicense never reached the file users see. Corrected to `GPLv3+` with a standing `libretro_info_audit.rs` that makes the upstream sync a **copy** rather than a re-derivation, and a licence change is now a mandatory upstream-sync trigger. Auditing the wrapper then found **five further defects, every one with correct emulation behind it** — PAL ran 20.2% fast, Reset did nothing ever, unload leaked Game Genie indices, the aspect ratio assumed square pixels, and the Zapper was unreachable — plus a **use-after-free** in the controller tables caught in review. The crate went from zero tests to eight. The APU also gained its first throughput bench and a default-configuration mix specialization (−3.3% to −4.2% on `nes_run_frame_nestest`), so **AccuracyCoin 141/141 was VERIFIED, not asserted**. Built on **v2.3.4 "Ledger"** — the coverage release: three boards (mapper 176 submapper 2 WAIXING-FS005, 154 NAMCOT-3453, 243 Sachen SA-020A, breadth **172 → 174 families**), the coverage harness moved onto the frontend's real load path, and the defect that exposed — the per-game database reading a `0` Mapper column as "force NROM" and overwriting correct headers, leaving **every Sachen cartridge** unloadable since **v1.2.0**. **This release touches the emulation core**, so AccuracyCoin exactly 141/141 is **verified, not asserted by construction**. Its Workstream C (the APU at 18.7% of frame time) was carried to v2.3.5 and delivered there. Built on **v2.3.3 "Cadence"** — the display-pacing release: the run-ahead throttle oscillation traced to a stale median (a gate counting 120 frames of a 600-sample ring), a predictive engage arm that converges a `run_ahead = 3` host in 2.8 s instead of 12.1 s, and the `wp_presentation` measurement apparatus that made the diagnosis possible. **No emulation-core changes** (AccuracyCoin exactly 141/141). Built on **v2.3.2 "Lucid"** (pixel provenance + deterministic replay attestation), **v2.3.1 "Plumb Line"** (ten measured rejections), and **v2.3.0 "Datum II"**, the capstone that **closed** the v2.2.6 → v2.3.0 line (true multi-viewport OS-window detach, the emulator-lock frame-pacing fix, a −5.1% byte-identical PPU optimization, and both forum-reported accuracy items verified already-correct) — all on the **v2.0.0 "Timebase"** MAJOR base (the one-clock / every-cycle-bus-access scheduler rewrite). **v1.0.0** was the first stable, production cut. As of **v2.2.9**, RustyNES is **GPL-3.0-or-later** — a derivative work of GPL-licensed emulators (ADR 0036); a licensing correction, **not** a SemVer break (no public-API or save-state change). `docs/STATUS.md` is the authoritative current-state record; `CHANGELOG.md` carries the full per-release history. RustyNES follows [Semantic Versioning 2.0.0](https://semver.org/). @@ -55,7 +55,7 @@ The cycle-accurate engine was integrated as the core in a sequence of documentar | **v0.9.7** | Performance pass (display-sync pacing, dedicated emu thread, audio DRC, run-ahead) | | **v1.0.0** | Production cut — engine + ported desktop UX shell + documentation synthesis | -> **Engine lineage note.** The deep technical history under `docs/` (the `v2.0` master-clock refactor, ADRs, audit logs, the long accuracy program) describes the **upstream engine lineage**. Those old "v1.x"/"v2.x" anchors are engineering history, **not** RustyNES release versions. RustyNES's own release line is v0.1.0 → v0.8.6 → (documentary v0.9.0–v0.9.7) → **v1.0.0** → the v1.1.0–v1.10.0 additive feature line → **v2.0.0 "Timebase"** (the designated MAJOR break) → the v2.0.x "Harbor" line → the v2.1.x "Fathom" accuracy line → the v2.2.x line → v2.3.0 "Datum II" → v2.3.1 "Plumb Line" → v2.3.2 "Lucid" → v2.3.3 "Cadence" → **v2.3.4 "Ledger"** (current). +> **Engine lineage note.** The deep technical history under `docs/` (the `v2.0` master-clock refactor, ADRs, audit logs, the long accuracy program) describes the **upstream engine lineage**. Those old "v1.x"/"v2.x" anchors are engineering history, **not** RustyNES release versions. RustyNES's own release line is v0.1.0 → v0.8.6 → (documentary v0.9.0–v0.9.7) → **v1.0.0** → the v1.1.0–v1.10.0 additive feature line → **v2.0.0 "Timebase"** (the designated MAJOR break) → the v2.0.x "Harbor" line → the v2.1.x "Fathom" accuracy line → the v2.2.x line → v2.3.0 "Datum II" → v2.3.1 "Plumb Line" → v2.3.2 "Lucid" → v2.3.3 "Cadence" → v2.3.4 "Ledger" → **v2.3.5 "Manifest"** (current). ### Post-1.0 release line (v1.1.0 → current) @@ -80,9 +80,10 @@ The 1.x line was **additive / off-by-default** — every release stayed byte-ide | **v2.3.1 "Plumb Line"** | Measurement apparatus made trustworthy, then used: a harness-free frame probe, per-source-file subsystem attribution (which recovers the **APU at 18.7% of frame**, invisible in the symbol profile), an adoption A/B with an A/B/A order-bias control, and a contention-aware relative gate. **Ten core hot-path candidates measured, all ten rejected** via six distinct mechanisms — **no emulation-core changes**, AccuracyCoin exactly 141/141 — see `CHANGELOG.md` `[2.3.1]` | | **v2.3.2 "Lucid"** | Pixel provenance — click any pixel for its full causal chain, down to **the CPU instruction and cycle that last wrote each byte** — plus deterministic replay attestation (`rustynes verify`). All `debug-hooks`-gated and output-only, so AccuracyCoin holds exactly 141/141 — see `CHANGELOG.md` `[2.3.2]` | | **v2.3.3 "Cadence"** | Display pacing. The run-ahead throttle oscillation attributed to a **stale median** — the gate counted 120 frames of a 600-sample ring, so a p50 at index 300 could not leave the previous depth; **6-7 transitions per 24 s → 1**, spurious releases **2 → 0**. The engage arm now predicts instead of waiting (`run_ahead = 3` converges in **2.8 s vs 12.1 s**, 5/5 paired rounds, p = 0.0312) while releasing still demands a real measurement. Compositor refresh via `wp_presentation`, divisor display-sync, and a validity gate that fails closed; dropped frames **135-254 → 1-9**. Two arms measured and **rejected** with their numbers. No emulation-core changes — see `CHANGELOG.md` `[2.3.3]` | -| **v2.3.4 "Ledger"** (current) | Mapper coverage. Three boards — **176 submapper 2** (WAIXING-FS005), **154** (NAMCOT-3453), **243** (Sachen SA-020A) — breadth **172 → 174** (51 Core + 95 Curated + 28 BestEffort), all implemented from the NESdev wiki with no reference-emulator source consulted. The coverage harness moved onto the frontend's real load path, which exposed a **v1.2.0-era defect reaching users**: the per-game database read a `0` Mapper column as "force NROM" and overwrote correct headers, leaving **12 ROMs — every Sachen board in the corpus** — unable to load. Also a Bandai FCG EEPROM debug panic, CLI launches skipping header overrides, mapper 15 PRG-RAM/CHR-RAM, save-state back-compat for 15/88/176, and #360. **Touches the core**, so AccuracyCoin 141/141 is verified, not construction. Workstream C (the APU at 18.7%) **not delivered**, carried to v2.3.5 — see `CHANGELOG.md` `[2.3.4]` | +| **v2.3.4 "Ledger"** | Mapper coverage. Three boards — **176 submapper 2** (WAIXING-FS005), **154** (NAMCOT-3453), **243** (Sachen SA-020A) — breadth **172 → 174** (51 Core + 95 Curated + 28 BestEffort), all implemented from the NESdev wiki with no reference-emulator source consulted. The coverage harness moved onto the frontend's real load path, which exposed a **v1.2.0-era defect reaching users**: the per-game database read a `0` Mapper column as "force NROM" and overwrote correct headers, leaving **12 ROMs — every Sachen board in the corpus** — unable to load. Also a Bandai FCG EEPROM debug panic, CLI launches skipping header overrides, mapper 15 PRG-RAM/CHR-RAM, save-state back-compat for 15/88/176, and #360. **Touches the core**, so AccuracyCoin 141/141 is verified, not construction. Workstream C (the APU at 18.7%) **not delivered**, carried to v2.3.5 — see `CHANGELOG.md` `[2.3.4]` | +| **v2.3.5 "Manifest"** (current) | What the core declares about itself. RetroArch reads `dist/info/rustynes_libretro.info` from **libretro/libretro-super**, a SEPARATE copy from this repo's that nothing synced — so the v2.2.9 GPL relicense reached `Cargo.toml`, `NOTICE`, `deny.toml` and the SPDX headers, and **not the file users see**, which advertised MIT/Apache-2.0 at `v2.2.1` for eleven days. Corrected to **`GPLv3+`** (libretro uses short tokens and marks "or later" with a trailing `+`, tallied across all 316 upstream cores) and pinned by a standing `libretro_info_audit.rs`, so the sync is a copy rather than a re-derivation; **a licence change is now a mandatory upstream-sync trigger**. The wrapper audit that followed found **five defects, each with correct emulation behind it**: a hardcoded 60.0988 fps for every cartridge with `retro_get_region` unimplemented (**PAL ran 20.2% fast**), `retro_reset` unimplemented (**RetroArch's Reset did nothing, ever** — the library default is a literal no-op), `retro_unload_game` unimplemented (Game Genie indices leaked across cartridges), `aspect_ratio = 0.0` (square pixels against the desktop frontend's 8:7), and no controller info (**the Zapper was unreachable** despite `Nes::set_zapper` being fully implemented). Review caught a **use-after-free**: RetroArch shallow-`memcpy`s the outer `retro_controller_info` array but RETAINS each `types` pointer, so those tables must be `'static` — `SET_INPUT_DESCRIPTORS` is different and safe, and the two must never be generalized between. The crate went **0 tests → 8**. Separately the APU (18.7% of frame time, invisible to a symbol profile because fat LTO inlines it into `cpu_clock`) gained its first throughput bench and a default-configuration mix specialization, **−3.3% to −4.2%** on `nes_run_frame_nestest`. Declared values are now DERIVED from `rustynes_core` constants rather than transcribed. **The APU implementation changed**, so AccuracyCoin 141/141 and nestest 0-diff are **verified, not true by construction**. NOT fixed here: RetroArch shows the right licence only once libretro merges, and iOS/iPadOS/tvOS availability is a hardcoded `appstore_cores` list in `libretro/RetroArch` — both upstream — see `CHANGELOG.md` `[2.3.5]` | -> **Forward path.** The v2.0.x "Harbor", v2.1.x "Fathom", and v2.2.x lines have all shipped; the v2.2.6 → v2.3.0 line has now **closed** with v2.3.0 "Datum II"; the v2.3.x performance campaign has now **shipped in full**, as three releases: **v2.3.1 "Plumb Line"** absorbed both the measurement apparatus and the core hot-path campaign, whose ten items were all measured and all rejected and so had no shippable content of their own; **v2.3.2 "Lucid"** the novel features (pixel provenance + replay attestation); and **v2.3.3 "Cadence"** the display-pacing work — the run-ahead throttle oscillation traced to a stale median, the predictive engage arm, and the `wp_presentation` measurement apparatus that made the diagnosis possible. The campaign closed there; **v2.3.4 "Ledger"** (current) opens the next line with mapper coverage — three boards to **174 families**, and the coverage harness moved onto the frontend's real load path, which exposed a per-game-database defect that had left every Sachen cartridge unloadable since v1.2.0. Its Workstream C, the APU at 18.7% of frame time, was **not delivered** and carries to v2.3.5. Note the codenames diverged from this plan as written: what shipped as v2.3.2 took "Lucid" rather than the planned "Grain"/"Conduit II", and v2.3.3 is "Cadence". RustyNES is **permanently open-source and income-free** (ADR 0035): the earlier "joint Google Play + App Store + AltStore + F-Droid launch" is **withdrawn** — any store listing is a **free** app with **no monetization** (no ads, tracking, or paid unlock), an unversioned later step. `to-dos/ROADMAP.md` is the authoritative forward roadmap. +> **Forward path.** The v2.0.x "Harbor", v2.1.x "Fathom", and v2.2.x lines have all shipped; the v2.2.6 → v2.3.0 line has now **closed** with v2.3.0 "Datum II"; the v2.3.x performance campaign has now **shipped in full**, as three releases: **v2.3.1 "Plumb Line"** absorbed both the measurement apparatus and the core hot-path campaign, whose ten items were all measured and all rejected and so had no shippable content of their own; **v2.3.2 "Lucid"** the novel features (pixel provenance + replay attestation); and **v2.3.3 "Cadence"** the display-pacing work — the run-ahead throttle oscillation traced to a stale median, the predictive engage arm, and the `wp_presentation` measurement apparatus that made the diagnosis possible. The campaign closed there; **v2.3.4 "Ledger"** opened the next line with mapper coverage — three boards to **174 families**, and the coverage harness moved onto the frontend's real load path, which exposed a per-game-database defect that had left every Sachen cartridge unloadable since v1.2.0. Its Workstream C, the APU at 18.7% of frame time, was not delivered there and landed in **v2.3.5 "Manifest"** (current), which is otherwise about what the core declares about itself: the libretro `.info` licence drift a user reported, and the five wrapper defects auditing it uncovered. Note the codenames diverged from this plan as written: what shipped as v2.3.2 took "Lucid" rather than the planned "Grain"/"Conduit II", and v2.3.3 is "Cadence". RustyNES is **permanently open-source and income-free** (ADR 0035): the earlier "joint Google Play + App Store + AltStore + F-Droid launch" is **withdrawn** — any store listing is a **free** app with **no monetization** (no ads, tracking, or paid unlock), an unversioned later step. `to-dos/ROADMAP.md` is the authoritative forward roadmap. ## Versioning guidelines From 359f0e845e5ac147578210575d41f920d19bf0f6 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Sun, 16 Aug 2026 18:04:14 -0400 Subject: [PATCH 06/10] fix(probe): make the trial budget binding, and correct the D3 doc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings from Copilot on #384, both correct. 1. `measure` computed `Budget::max_trials`, documented it, and then spent trials through `Probe::run` — which does not consume them. The budget was never enforced: a stated contract nothing checked, which is the same shape as the defects this release line exists to fix. It now spends through `run_counted` and fails closed, returning an inconclusive report if the budget is ever exhausted mid-run. The ceiling is also corrected to EXACTLY what the loop can run — one idle baseline plus one held trial per button, per observable, 21 — rather than the previous "plus headroom" figure, which was both wrong (it double-counted) and wrong in kind: a ceiling with slack in it is not a ceiling. 2. The `gain_is_unity` doc claimed the cache must be recomputed in `reset`. `reset` does not touch `channel_gain` — it deliberately preserves the mixer overlay, since a console reset is not a mixer reset — so the pair stays consistent across it with no work. Corrected to name the two real write sites. THE TEST FOR (1) WAS DECORATION AT FIRST, and the mutation check caught it. The initial version asserted `per_button.len() == 6`, which cannot distinguish the two cases: a budget one trial short still bails on the LAST trial of the LAST observable and still returns the previous observable's full six-entry evidence, so it passed under exactly the mutation it existed to detect. `Probe::trials_used()` is now exposed and carried on `LatencyReport`, and the test asserts the count. Re-mutated to confirm: with the budget one short it fails `left: 20, right: 21`. The field earns its place beyond the test — a UI can say how much work a measurement cost. Co-Authored-By: Claude Opus 5 --- crates/rustynes-apu/src/apu.rs | 10 ++- crates/rustynes-probe/src/latency.rs | 105 ++++++++++++++++++++++----- crates/rustynes-probe/src/lib.rs | 11 +++ 3 files changed, 106 insertions(+), 20 deletions(-) diff --git a/crates/rustynes-apu/src/apu.rs b/crates/rustynes-apu/src/apu.rs index bde295ce..8028408c 100644 --- a/crates/rustynes-apu/src/apu.rs +++ b/crates/rustynes-apu/src/apu.rs @@ -302,8 +302,14 @@ pub struct Apu { /// /// Not serialized, and correctly so: `channel_gain` is a UI playback overlay /// rather than NES hardware state, so it is not in the APU snapshot either. - /// This field is derived from it and must be recomputed wherever it is - /// written — `new`, `reset`, and `set_channel_gain`. + /// + /// This field is derived from it and must be recomputed at every site that + /// writes it — which is exactly two: [`Apu::new`] and + /// [`Apu::set_channel_gain`]. **[`Apu::reset`] is deliberately not one of + /// them**: it leaves the gain overlay alone (a reset is a console reset, not + /// a mixer reset), so the pair stays consistent across it without any work. + /// `the_cached_gain_predicate_cannot_desync` pins that, along with the + /// clamping case a naive implementation gets wrong. pub(crate) gain_is_unity: bool, /// v2.1.6 "Expansion Audio" — the most recent RAW external / on-cart /// expansion-audio sample fed into [`Self::tick_with_external`] (BEFORE the diff --git a/crates/rustynes-probe/src/latency.rs b/crates/rustynes-probe/src/latency.rs index 6e656bf9..48aee155 100644 --- a/crates/rustynes-probe/src/latency.rs +++ b/crates/rustynes-probe/src/latency.rs @@ -99,14 +99,19 @@ pub struct LatencyReport { pub observable: Option, /// Per-button first-divergence frames, in `PROBE_BUTTONS` order (a plain /// code span: that item is private and `rustdoc::private_intra_doc_links` is - /// denied), for a UI - /// that wants to show the evidence rather than just the conclusion. + /// denied), for a UI that wants to show the evidence rather than just the + /// conclusion. pub per_button: Vec>, + /// Trials the measurement actually spent. + /// + /// Useful to a UI ("measured in 7 trials"), and load-bearing for the test + /// that proves the trial budget is binding rather than merely declared. + pub trials_used: u32, } impl LatencyReport { /// An inconclusive report, with the evidence that produced it. - fn inconclusive(per_button: Vec>, probed: u32) -> Self { + fn inconclusive(per_button: Vec>, probed: u32, trials_used: u32) -> Self { let reacting = u32::try_from(per_button.iter().filter(|d| d.is_some()).count()).unwrap_or(u32::MAX); Self { @@ -116,6 +121,7 @@ impl LatencyReport { probed_buttons: probed, observable: None, per_button, + trials_used, } } @@ -163,39 +169,52 @@ impl Default for LatencyConfig { /// Returns as soon as an observable yields agreeing answers, so the common case /// costs one observable's worth of trials rather than all three. pub fn measure(nes: &mut Nes, anchor: &Nes, cfg: LatencyConfig) -> LatencyReport { + // EXACTLY the trials this loop can run: one idle baseline plus one held + // trial per button, per observable. Not "plus headroom" — a ceiling with + // slack in it is not a ceiling, and `run_counted` below makes it binding, so + // a future edit that adds a trial fails closed here rather than silently + // spending more of the caller's time than the budget advertises. let budget = Budget { max_frames_per_trial: cfg.frames_per_trial, - // Two trials per button per observable, plus headroom. - max_trials: u32::try_from((PROBE_BUTTONS.len() + 1) * 2 * OBSERVABLE_ORDER.len()) + max_trials: u32::try_from((PROBE_BUTTONS.len() + 1) * OBSERVABLE_ORDER.len()) .unwrap_or(u32::MAX), }; - let probe = Probe::anchor(anchor, budget); + let mut probe = Probe::anchor(anchor, budget); let probed = u32::try_from(PROBE_BUTTONS.len()).unwrap_or(u32::MAX); let mut last_evidence = vec![None; PROBE_BUTTONS.len()]; for observable in OBSERVABLE_ORDER { // The idle baseline is the same for every button under one observable, // so it is run once rather than per button. - let idle = probe.run(nes, cfg.frames_per_trial, observable, |_| { + // + // A `None` from `run_counted` means the budget is spent. Report what has + // been gathered rather than continuing unbudgeted: the honest answer to + // "I ran out of trials" is inconclusive, never a verdict from partial + // evidence. + let Some(idle) = probe.run_counted(nes, cfg.frames_per_trial, observable, |_| { (Buttons::empty(), Buttons::empty()) - }); + }) else { + return LatencyReport::inconclusive(last_evidence, probed, probe.trials_used()); + }; let mut per_button = Vec::with_capacity(PROBE_BUTTONS.len()); for button in PROBE_BUTTONS { - let held = probe.run(nes, cfg.frames_per_trial, observable, move |_| { + let Some(held) = probe.run_counted(nes, cfg.frames_per_trial, observable, move |_| { (button, Buttons::empty()) - }); + }) else { + return LatencyReport::inconclusive(last_evidence, probed, probe.trials_used()); + }; let d = Probe::first_divergence(&held, &idle).filter(|f| *f <= cfg.max_plausible_lag); per_button.push(d); } - if let Some(report) = conclude(&per_button, probed, observable) { + if let Some(report) = conclude(&per_button, probed, observable, probe.trials_used()) { return report; } last_evidence = per_button; } - LatencyReport::inconclusive(last_evidence, probed) + LatencyReport::inconclusive(last_evidence, probed, probe.trials_used()) } /// Turn per-button divergences into a verdict, or `None` if this observable @@ -204,6 +223,7 @@ fn conclude( per_button: &[Option], probed: u32, observable: Observable, + trials_used: u32, ) -> Option { let reacting: Vec = per_button.iter().filter_map(|d| *d).collect(); if reacting.is_empty() { @@ -240,6 +260,7 @@ fn conclude( probed_buttons: probed, observable: Some(observable), per_button: per_button.to_vec(), + trials_used, }) } @@ -388,6 +409,53 @@ mod tests { assert!(report.observable.is_some()); } + /// The trial budget must be **binding**, not decorative. + /// + /// `measure` sizes `Budget::max_trials` to exactly the trials it can run — + /// one idle baseline plus one held trial per button, per observable — and + /// spends them through `run_counted`. Before review this used `Probe::run`, + /// which does not consume trials, so the budget was computed, documented, + /// and never enforced: a stated contract that nothing checked. + /// + /// This pins the arithmetic, because that is what a future edit breaks. A + /// loop that adds a trial without widening the budget now fails closed at + /// the last observable rather than quietly running over. + #[test] + fn the_trial_budget_is_exactly_what_the_loop_spends() { + let expected = (PROBE_BUTTONS.len() + 1) * OBSERVABLE_ORDER.len(); + + // Drive a full three-observable run: a ROM that never reacts exhausts + // every observable, which is the worst case and the one the budget must + // accommodate exactly. + let rom = spin_rom(); + let anchor = warmed(&rom, 20); + let mut scratch = Nes::from_rom(&rom).expect("fixture parses"); + let report = measure(&mut scratch, &anchor, LatencyConfig::default()); + + // Inconclusive because nothing reacted — NOT because the budget ran out + // mid-run. If the budget were even one trial short, the run would bail + // early through the `run_counted` -> `None` path and still report + // inconclusive, so the count is what distinguishes the two. + assert_eq!(report.confidence, Confidence::Inconclusive); + + // THE assertion. `per_button.len()` cannot distinguish these two cases — + // a budget one trial short still bails on the LAST trial of the LAST + // observable and still returns the previous observable's full six-entry + // evidence, so the first version of this test passed under exactly the + // mutation it existed to catch. `trials_used` is the quantity that + // actually moves. + assert_eq!( + usize::try_from(report.trials_used).unwrap_or(usize::MAX), + expected, + "the run did not spend exactly its budget: either it bailed early \ + (budget too small) or the loop and the budget have drifted apart" + ); + assert_eq!( + expected, 21, + "the trial arithmetic changed; re-check Budget::max_trials in `measure`" + ); + } + /// A divergence past `max_plausible_lag` is discarded: at that distance it is /// far likelier to be the game's own animation than a reaction to the pad. #[test] @@ -398,7 +466,7 @@ mod tests { .iter() .map(|d| d.filter(|f| *f <= LatencyConfig::default().max_plausible_lag)) .collect(); - assert!(conclude(&filtered, 6, Observable::Framebuffer).is_none()); + assert!(conclude(&filtered, 6, Observable::Framebuffer, 0).is_none()); } /// Unanimity, majority and a bare plurality must be told apart — a plurality @@ -406,19 +474,19 @@ mod tests { #[test] fn agreement_is_graded_and_a_plurality_is_not_agreement() { let unanimous = [Some(2), Some(2), None, None, None, None]; - let r = conclude(&unanimous, 6, Observable::Framebuffer).expect("a verdict"); + let r = conclude(&unanimous, 6, Observable::Framebuffer, 0).expect("a verdict"); assert_eq!(r.frames, Some(2)); assert_eq!(r.confidence, Confidence::Unanimous); let majority = [Some(1), Some(1), Some(4), None, None, None]; - let r = conclude(&majority, 6, Observable::Framebuffer).expect("a verdict"); + let r = conclude(&majority, 6, Observable::Framebuffer, 0).expect("a verdict"); assert_eq!(r.frames, Some(1)); assert_eq!(r.confidence, Confidence::Majority); // Two against two: no majority, so no number. let split = [Some(1), Some(1), Some(4), Some(4), None, None]; assert!( - conclude(&split, 6, Observable::Framebuffer).is_none(), + conclude(&split, 6, Observable::Framebuffer, 0).is_none(), "a plurality was reported as a measurement" ); } @@ -435,11 +503,11 @@ mod tests { fn an_even_split_is_inconclusive_at_any_size() { let one_v_one = [Some(3), Some(1), None, None, None, None]; assert!( - conclude(&one_v_one, 6, Observable::Framebuffer).is_none(), + conclude(&one_v_one, 6, Observable::Framebuffer, 0).is_none(), "a 1-1 split was reported as a measurement" ); let two_v_two = [Some(3), Some(3), Some(1), Some(1), None, None]; - assert!(conclude(&two_v_two, 6, Observable::Framebuffer).is_none()); + assert!(conclude(&two_v_two, 6, Observable::Framebuffer, 0).is_none()); } /// The suggested depth is clamped, so a measurement cannot ask for more @@ -453,6 +521,7 @@ mod tests { probed_buttons: 6, observable: Some(Observable::Framebuffer), per_button: vec![Some(7); 6], + trials_used: 0, }; assert_eq!(r.suggested_run_ahead(3), Some(3)); assert_eq!(r.suggested_run_ahead(0), Some(0)); diff --git a/crates/rustynes-probe/src/lib.rs b/crates/rustynes-probe/src/lib.rs index ce5addc6..2fc204e2 100644 --- a/crates/rustynes-probe/src/lib.rs +++ b/crates/rustynes-probe/src/lib.rs @@ -152,6 +152,17 @@ impl Probe { self.budget.max_trials.saturating_sub(self.trials_used) } + /// Trials spent through [`Self::run_counted`] so far. + /// + /// Reported so a caller can say how much work a measurement cost, and so a + /// test can assert that a budget is actually *binding* rather than merely + /// declared — the two are easy to confuse, and a test that cannot tell them + /// apart is decoration. + #[must_use] + pub const fn trials_used(&self) -> u32 { + self.trials_used + } + /// The budget this anchor was taken under. #[must_use] pub const fn budget(&self) -> Budget { From 83cddeb7b30c52dd3d7c1384aaf7a6a85e64a966 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Sun, 16 Aug 2026 18:07:37 -0400 Subject: [PATCH 07/10] fix(probe): drain audio unconditionally, and keep the richest evidence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from the Antigravity review on #384. A third and a nitpick from the same review were already fixed by 359f0e84 (the unenforced trial budget, and the gain-cache doc naming `reset` as a write site). **Audio draining.** `sample` drained only for the `AudioEnergy` observable, so the safety of the whole scheme rested on `Nes::restore` dropping the blip's pending queue. It does — `rustynes-apu`'s snapshot module drops it deliberately — so the contamination the reviewer described cannot occur here. But resting a correctness property on another module's incidental behaviour is the fragile half, and a 120-frame framebuffer trial was piling up ~88k samples for nothing. Now drained every frame, into a buffer allocated once per trial. `tests/restore_audio_pin.rs` records the assumption as MEASURED rather than read off a comment: 30 undrained frames accumulate 21,263 samples, and after a restore the queue holds 0. If `restore` ever starts preserving audio, that test says so directly instead of the failure surfacing as "every game has zero input lag". **Evidence retention.** `last_evidence` was overwritten on each observable, so an inconclusive report could claim `reacting_buttons: 0` because the final observable saw nothing — discarding a framebuffer round that had six reactions and merely failed to agree. The evidence is exactly what a user is shown when the probe declines to answer, so throwing away the informative half makes the decline useless. It now keeps the round with the most reactions. Two other items from that review were checked and NOT changed, with evidence: - "APU cache defaults to false on save-state load": `Apu` has no serde derive and `Apu::restore` is a hand-written field reader that never touches `channel_gain` or `gain_is_unity`, so both survive a load unchanged. The concern assumed whole-struct deserialization this crate does not use. - "`probed` could just be 6": kept derived from `PROBE_BUTTONS.len()` deliberately, so it cannot drift from the array it counts. Co-Authored-By: Claude Opus 5 --- crates/rustynes-probe/src/latency.rs | 12 +++- crates/rustynes-probe/src/lib.rs | 19 ++++-- .../rustynes-probe/tests/restore_audio_pin.rs | 62 +++++++++++++++++++ 3 files changed, 87 insertions(+), 6 deletions(-) create mode 100644 crates/rustynes-probe/tests/restore_audio_pin.rs diff --git a/crates/rustynes-probe/src/latency.rs b/crates/rustynes-probe/src/latency.rs index 48aee155..d089c2b6 100644 --- a/crates/rustynes-probe/src/latency.rs +++ b/crates/rustynes-probe/src/latency.rs @@ -211,7 +211,17 @@ pub fn measure(nes: &mut Nes, anchor: &Nes, cfg: LatencyConfig) -> LatencyReport if let Some(report) = conclude(&per_button, probed, observable, probe.trials_used()) { return report; } - last_evidence = per_button; + // Keep the RICHEST evidence, not the most recent. Raised in review on + // #384: blindly overwriting meant an inconclusive report could end up + // claiming `reacting_buttons: 0` because the last observable saw nothing, + // discarding a framebuffer round that had six reactions and merely failed + // to agree. The evidence is what a user is shown when the probe declines, + // so throwing away the informative half makes the decline useless. + let richer = per_button.iter().filter(|d| d.is_some()).count() + > last_evidence.iter().filter(|d| d.is_some()).count(); + if richer { + last_evidence = per_button; + } } LatencyReport::inconclusive(last_evidence, probed, probe.trials_used()) diff --git a/crates/rustynes-probe/src/lib.rs b/crates/rustynes-probe/src/lib.rs index 2fc204e2..e29bbb2d 100644 --- a/crates/rustynes-probe/src/lib.rs +++ b/crates/rustynes-probe/src/lib.rs @@ -208,13 +208,24 @@ impl Probe { let n = frames.min(self.budget.max_frames_per_trial); let mut samples = Vec::with_capacity(n as usize); - let mut audio = Vec::new(); + // Generously sized so one frame always fits: an NTSC frame at 192 kHz is + // ~3,200 samples. Allocated once per trial, not per frame. + let mut audio = vec![0.0f32; 8192]; for f in 0..n { let (p1, p2) = input(f); nes.set_buttons(0, p1); nes.set_buttons(1, p2); nes.run_frame(); - samples.push(sample(nes, observable, &mut audio)); + // Drain EVERY frame, whatever the observable. `Nes::restore` does + // drop the blip's pending queue (verified by + // `tests/restore_audio_pin.rs`), so trials cannot contaminate each + // other through it — but draining only for `AudioEnergy` made that + // safety depend on restore's audio semantics staying as they are, + // and let a 120-frame framebuffer trial pile up ~88k samples for + // nothing. Raised in review on #384. + audio.clear(); + + samples.push(sample(nes, observable, &audio)); } samples } @@ -267,7 +278,7 @@ impl Probe { } /// Reduce the emulator's current state to one comparable value. -fn sample(nes: &mut Nes, observable: Observable, audio: &mut Vec) -> u64 { +fn sample(nes: &Nes, observable: Observable, audio: &[f32]) -> u64 { match observable { Observable::Framebuffer => fnv1a64(nes.framebuffer()), Observable::IndexFramebuffer => { @@ -281,8 +292,6 @@ fn sample(nes: &mut Nes, observable: Observable, audio: &mut Vec) -> u64 { } Observable::Wram => fnv1a64(nes.wram()), Observable::AudioEnergy => { - audio.clear(); - audio.extend_from_slice(&nes.drain_audio()); // Quantised sum of |amplitude|. Exact float equality across a // resampled stream would compare noise; this asks the coarser // question the fallback is for — "did this frame make a diff --git a/crates/rustynes-probe/tests/restore_audio_pin.rs b/crates/rustynes-probe/tests/restore_audio_pin.rs new file mode 100644 index 00000000..0f5d3b81 --- /dev/null +++ b/crates/rustynes-probe/tests/restore_audio_pin.rs @@ -0,0 +1,62 @@ +//! Pins an assumption the probe engine depends on, raised in review on #384. +//! +//! A reviewer flagged possible **cross-trial audio contamination**: `sample` +//! drained audio only for the `AudioEnergy` observable, so if `Nes::restore` kept +//! the pending presentation queue — as save-states in many emulators do — the +//! undrained framebuffer trials would pile up hundreds of frames, and the first +//! `AudioEnergy` trial would drain them all on frame 0 and diverge falsely +//! against every later trial. +//! +//! It does not happen here: `restore` replaces the blip wholesale and drops the +//! pending queue, which `rustynes-apu`'s snapshot module documents deliberately. +//! This test is the evidence for that claim rather than the claim itself — +//! measured, not read off a comment. The engine now also drains unconditionally, +//! so the property is belt-and-braces, but if `restore` ever starts preserving +//! audio this test says so directly instead of the failure surfacing as a +//! mysterious "every game has zero input lag". + +#[test] +fn restore_drops_pending_audio_so_trials_cannot_contaminate_each_other() { + use rustynes_core::Nes; + let mut prg = vec![0u8; 16 * 1024]; + prg[0] = 0x4C; + prg[1] = 0x00; + prg[2] = 0xC0; + let len = prg.len(); + for (i, b) in [0x00u8, 0xC0, 0x00, 0xC0, 0x00, 0xC0].iter().enumerate() { + prg[len - 6 + i] = *b; + } + let mut bytes = Vec::new(); + bytes.extend_from_slice(b"NES\x1A"); + bytes.extend_from_slice(&[1, 1, 0, 0]); + bytes.extend_from_slice(&[0u8; 8]); + bytes.extend_from_slice(&prg); + bytes.extend_from_slice(&vec![0u8; 8 * 1024]); + + let mut nes = Nes::from_rom(&bytes).unwrap(); + let snap = nes.snapshot(); + // Accumulate WITHOUT draining, exactly as a Framebuffer-observable trial does. + for _ in 0..30 { + nes.run_frame(); + } + let accumulated = nes.drain_audio().len(); + assert!( + accumulated > 10_000, + "premise: 30 undrained frames accumulate (got {accumulated})" + ); + + // Re-accumulate, then restore and drain: if restore keeps the queue, this + // matches `accumulated`; if it drops it, this is ~one frame's worth. + for _ in 0..30 { + nes.run_frame(); + } + nes.restore(&snap).unwrap(); + nes.run_frame(); + let after_restore = nes.drain_audio().len(); + println!("accumulated={accumulated} after_restore={after_restore}"); + assert!( + after_restore < accumulated / 10, + "restore did NOT drop pending audio: {after_restore} vs {accumulated} — \ + cross-trial audio contamination is real" + ); +} From 0980761e6a46babaf04654740bae6b8421e3ccda Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Sun, 16 Aug 2026 18:09:59 -0400 Subject: [PATCH 08/10] refactor(probe)!: the budgeted trial path is now the only public one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit's point on #384, and it is the right one: `Probe::run` took `&self`, never touched `trials_used`, and was public — so the unbudgeted choice was the convenient default, and `latency::measure` duly took it. That is how the budget came to be computed, documented, and unenforced in the first place. Fixing the caller (359f0e84) removed the symptom; this removes the shape. `run` is now the counted path and returns `Option>`; the raw replay is private as `run_uncounted`. A caller cannot bypass the ceiling without editing the crate. Breaking, and deliberately taken now: the crate is new, unreleased, and has one consumer. The same change in six months would be a migration. Gates: 18 unit tests + the restore pin + the doctest green; workspace clippy, rustdoc `-D warnings` and fmt clean. Co-Authored-By: Claude Opus 5 --- crates/rustynes-probe/src/latency.rs | 4 +- crates/rustynes-probe/src/lib.rs | 97 ++++++++++++++++------------ 2 files changed, 58 insertions(+), 43 deletions(-) diff --git a/crates/rustynes-probe/src/latency.rs b/crates/rustynes-probe/src/latency.rs index d089c2b6..7eaec867 100644 --- a/crates/rustynes-probe/src/latency.rs +++ b/crates/rustynes-probe/src/latency.rs @@ -191,7 +191,7 @@ pub fn measure(nes: &mut Nes, anchor: &Nes, cfg: LatencyConfig) -> LatencyReport // been gathered rather than continuing unbudgeted: the honest answer to // "I ran out of trials" is inconclusive, never a verdict from partial // evidence. - let Some(idle) = probe.run_counted(nes, cfg.frames_per_trial, observable, |_| { + let Some(idle) = probe.run(nes, cfg.frames_per_trial, observable, |_| { (Buttons::empty(), Buttons::empty()) }) else { return LatencyReport::inconclusive(last_evidence, probed, probe.trials_used()); @@ -199,7 +199,7 @@ pub fn measure(nes: &mut Nes, anchor: &Nes, cfg: LatencyConfig) -> LatencyReport let mut per_button = Vec::with_capacity(PROBE_BUTTONS.len()); for button in PROBE_BUTTONS { - let Some(held) = probe.run_counted(nes, cfg.frames_per_trial, observable, move |_| { + let Some(held) = probe.run(nes, cfg.frames_per_trial, observable, move |_| { (button, Buttons::empty()) }) else { return LatencyReport::inconclusive(last_evidence, probed, probe.trials_used()); diff --git a/crates/rustynes-probe/src/lib.rs b/crates/rustynes-probe/src/lib.rs index e29bbb2d..805de056 100644 --- a/crates/rustynes-probe/src/lib.rs +++ b/crates/rustynes-probe/src/lib.rs @@ -40,15 +40,16 @@ //! use rustynes_probe::{Budget, Observable, Probe}; //! //! # fn demo(live: &Nes, scratch: &mut Nes) { -//! let probe = Probe::anchor(live, Budget::default()); +//! let mut probe = Probe::anchor(live, Budget::default()); //! //! // Trial A: hold Right from the first frame. Trial B: never press anything. +//! // `run` is budgeted: `None` means the trial ceiling is spent. //! let held = probe.run(scratch, 16, Observable::Framebuffer, |_| { //! (Buttons::RIGHT, Buttons::empty()) -//! }); +//! }).expect("within budget"); //! let idle = probe.run(scratch, 16, Observable::Framebuffer, |_| { //! (Buttons::empty(), Buttons::empty()) -//! }); +//! }).expect("within budget"); //! //! match Probe::first_divergence(&held, &idle) { //! Some(frame) => println!("reacted on frame {frame}"), @@ -152,7 +153,7 @@ impl Probe { self.budget.max_trials.saturating_sub(self.trials_used) } - /// Trials spent through [`Self::run_counted`] so far. + /// Trials spent through [`Self::run`] so far. /// /// Reported so a caller can say how much work a measurement cost, and so a /// test can assert that a budget is actually *binding* rather than merely @@ -187,7 +188,7 @@ impl Probe { /// from, or if the anchor fails to restore. Both are caller errors that /// would otherwise yield a plausible, wrong answer, and this engine exists /// to produce answers people will act on. - pub fn run( + fn run_uncounted( &self, nes: &mut Nes, frames: u32, @@ -230,12 +231,18 @@ impl Probe { samples } - /// [`Self::run`], counting the trial against the budget. + /// Restore the anchor and run one **budgeted** trial. /// /// Returns `None` once [`Self::trials_remaining`] reaches zero, so a search /// loop terminates on the budget rather than on the caller remembering to /// check. - pub fn run_counted( + /// + /// This is the only public way to run a trial, deliberately. An earlier + /// version also exposed an uncounted `run`, which made the unbudgeted choice + /// the convenient default — and `latency::measure` duly used it, so the + /// budget was computed, documented and never enforced. Raised in review on + /// #384; the uncounted path is now private. + pub fn run( &mut self, nes: &mut Nes, frames: u32, @@ -249,7 +256,7 @@ impl Probe { return None; } self.trials_used += 1; - Some(self.run(nes, frames, observable, input)) + Some(self.run_uncounted(nes, frames, observable, input)) } /// The first frame index at which two trials differ, or `None` if they agree @@ -366,10 +373,14 @@ mod tests { for _ in 0..10 { n.run_frame(); } - let probe = Probe::anchor(&n, Budget::default()); - - let a = probe.run(&mut n, 20, Observable::Framebuffer, idle); - let b = probe.run(&mut n, 20, Observable::Framebuffer, idle); + let mut probe = Probe::anchor(&n, Budget::default()); + + let a = probe + .run(&mut n, 20, Observable::Framebuffer, idle) + .expect("within budget"); + let b = probe + .run(&mut n, 20, Observable::Framebuffer, idle) + .expect("within budget"); assert_eq!(a, b, "the determinism contract failed under replay"); assert_eq!(Probe::first_divergence(&a, &b), None); assert!(Probe::agree(&a, &b)); @@ -381,13 +392,17 @@ mod tests { #[test] fn each_trial_restarts_from_the_anchor() { let mut n = nes(); - let probe = Probe::anchor(&n, Budget::default()); - let first = probe.run(&mut n, 8, Observable::Wram, idle); + let mut probe = Probe::anchor(&n, Budget::default()); + let first = probe + .run(&mut n, 8, Observable::Wram, idle) + .expect("within budget"); // Advance well past the anchor between trials. for _ in 0..50 { n.run_frame(); } - let second = probe.run(&mut n, 8, Observable::Wram, idle); + let second = probe + .run(&mut n, 8, Observable::Wram, idle) + .expect("within budget"); assert_eq!(first, second, "the anchor did not restore between trials"); } @@ -395,15 +410,15 @@ mod tests { #[test] fn every_observable_is_deterministic() { let mut n = nes(); - let probe = Probe::anchor(&n, Budget::default()); + let mut probe = Probe::anchor(&n, Budget::default()); for obs in [ Observable::Framebuffer, Observable::IndexFramebuffer, Observable::Wram, Observable::AudioEnergy, ] { - let a = probe.run(&mut n, 6, obs, idle); - let b = probe.run(&mut n, 6, obs, idle); + let a = probe.run(&mut n, 6, obs, idle).expect("within budget"); + let b = probe.run(&mut n, 6, obs, idle).expect("within budget"); assert_eq!(a, b, "{obs:?} was not deterministic under replay"); assert_eq!(a.len(), 6, "{obs:?} produced the wrong sample count"); } @@ -419,8 +434,10 @@ mod tests { max_frames_per_trial: 3, ..Budget::default() }; - let probe = Probe::anchor(&n, budget); - let samples = probe.run(&mut n, 100, Observable::Framebuffer, idle); + let mut probe = Probe::anchor(&n, budget); + let samples = probe + .run(&mut n, 100, Observable::Framebuffer, idle) + .expect("within budget"); assert_eq!(samples.len(), 3, "budget did not cap the trial"); } @@ -435,21 +452,11 @@ mod tests { }; let mut probe = Probe::anchor(&n, budget); assert_eq!(probe.trials_remaining(), 2); - assert!( - probe - .run_counted(&mut n, 2, Observable::Wram, idle) - .is_some() - ); - assert!( - probe - .run_counted(&mut n, 2, Observable::Wram, idle) - .is_some() - ); + assert!(probe.run(&mut n, 2, Observable::Wram, idle).is_some()); + assert!(probe.run(&mut n, 2, Observable::Wram, idle).is_some()); assert_eq!(probe.trials_remaining(), 0); assert!( - probe - .run_counted(&mut n, 2, Observable::Wram, idle) - .is_none(), + probe.run(&mut n, 2, Observable::Wram, idle).is_none(), "the engine handed out a trial past its budget" ); } @@ -498,7 +505,7 @@ mod tests { #[test] fn the_input_closure_is_called_once_per_frame_in_order() { let mut n = nes(); - let probe = Probe::anchor(&n, Budget::default()); + let mut probe = Probe::anchor(&n, Budget::default()); let mut seen = Vec::new(); let _ = probe.run(&mut n, 5, Observable::Wram, |f| { seen.push(f); @@ -527,14 +534,20 @@ mod tests { } n.poke_ram(0x0200, 0x00); - let probe_a = Probe::anchor(&n, Budget::default()); - let a = probe_a.run(&mut n, 4, Observable::Wram, idle); + let mut probe_a = Probe::anchor(&n, Budget::default()); + let a = probe_a + .run(&mut n, 4, Observable::Wram, idle) + .expect("within budget"); // Restore to the same point, change ONE byte, and re-anchor. - probe_a.run(&mut n, 0, Observable::Wram, idle); // restore only + probe_a + .run(&mut n, 0, Observable::Wram, idle) + .expect("within budget"); // restore only n.poke_ram(0x0200, 0xA5); - let probe_b = Probe::anchor(&n, Budget::default()); - let b = probe_b.run(&mut n, 4, Observable::Wram, idle); + let mut probe_b = Probe::anchor(&n, Budget::default()); + let b = probe_b + .run(&mut n, 4, Observable::Wram, idle) + .expect("within budget"); assert_eq!( Probe::first_divergence(&a, &b), @@ -551,13 +564,15 @@ mod tests { #[should_panic(expected = "different ROM")] fn replaying_into_a_different_rom_panics() { let n = nes(); - let probe = Probe::anchor(&n, Budget::default()); + let mut probe = Probe::anchor(&n, Budget::default()); // A ROM with different PRG contents => a different hash tag. let mut other_bytes = synth_nrom(); let prg_start = 16; other_bytes[prg_start + 8] = 0xEA; // NOP somewhere harmless let mut other = Nes::from_rom(&other_bytes).expect("fixture parses"); - let _ = probe.run(&mut other, 1, Observable::Wram, idle); + let _ = probe + .run(&mut other, 1, Observable::Wram, idle) + .expect("within budget"); } } From bdc308a49851702eeb7745d7a6d2cdf060241e63 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Sun, 16 Aug 2026 18:31:45 -0400 Subject: [PATCH 09/10] test(schema): register D3's cached field with the snapshot audit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI caught what I did not: `snapshot_schema_audit::every_chip_field_is_serialized_or_explicitly_excluded` failed, because D3 added `Apu::gain_is_unity` without registering it. The audit requires every chip field to be either serialized or listed as deliberately excluded WITH a reason — precisely so a new field cannot slip into the core without someone stating what happens to it across a save state. The field is correctly excluded: it is the cached form of `channel_gain == CHANNEL_GAIN_UNITY`, and `channel_gain` is itself excluded as a frontend mixer overlay rather than NES hardware state. It is safe to omit because `Apu::restore` is a hand-written field reader that touches neither, so the two cannot desync across a load. Now recorded with that reasoning rather than merely listed. Mine to own, and the same miss as the rustdoc failure earlier in this PR: after adding D3 I ran `-p rustynes-probe`, `-p rustynes-apu --lib`, fmt, clippy and rustdoc — but not `cargo test --workspace`, which is where this test lives. Per-crate green is not workspace green. Full suite now run: 2,059 passing. Worth noting the audit did exactly its job. It is the same mechanism that mechanically found the v2.2.3 PPU/APU snapshot gaps, and it fired here on a field that is genuinely fine — the value is that "genuinely fine" now has to be written down. Co-Authored-By: Claude Opus 5 --- .../tests/snapshot_schema_audit.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/crates/rustynes-test-harness/tests/snapshot_schema_audit.rs b/crates/rustynes-test-harness/tests/snapshot_schema_audit.rs index ab71c5fa..80b64ad9 100644 --- a/crates/rustynes-test-harness/tests/snapshot_schema_audit.rs +++ b/crates/rustynes-test-harness/tests/snapshot_schema_audit.rs @@ -308,6 +308,17 @@ const CHIPS: &[Chip] = &[ "channel_gain", "config: frontend Audio Mixer per-channel gain", ), + ( + "gain_is_unity", + "derived config (v2.3.6 D3): the cached form of `channel_gain == \ + CHANNEL_GAIN_UNITY`, read once per CPU cycle by the default-mix fast \ + path so that predicate is not a 6-wide f32 array compare at 1.789 MHz. \ + Excluded for the same reason `channel_gain` is — it is a frontend mixer \ + overlay, not NES hardware state — and safe to omit because `Apu::restore` \ + is a hand-written field reader that touches neither, so the pair cannot \ + desync across a load. `the_cached_gain_predicate_cannot_desync` pins the \ + write paths that CAN change it", + ), ( "last_external", "output-only: write-only-from-synthesis copy of the expansion-audio DAC tap \ From 982b5a64babb8612bbb166dbfe7f7ad8acaa278b Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Sun, 16 Aug 2026 19:02:05 -0400 Subject: [PATCH 10/10] =?UTF-8?q?perf(apu):=20D3=20measured=20and=20REJECT?= =?UTF-8?q?ED=20=E2=80=94=20reverted,=20recorded=20with=20its=20numbers?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The measurement this PR said it owed. `scripts/perf/ab_check.sh --base --bench nes_run_frame_nestest`, two independent runs, quiet host: workload run 1 run 2 nes_run_frame_nestest +1.72% p=0.00 -0.91% p=0.01 nes_run_frame_nestest_fast -0.45% p=0.31 -0.70% p=0.05 order-bias control (_fast) -2.53% FAILED clean Rejected on three independent grounds, any one sufficient: 1. The sign FLIPS between independent runs on nes_run_frame_nestest, both nominally significant. Mixed signs are a rejection, never something to average — and mixed signs across runs mean the effect is not reproducible. 2. Run 1's order-bias control failed (-2.53% drift from position in the run alone), so its candidate numbers carry at least that much systematic error. 3. The shipped `_fast` variant never moved significantly. fast_dotloop is default-on since v2.2.3, so a change that does not move `_fast` moves nothing a user runs. This is the shape v2.3.1 G2 recorded: a textbook single-run result that evaporates on re-run. REVERTED rather than kept as a simplification. The cache is derived state that must stay in sync with `channel_gain`, which cost a dedicated desync test AND an entry in `snapshot_schema_audit` — two standing obligations for an effect indistinguishable from zero. `apu.rs` and the audit are now byte-identical to their pre-D3 state; verified with `git diff` against D3^. What this does NOT claim: "not measurable here" is not "no difference". The instrument resolves roughly +/-1-2% on this host, so a sub-1% effect is invisible to it. The honest statement is that D3 has no demonstrated benefit, and this project does not carry core state on undemonstrated benefit. `docs/performance.md` records the rejection with its numbers, per the convention that let this campaign skip so many settled dead ends — and lists the five Workstream C levers still unmeasured, D1 (the DMC end-of-cycle pair, ~23% of per-cycle cost) being the largest remaining target. Co-Authored-By: Claude Opus 5 --- crates/rustynes-apu/src/apu.rs | 71 +------------------ .../tests/snapshot_schema_audit.rs | 11 --- docs/performance.md | 52 ++++++++++++++ 3 files changed, 53 insertions(+), 81 deletions(-) diff --git a/crates/rustynes-apu/src/apu.rs b/crates/rustynes-apu/src/apu.rs index 8028408c..0b7b0b22 100644 --- a/crates/rustynes-apu/src/apu.rs +++ b/crates/rustynes-apu/src/apu.rs @@ -292,25 +292,6 @@ pub struct Apu { /// the oracle / test ROMs (which never touch a gain) are unaffected. NEVER /// serialized into the save state (a UI preference, like the mask / volume). pub(crate) channel_gain: [f32; 6], - /// v2.3.6 D3 — cached `channel_gain == CHANNEL_GAIN_UNITY`. - /// - /// The C1 fast-path predicate compared a `[f32; 6]` array **1.789 million - /// times a second** to answer a question that can only change in - /// [`Apu::set_channel_gain`], which a user reaches through a mixer slider. - /// Caching it turns the per-cycle test into a `u8` compare plus a `bool` - /// load. - /// - /// Not serialized, and correctly so: `channel_gain` is a UI playback overlay - /// rather than NES hardware state, so it is not in the APU snapshot either. - /// - /// This field is derived from it and must be recomputed at every site that - /// writes it — which is exactly two: [`Apu::new`] and - /// [`Apu::set_channel_gain`]. **[`Apu::reset`] is deliberately not one of - /// them**: it leaves the gain overlay alone (a reset is a console reset, not - /// a mixer reset), so the pair stays consistent across it without any work. - /// `the_cached_gain_predicate_cannot_desync` pins that, along with the - /// clamping case a naive implementation gets wrong. - pub(crate) gain_is_unity: bool, /// v2.1.6 "Expansion Audio" — the most recent RAW external / on-cart /// expansion-audio sample fed into [`Self::tick_with_external`] (BEFORE the /// UI [`Self::channel_gain`] `[5]` re-weight), retained purely so the @@ -401,7 +382,6 @@ impl Apu { last_frame_events: FrameEvents::default(), channel_mask: CHANNEL_MASK_ALL, channel_gain: CHANNEL_GAIN_UNITY, - gain_is_unity: true, last_external: 0.0, } } @@ -483,10 +463,6 @@ impl Apu { for (slot, g) in self.channel_gain.iter_mut().zip(gain.iter()) { *slot = g.clamp(0.0, 2.0); } - // v2.3.6 D3 — refresh the cached predicate the per-cycle fast path - // reads. Recomputed from the CLAMPED values, so a caller passing 3.0 - // (clamped to 2.0) cannot leave the cache claiming unity. - self.gain_is_unity = self.channel_gain == CHANNEL_GAIN_UNITY; } /// v2.1.3 — select the analog output-filter model (see @@ -1101,10 +1077,7 @@ impl Apu { // it would have received, so the output is byte-identical by // construction rather than by measurement. `apu_default_mix_matches_the_gated_path` // pins that across a 2,048-point sweep anyway. - // v2.3.6 D3 — `gain_is_unity` is the cached form of - // `channel_gain == CHANNEL_GAIN_UNITY`; see the field. Same predicate, - // without a 6-wide `f32` array compare per CPU cycle. - if mask == CHANNEL_MASK_ALL && self.gain_is_unity { + if mask == CHANNEL_MASK_ALL && self.channel_gain == CHANNEL_GAIN_UNITY { self.last_external = external; let mixed = self.mixer.mix( self.pulse1.output(), @@ -1955,48 +1928,6 @@ mod tests { "a non-unity gain must change the emitted audio" ); } - - /// v2.3.6 D3 — the cached `gain_is_unity` must never disagree with the array - /// it summarises. - /// - /// The cache is what the per-cycle fast path reads, so a stale `true` would - /// silently apply unity gain while the user's mixer said otherwise — a - /// wrong-output bug with no assertion anywhere else to catch it. Every write - /// path to `channel_gain` is exercised, including the clamp: a caller asking - /// for 3.0 gets 2.0, which is NOT unity, and the cache must say so. - #[test] - fn the_cached_gain_predicate_cannot_desync() { - let mut apu = Apu::new(Region::Ntsc, 48_000); - assert!(apu.gain_is_unity, "a fresh APU is at unity gain"); - assert_eq!(apu.channel_gain, CHANNEL_GAIN_UNITY); - - apu.set_channel_gain([0.5, 1.0, 1.0, 1.0, 1.0, 1.0]); - assert!(!apu.gain_is_unity, "cache missed a non-unity gain"); - assert_eq!(apu.gain_is_unity, apu.channel_gain == CHANNEL_GAIN_UNITY); - - // Back to unity: the cache must recover, not latch. - apu.set_channel_gain(CHANNEL_GAIN_UNITY); - assert!(apu.gain_is_unity, "cache latched non-unity"); - - // Clamped input: 3.0 becomes 2.0, which is not unity. - apu.set_channel_gain([3.0, 1.0, 1.0, 1.0, 1.0, 1.0]); - assert_eq!(apu.channel_gain[0], 2.0, "premise: the setter clamps"); - assert!( - !apu.gain_is_unity, - "cache computed from the pre-clamp value, not the stored one" - ); - assert_eq!(apu.gain_is_unity, apu.channel_gain == CHANNEL_GAIN_UNITY); - - // `reset` does not touch the gain overlay, so the cache must survive it. - apu.set_channel_gain([0.25; 6]); - apu.reset(); - assert_eq!( - apu.gain_is_unity, - apu.channel_gain == CHANNEL_GAIN_UNITY, - "reset desynced the cache from the array" - ); - } - use super::*; #[test] diff --git a/crates/rustynes-test-harness/tests/snapshot_schema_audit.rs b/crates/rustynes-test-harness/tests/snapshot_schema_audit.rs index 80b64ad9..ab71c5fa 100644 --- a/crates/rustynes-test-harness/tests/snapshot_schema_audit.rs +++ b/crates/rustynes-test-harness/tests/snapshot_schema_audit.rs @@ -308,17 +308,6 @@ const CHIPS: &[Chip] = &[ "channel_gain", "config: frontend Audio Mixer per-channel gain", ), - ( - "gain_is_unity", - "derived config (v2.3.6 D3): the cached form of `channel_gain == \ - CHANNEL_GAIN_UNITY`, read once per CPU cycle by the default-mix fast \ - path so that predicate is not a 6-wide f32 array compare at 1.789 MHz. \ - Excluded for the same reason `channel_gain` is — it is a frontend mixer \ - overlay, not NES hardware state — and safe to omit because `Apu::restore` \ - is a hand-written field reader that touches neither, so the pair cannot \ - desync across a load. `the_cached_gain_predicate_cannot_desync` pins the \ - write paths that CAN change it", - ), ( "last_external", "output-only: write-only-from-synthesis copy of the expansion-audio DAC tap \ diff --git a/docs/performance.md b/docs/performance.md index 3ca57c75..e1383476 100644 --- a/docs/performance.md +++ b/docs/performance.md @@ -3597,6 +3597,58 @@ Prediction recorded and wrong, for the record: this campaign expected the shorter, rendering-heavy `flowing_palette` frame to show the *larger* relative win, since the APU should be a bigger fraction of it. It showed essentially none. +### v2.3.6 D3 — caching the C1 fast-path gain predicate (decision: REJECTED, reverted) + +**The change.** v2.3.5's C1 fast path tests +`mask == CHANNEL_MASK_ALL && channel_gain == CHANNEL_GAIN_UNITY` once per CPU +cycle. The second half is a 6-wide `f32` array comparison evaluated **1.789 +million times a second** to answer a question that can only change when a user +drags a mixer slider. D3 cached it in a `gain_is_unity: bool`, reducing the +per-cycle test to a `u8` compare plus a `bool` load. Byte-identical by +construction: same predicate over the same array, same branch taken. + +**Adjudicated with `scripts/perf/ab_check.sh --base --bench nes_run_frame_nestest`, +two independent runs, quiet host.** + +| workload | run 1 | run 2 | +|---|---:|---:| +| `nes_run_frame_nestest` | **+1.72%** (p = 0.00) | **−0.91%** (p = 0.01) | +| `nes_run_frame_nestest_fast` (shipped default) | −0.45% (p = 0.31) | −0.70% (p = 0.05) | +| order-bias control, `_fast` | **−2.53% (p = 0.00) — FAILED** | clean | + +**Rejected**, on three independent grounds, any one of which suffices: + +1. **The sign flips between independent runs** on `nes_run_frame_nestest`: + +1.72% then −0.91%, both nominally significant. Mixed signs are a rejection, + never something to average — and mixed signs *across runs* mean the effect is + not reproducible at all. +2. **Run 1's order-bias control failed** (`_fast` drifted −2.53% from position in + the run alone), so run 1's candidate numbers carry at least that much + systematic error and its small result is not interpretable. +3. **The shipped `_fast` variant never moved significantly** (p = 0.31, then + p = 0.05). `fast_dotloop` has been default-on since v2.2.3, so a change that + does not move `_fast` moves nothing a user runs. + +This is the shape v2.3.1 G2 recorded: a textbook single-run result that +evaporates on re-run. A third run was not pursued — even the most favourable +reading is under 1%, and the change is not free: the cache is derived state that +must be kept in sync with `channel_gain`, which cost a dedicated desync test and +an entry in `snapshot_schema_audit`. Two standing obligations for an effect +indistinguishable from zero is a bad trade, so the code was reverted rather than +kept as a simplification. + +**What this does not say.** "Not measurable here" is not "no difference". The +instrument's resolution on this host is roughly ±1-2%, so a sub-1% effect is +invisible to it. The honest claim is that D3 has no *demonstrated* benefit, and +the project does not carry core state on undemonstrated benefit. + +**Still open from the v2.3.4 Workstream C list**, unmeasured: D1 (gating the DMC +end-of-cycle pair, ~23% of per-cycle cost and never optimized — the largest +remaining target, and the hardest byte-identity proof), D2 (`FrameCounter::tick` +as a countdown rather than a 6-arm match per cycle), D4 (`Pulse::muted()` +caching), D5 (hoisting `add_sample`'s finite-check), D6 (gating the four +unconditional `length.reload()` calls). + ## Things explicitly *not* in scope for v1.0 - **JIT recompilation** of CPU code. NES games are small enough that interpretation suffices; JIT complicates everything. (Higan/ares don't JIT either.)