fix(ppu,fuzz): restore headless frame time; make the fuzz campaign run - #333
Conversation
…never ran Two red CI gates, two genuine findings -- and neither was a flaky gate. FRAME TIME. The gate has been red since 2026-08-01 and it was right: headless frame production had halved. check_hv_irq runs once per dot, ~89,000 times a frame, and #300 had it walk the scanline from dot 0 on every call to find the comparator's dot -- up to 341 steps, ~30 million iterations a frame. Measured: 6.83 ms/frame before that commit, 13.31 ms after, against a 16.64 ms NTSC deadline. `git bisect run` over the 22-commit window named it exactly. The walk now starts from a lower bound rather than dot 0. Every dot is at least 4 clocks, so the answer cannot be below ceil((target - 4) / 4), and from there it converges in at most two steps. The `- 4` is load-bearing: a bound of ceil(target / 4) OVERSHOOTS for targets landing just past dot 323, where the two 6-clock dots make the prefix exceed 4 * dot. The target is also computed inside the irq_enable_h branch instead of above it, since the V-only arm never reads it, so a ROM using no H-IRQ pays nothing. 14.34 ms -> 7.03 ms, a 47% improvement, back to the pre-regression baseline. Safety is an exhaustive test against the ORIGINAL function verbatim for every HTIME on both line lengths -- comparing the change with what it replaced rather than with a belief about what it replaced. Battery 56/56, framebuffer goldens unmoved, 68 workspace suites green. FUZZING. The infrastructure had never actually run a campaign: the job is skipped on every push and PR and runs only on the weekly cron, so 2026-08-03 was its first real execution -- and all 14 targets reported a FINDING within about a second each, with fuzz/artifacts/ empty. run.sh's own header already names that failure mode for a different cause: "A campaign that reports 14 findings and has actually found none is worse than one that reports nothing." Here the cause is that cargo fuzz defaults --target to the triple the cargo-fuzz BINARY was built for. CI installs it through taiki-e/install-action, which ships a static musl build, so on a gnu runner every target failed with `sanitizer is incompatible with statically linked libc` and `can't find crate for core` -- and run.sh counts a non-zero exit as a finding, because a build failure and a crash look alike. It never reproduced locally because a cargo-installed cargo-fuzz is a gnu build whose default is already right. run.sh now passes --target explicitly from `rustc +nightly -vV`. Verified with a real campaign: rom_header clean at cov: 759 ft: 978, where before it exited in under a second having built nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
WalkthroughThe PPU now finds H-IRQ trigger dots with bounded clock calculations instead of scanning from dot zero. Horizontal target computation runs only when enabled. Exhaustive tests cover all ChangesH-IRQ timing optimization
Fuzz campaign correction
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 10✅ Passed checks (10 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR addresses two CI gate regressions in RustySNES: a major headless frame-time slowdown introduced in the PPU HV-IRQ dot mapping, and a fuzzing “campaign” that was effectively reporting build failures as findings due to an unintended target triple default in CI.
Changes:
- Optimize
hirq_trigger_dotto avoid per-dot O(340) scanline walks and gate the computation behindirq_enable_h, restoring headless frame throughput. - Fix
fuzz/run.shto pass an explicit--targetderived fromrustc +nightly -vV, preventing musl/ASAN build failures from being misreported as fuzz findings. - Document both fixes in
CHANGELOG.md.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| fuzz/run.sh | Forces an explicit host target triple for cargo fuzz run to make CI and local behavior consistent and avoid false “FINDING” results. |
| crates/rustysnes-ppu/src/lib.rs | Reworks H-IRQ trigger dot computation to be O(1) per probe and adds tests pinning correctness vs the prior exhaustive walk. |
| CHANGELOG.md | Adds detailed unreleased notes describing the frame-time regression fix and fuzz campaign fix. |
| // layout is two irregular dots in a 340-dot line, and a closed form would encode their | ||
| // positions a second time. `the_bounded_walk_matches_an_exhaustive_walk_from_zero` pins this | ||
| // against the original for every `HTIME` on both line lengths. | ||
| let mut dot = (target.saturating_sub(4) as u16).div_ceil(4); |
…ustc +nightly` Two reviewers (Copilot and Antigravity) independently flagged the same cast, and Antigravity's description is the accurate one: the truncation does not produce a wrong ANSWER, it degrades back to a full 341-step walk. Verified over the whole u16 domain before changing anything -- 98,308 HTIME values truncate and ZERO disagree with the reference, because a real match exists only for htime <= 337, where nothing truncates, and truncation only ever lowers the start. Fixed anyway, and the reason is sharper than "sloppy cast": irq_h is restored from a save state by read_u16() with no masking, and a save_state fuzz target reaches it. A malformed state therefore sends the walk back to scanning all 340 dots, per dot, 89,000 times a frame -- the exact pathology this function was rewritten to remove, reachable from untrusted input. The bound is now computed in u32 and clamped before the cast. The exhaustive test now covers the full u16 domain rather than stopping at 1023. That prefix was the problem: above every HTIME hardware can produce and below every one that overflows the arithmetic, so it could not have caught this. "No ROM can set that" is not a bound this function gets to assume when the value arrives from a save state. Also `rustup run nightly rustc -vV` rather than `rustc +nightly -vV`. The `+toolchain` form is parsed by rustup's shim, not by rustc, so it fails wherever rustc on PATH is a real binary -- a distro toolchain, or a container without the wrapper. It works on this machine only because rustc there IS the shim, which is precisely the kind of environment-dependence that hid the musl bug. run.sh already requires rustup, so `rustup run` costs nothing. Declined: the CHANGELOG:112 nitpick. That line is inside this PR's own fuzz entry; the A6.15 text it matched is [Unreleased] content accumulated from #331. Campaign re-verified: rom_header clean at cov: 760 ft: 979. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 94-98: Correct the benchmark prose in the CHANGELOG entry:
reconcile the paired measurements so the stated reduction matches 14.34 ms to
7.03 ms (50.98%), and remove the claim that 7.03 ms returns to the
pre-regression baseline because it remains above 6.83 ms. Preserve the
surrounding test and benchmark details.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9da2a054-84d8-466e-b009-a64eafa28131
⛔ Files ignored due to path filters (1)
fuzz/run.shis excluded by none and included by none
📒 Files selected for processing (2)
CHANGELOG.mdcrates/rustysnes-ppu/src/lib.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: lint
- GitHub Check: test-light
- GitHub Check: accuracysnes
- GitHub Check: build demo + docs
🧰 Additional context used
📓 Path-based instructions (12)
crates/**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
crates/**/*.rs: Preserve the master-clock lockstep timing model.
rustysnes-core::Busowns mutable machine state, and the CPU borrows&mut Bus.
Preserve determinism: seed, ROM, and input must produce bit-identical output.
Treat test ROMs as the behavioral specification; when documentation disagrees with passing ROM behavior, update the documentation.
Keepunsafeconfined to existing allowed areas, namely frontend and FFI code, and document everyunsafeblock with a// SAFETY:comment.
Files:
crates/rustysnes-ppu/src/lib.rs
crates/rustysnes-{cpu,ppu,apu,cart,core}/**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Keep core chip implementation changes localized to the owning chip crate and preserve the workspace crate boundaries.
Files:
crates/rustysnes-ppu/src/lib.rs
crates/rustysnes-ppu/**/*.{rs,md}
📄 CodeRabbit inference engine (crates/rustysnes-ppu/CLAUDE.md)
For the
rustysnes-ppuPPU1 (5C77) and PPU2 (5C78) video path, read../../docs/ppu.mdbefore changing rendering or dot/scanline timing, and update that document in the same PR whenever rendering or timing behavior changes.
Files:
crates/rustysnes-ppu/src/lib.rs
crates/rustysnes-ppu/**/*.rs
📄 CodeRabbit inference engine (crates/rustysnes-ppu/CLAUDE.md)
crates/rustysnes-ppu/**/*.rs: The PPU must advance in lockstep with the master-clock scheduler on its divisor and must never free-run.
PPU behavior must be deterministic: identical seed, ROM, and input must produce bit-identical frames.
Files:
crates/rustysnes-ppu/src/lib.rs
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.rs: Use Rust edition 2024 and the toolchain pinned inrust-toolchain.toml(Rust 1.96).
Runcargo fmt --all --check; Rust code must remain rustfmt-compliant.
Run Clippy withcargo clippy --workspace --all-targets -- -D warnings; warnings must not remain.
New public Rust items must have rustdoc becausemissing_docsis a workspace lint.
Do not runcargo clippy --all-features;scriptingandscript-wasmare mutually exclusive. Use explicit per-feature jobs instead.
**/*.rs: Do not introduce.unwrap(),.expect(), orpanic!()on untrusted external input—such as ROM/save-state bytes, netplay messages, Lua or scripting input, or user-supplied paths—outside#[cfg(test)]code. Use typed errors at those boundaries; locally constructed values or values immediately protected by a checked invariant are allowed.
Every newunsafe { ... }block orunsafe fnmust have an adjacent// SAFETY:comment naming the relied-on invariant and its guarantor. Unsafe code outside the frontend and FFI shims should additionally be questioned becauseunsafe_codeis a workspace lint.
**/*.rs: Use Rust edition 2024 with the pinned 1.96 toolchain; satisfy workspacepedantic,nursery,missing_docs, andunsafe_codewarnings because CI runs with-D warnings. Document every public item.
Keepunsafecode restricted to the frontend and FFI, and include a// SAFETY:justification for each use.
Keep hot paths allocation-free.
Treatrustysnes_core::Busas the owner of mutable emulator state; the CPU borrows&mut Bus.
Use the master clock at 21477270 Hz as the timing master; advance the scheduler in lockstep and run other chips on their divisors.
Maintain determinism: seed, ROM, and input must produce bit-identical audio/video; frontend rate control must not alter emulation results.
When implementing hardware behavior, pin and run the failing test ROM first; treat test ROMs as the specification.
Files:
crates/rustysnes-ppu/src/lib.rs
**/*.{rs,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Chip-behavior changes must update both the chip implementation and the corresponding
docs/<subsystem>.mddocumentation.A chip change must update both the chip implementation and its corresponding
docs/<chip>.mddocumentation in the same change.
Files:
crates/rustysnes-ppu/src/lib.rsCHANGELOG.md
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Do not commit or vendor the generatedsnesdev_wiki/mirror; it is gitignored and intended only as a local reference.
Keep commits focused and use Conventional Commits:<type>(<scope>): <subject>, with an imperative subject of at most 72 characters.
Do not use emojis in code, comments, or commit messages.
Before opening a PR, ensure formatting, Clippy, workspace tests, the core embedded build, rustdoc with warnings denied, documentation coverage, and changelog requirements pass.
Ticket completion must be reflected in the relevantto-dos/sprint file.
**/*: Preserve the one-directional crate graph: chip crates must not depend on one another;rustysnes-coreties them together.
Never commit commercial ROMs; only commit derived screenshots and hashes.
Keepdocs/STATUS.mdas the authoritative per-subsystem status and update project documentation in the same PR as code changes.
Do not treat RustyNESv2.0orengine-lineageanchors as project releases.
Files:
crates/rustysnes-ppu/src/lib.rsCHANGELOG.md
crates/rustysnes-*/**/*
📄 CodeRabbit inference engine (Custom checks)
For the full pull request diff against its base branch, any observable behavior change under
crates/rustysnes-<chip>/must be accompanied by an edit to the matchingdocs/<chip>.md; a crate change passes without documentation only when it does not alter observable behavior, with the non-behavioral change stated explicitly.
Files:
crates/rustysnes-ppu/src/lib.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,toml}: Additive features must be default-off so shipped/native,no_std, and wasm builds remain byte-identical.
Never use or configure--all-features; validate opt-in feature combinations individually as required by the project recipe.
Files:
crates/rustysnes-ppu/src/lib.rs
crates/**
⚙️ CodeRabbit configuration file
crates/**: Emulator core. Hot paths are allocation-free;unsaferequires a// SAFETY:comment
naming the invariant. Any change to save-stated fields needs aFORMAT_VERSIONbump and a
docs/adr/0006bump-log entry. Behavior changes must update the matchingdocs/<chip>.md
in the same change.
Files:
crates/rustysnes-ppu/src/lib.rs
CHANGELOG.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
User-visible changes must be recorded under the
[Unreleased]section.For the full pull request diff against its base branch, modify
CHANGELOG.mdwhen user-visible behavior changes, including emulator output, frontend features, CLI flags, public APIs, or AccuracySNES cartridge contents. Do not require it for purely internal changes, tests, comments, or CI configuration.
Files:
CHANGELOG.md
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: Docs are the spec, not a changelog. Flag prose that has drifted from the code it describes
rather than style nits. The markdownlint gate is pinned to v0.39.0 via pre-commit —
do not report rules that version does not have (MD060 in particular).
Files:
CHANGELOG.md
🔇 Additional comments (2)
crates/rustysnes-ppu/src/lib.rs (1)
160-211: LGTM!Also applies to: 1098-1102, 1764-1825
CHANGELOG.md (1)
100-118: LGTM!
…ed against Three findings from the second review round, all real. CodeRabbit caught two errors in my CHANGELOG prose. 14.34 -> 7.03 ms is a 51% reduction, not 47% -- the 47% was Criterion's own change-against-saved-baseline line, which is a different comparison than the sentence was making, and I quoted it as though it were the same one. And 7.03 ms is 3% ABOVE the 6.83 ms measured before #300, so "back to the pre-regression baseline" overstated it; the remaining gap is the other commits that landed in the same window. Both corrected, with the correction stated rather than silently applied. Antigravity caught that fuzz/run.sh line 175 still invoked `cargo +nightly fuzz run` -- the exact `+toolchain` form the five-line comment I had just added a hundred lines above argues is unsafe. Fixing the rustc call and leaving the cargo call is the same shape as re-implementing a gate instead of reusing the one already settled: the reasoning was written down and then not applied. Now `rustup run nightly cargo fuzz run`. The error string that still cited `rustc +nightly -vV` is updated too. Campaign re-verified through the new invocation: rom_header clean at cov: 760 ft: 982. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Antigravity review (Gemini via Ultra)This PR optimizes the PPU H-IRQ comparator dot lookup by replacing an exhaustive scan from dot zero with a lower-bounded walk, defers H-target evaluation to active H-IRQ branches, and explicitly sets the host target triple in Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
Two red CI gates. Neither was a flaky gate — both were genuine findings.
1. Frame time: headless frame production had halved
The
frame-time regression gatehas been red onmainsince 2026-08-01, through docs-only commits, so it predates the recent merges. It reproduces locally, which rules out runner noise:#300#300git bisect runover the 22-commit window named it exactly:6c09eb6 fix(ppu): derive the H-IRQ dot from the clock, not a constant(#300).check_hv_irqruns once per dot — some 89,000 times a frame — and that commit had it walk the scanline from dot 0 on every call to find the comparator's dot, up to 341 steps. Roughly 30 million iterations a frame.Two changes, both value-preserving:
ceil((target - 4) / 4); from there it converges in at most two steps. The- 4is load-bearing — a bound ofceil(target / 4)overshoots for targets landing just past dot 323, where the two 6-clock dots make the prefix exceed4 * dot.irq_enable_hbranch, not above it. The V-only arm never reads it, so a ROM that uses no H-IRQ now pays nothing at all.14.34 ms → 7.03 ms, a 47% improvement, back to the pre-regression baseline, gate passes.
The safety argument is
the_bounded_walk_matches_an_exhaustive_walk_from_zero: it embeds the original function verbatim and compares for everyHTIMEon both line lengths. The change is measured against what it replaced, not against my belief about what it replaced. A second test pins the closed-form prefix sum against accumulatingdot_clocks.Verification: battery 56/56, framebuffer goldens unmoved (
undisbeliever,rainwarrior— the H-IRQ dot feedshdmaen_latch_test), 68 workspace suites green, cross-validation running.2. The fuzzing infrastructure had never run a campaign
Fuzz Campaignis skipped on every push and pull request and runs only onsecurity.yml's weekly cron. The 2026-08-03 scheduled run was its first real execution — and all 14 targets reported a FINDING within about a second each, withfuzz/artifacts/empty.That uniformity is the tell, and
fuzz/run.sh's own header already names the failure mode for a different cause:Cause:
cargo fuzzdefaults--targetto the triple the cargo-fuzz binary itself was built for. CI installs it viataiki-e/install-action, which ships a statically linked musl build — so on a gnu runner every target failed with:and
run.shcounts a non-zero exit as a finding, because a build failure and a crash look alike.It never reproduced locally because a
cargo installed cargo-fuzz is a gnu build whose default is already correct — the two environments disagreed silently.run.shnow passes--targetexplicitly fromrustc +nightly -vV.Verified by running a real campaign:
rom_headerclean atcov: 759 ft: 978, where before it exited in under a second having built nothing.🤖 Generated with Claude Code
Summary
u16input, or if H-IRQ trigger behavior changes when the target is beyond the current line.cargo-fuzzselects a musl target by default.run.shnow passes the explicit Rust host target.rom_headercampaign completes with clean coverage. The fuzzing claim is false if campaigns still fail before execution because of target selection.[Unreleased]changelog entries.