perf(frontend): port the triple-buffer present handoff (GUI stall, step 1) - #206
Conversation
…RustySNES
First step on the user-reported GUI stall: menu clicks taking 15-45s, one
presented frame every 30-60s, SM64 showing a frame then hanging.
DIAGNOSED, not guessed. The emu thread (emu_thread.rs:70-80) holds the emu mutex
across its ENTIRE `coordinator.step()` -- not "under a brief lock" as its own
comment claims -- and when it falls behind it sets `next = now` and re-acquires
immediately with no sleep and no yield. Since the core is ~6.5x slower than real
time (measured: 462s of emulated time took 2985s wall in the corpus census), it
is ALWAYS behind, so it holds the lock ~100% of the time in a tight loop.
`app.rs:144 snapshot()` needs that same lock every UI frame just to clone the
framebuffer, so the UI starves on an unfair mutex.
DO NOT REINVENT -- the user pointed out the sibling projects already solve this,
and they do. `RustySNES/.../present_buffer.rs` (itself ported from RustyNES) is
exactly this handoff, and its module doc names RustyN64's bug verbatim: copying
the framebuffer out from under the emu mutex "would serialize the present against
the emulation thread's whole run_frame (which the emu-thread feature exists
specifically to decouple)". RustyN64 shipped `emu-thread` WITHOUT the handoff, so
it does the thing that module documents as wrong.
Ported here, same author throughout so it is licence-clean. Triple-buffer SPSC:
producer writes `back` and publishes by swapping back<->ready; consumer swaps
front<->ready. Producer and consumer touch disjoint slots (the packed index's
three 2-bit fields stay a permutation of {0,1,2}), and the small dedicated mutex
is held only for one memcpy -- never across emulation.
N64-specific adaptations:
- FB_LEN is the PAL worst case (720x576) and is a SIZING HINT ONLY; the N64's
scan-out is variable (625x237 typical NTSC) so slots resize per publish rather
than zero-padding a small frame up to the maximum.
- Dims travel WITH their bytes through the same lock. Not hypothetical here:
ledger R-18 records a single-point sample catching Super Mario 64 with
`H_VIDEO = 0` mid-VI-reprogram, which is exactly the mismatch a separately
queried dims read would produce.
- The status-bar readings (frame count, master_ticks, loaded, paused) are
published beside the frame as plain atomics, because they were the OTHER thing
`snapshot()` took the emu lock for -- decoupling only the bytes would have left
the starvation in place for the sake of a status line.
Determinism (ADR 0004) is unaffected: this moves WHERE already-produced
deterministic bytes are copied. No emulated state, no audio.
Six tests, each aimed at a way the index could be wrong rather than at coverage:
a publish/take round trip; a take with nothing new preserving the previous frame;
two publishes between takes yielding the NEWEST (a mis-packed index would return
stale bytes); dims surviving a size change; the index staying a permutation of
{0,1,2} over 32 cycles; and reset clearing the published state.
NOT YET WIRED -- this lands the primitive with its tests so the wiring change
(emu_thread publishes, app.rs stops taking the emu lock, and the missing yield
when behind) is reviewable on its own. Tasks #54-#57 track the rest, including
RustyNES's perf_log.rs (the instrumentation task #55 needs before optimizing
anything) and resampler.rs (the audio hiccup).
Gates: fmt, clippy -D warnings, cargo test --workspace, rustdoc -D warnings,
check_en_us -- all green.
|
Warning Review limit reached
Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe frontend now exposes a triple-buffer framebuffer handoff with frame dimensions, status atomics, reset support, and tests. The English-check script also scans untracked non-ignored files and reports the combined file count. ChangesFramebuffer presentation handoff
English-check file discovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant EmuThread
participant PresentBuffer
participant PresentPath
EmuThread->>PresentBuffer: publish framebuffer bytes and dimensions
PresentBuffer-->>PresentPath: signal new frame
PresentPath->>PresentBuffer: take_into output buffer
PresentBuffer-->>PresentPath: return newest bytes and dimensions
🚥 Pre-merge checks | ✅ 8 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (8 passed)
Comment |
Found while correcting a doc error in present_buffer.rs: the en-US gate reported PASS on commit 25b5212, and that commit contains a `licence`. ROOT CAUSE: the gate enumerated files with `git ls-files`, which lists only TRACKED files. A brand-new file is untracked until it is staged, so it was invisible to the check for exactly as long as it took to `git add` it -- and a new file is precisely where a new violation arrives. The pre-commit hook would not have caught it until the NEXT commit, by which point the bad spelling is already on main. My own gate, blind in the one place it most needed to see. FIX: enumerate tracked files PLUS `git ls-files --others --exclude-standard` (untracked but not ignored). `--exclude-standard` keeps the original property that scratch files under a gitignored path never fail the gate. Verified three ways, because only the set of three shows it is right: 1. clean tree -> passes 2. an UNTRACKED new file containing "colour" -> now CAUGHT (was invisible) 3. a file under the gitignored target/ -> still correctly exempt Also in this commit, both from the wiring survey of the frontend: - CORRECTED A FACTUAL ERROR in present_buffer.rs's module doc: it referred to `EmuApp::snapshot()` twice. There is no `EmuApp` in this crate -- the winit struct is `App` (app.rs:59). The name came in with the port from RustySNES and I did not check it against this codebase. A doc that names a type which does not exist sends the next reader grepping for nothing. - RECORDED A MEASUREMENT that strengthens the case for the handoff: `Frame::blank` sizes `rgba` at `FB_MAX_W * FB_MAX_H * 4` = 1,228,800 bytes and `produce_frame` never resizes it, so `snapshot()`'s `frame.rgba.clone()` copied the ENTIRE backing store every UI frame whatever the active resolution. The UI was not just waiting on the mutex, it was doing a 1.2 MB memcpy while holding it. Publishing the `w * h * 4` prefix instead is 592,500 bytes at 625x237 and 307,200 at 320x240 -- under a quarter the copy, and off the emu mutex entirely. Gates: fmt, clippy -D warnings, cargo test, rustdoc -D warnings, markdownlint, check_no_roms, check_en_us -- all green.
|
@coderabbitai review |
✅ Action performedReview finished.
|
Adopted from the Antigravity review of #206, and it is a violation of this module's own stated contract: the SPSC section claims the mutex guards the index, the has_new flag, the bytes and the dims *together*, while reset() cleared has_new BEFORE acquiring the lock and never touched the dims. The window is real, not theoretical -- reset() runs on the UI thread while the emu thread publishes. A publish landing between the store and the lock set has_new = true and then had its bytes wiped, so the next take_into returned Some(dims) over a zero-length buffer. The present path slices out[..w * h * 4] from exactly that pair, which panics. Two independent halves, so the invariant does not rest on locking alone: - has_new is now cleared inside the lock, making the state unreachable; - the dims are cleared, making "nonzero dims, no bytes" unrepresentable. generation stays outside the lock on purpose: clippy's significant_drop_tightening is right that this is the present hot path, and the counter is a diagnostic. The residual race is now documented with its bound -- one UI frame of the previous image instead of black, and never a slice, because has_new and the dims are under the lock. Tests (the review's nitpick, and the mutation check for the fix): - reset_clears_the_dims_so_none_can_be_paired_with_empty_bytes -- verified red against the old reset with exactly the stale (2, 2) the review predicted; - status_round_trips_and_reset_zeroes_only_the_counters -- pins that reset leaves rom_loaded/paused to the caller, so a closed ROM must republish them. Also records that status() can tear ACROSS its four independent atomics, not only lag by a frame, and that a seqlock -- not stronger orderings -- is the fix if a consistent (frames, ticks) pair is ever needed. Gates: fmt, clippy -D warnings, cargo test --workspace, rustdoc -D warnings, check_en_us.sh -- all green.
Adjudication of the Antigravity reviewAll four findings answered. Fixed in 2a2ca7c. Blocking:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/rustyn64-frontend/src/present_buffer.rs`:
- Around line 265-280: Update PresentBuffer::reset to also restore rom_loaded
and paused to their default false values, ensuring status() reports
PresentStatus::default() after a reset. Extend reset_clears_the_published_state
to assert the reset status equals PresentStatus::default().
In `@scripts/check_en_us.sh`:
- Around line 101-105: Update the file collection pipeline in check_en_us.sh so
failures from the git ls-files/grep producer are explicitly captured and checked
before mapfile consumes its output. Preserve the NUL-delimited file list and
ensure the script exits or reports failure when producer status is nonzero,
rather than continuing with an incomplete set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 22a6e68c-5674-40be-9469-fd9fdf9e2823
📒 Files selected for processing (3)
crates/rustyn64-frontend/src/lib.rscrates/rustyn64-frontend/src/present_buffer.rsscripts/check_en_us.sh
Adopted from the second Antigravity review of #206. `pack` takes three positional slot ids in field order, so the calls the transitions needed -- `pack(front, back, ready)` to publish and `pack(ready, front, back)` to take -- read like transpositions of pack's own parameters. The hazard is that such a slip is SILENT: every permutation of {0,1,2} satisfies the disjointness invariant, so `the_slot_index_stays_a_permutation` keeps passing while the wrong slot is handed over. This project has already paid for one transposed positional-argument call this cycle (the TEX_BLOCK base/step swap), so the concern is not hypothetical. swap_ready_back / swap_front_ready are const fns over the same `pack`, so the bit layout is unchanged and the port stays structurally comparable to the RustySNES original. Pinned by the_named_transitions_swap_exactly_one_pair, which asserts each transition moves exactly one pair and leaves the third field alone -- mutation-checked by transposing swap_ready_back's first two arguments, which takes that test and five others red. Gates: fmt, clippy -D warnings, cargo test --workspace, rustdoc -D warnings, check_en_us.sh -- all green.
Adjudication of the second Antigravity reviewBlocking issues: none — the
|
…e must fail closed Both findings from the CodeRabbit review of #206, both adopted. reset() left rom_loaded and paused untouched -------------------------------------------- The doc said "reset to the empty state" and the body did not reach it, which is this repo's recurring comment-versus-code split. This REVERSES a choice made one commit earlier -- and pinned in a test -- that the two flags were the caller's to republish. The review's reasoning is better: the failure directions are not symmetric. Clearing them costs at most one UI frame of "no ROM" right after a load, which the next publish corrects. NOT clearing them leaves a closed ROM reading as loaded until something publishes again, and with the emu-thread feature off nothing ever does. A stale positive outlives a stale negative, so the gate clears all four and status() now equals PresentStatus::default() after a reset. The test asserts that whole equality rather than field-by-field. The en-US gate could still report PASS over zero files ------------------------------------------------------ `mapfile -d '' -t files < <(producer)` discards the producer's exit status, and `set -euo pipefail` cannot see inside a process substitution -- so a failing `git ls-files` produced an empty array and the script then reported success over nothing. MEASURED against the previous commit's script with a stub `git` whose `ls-files` exits 128: en-US check passed: 0 files (tracked + untracked), no en-GB or malformed spellings. exit=0 This is the SECOND false PASS in this one file -- the first was the missing `--others`, which hid new files. Same lesson each time: a gate whose failure mode is a silent pass is worse than no gate. The listing now goes through a file so each `git ls-files` is a simple command `set -e` can see, and an empty result is treated as a broken gate rather than a clean tree. The grep filters stay status-tolerant on purpose (`grep -v` exits 1 when it selects nothing, which is not an error), so the emptiness check is what carries the guarantee. Verified three ways: normal run passes over 444 files; a `git` that fails exits 128; a `git` that succeeds while returning nothing exits 1 with an explicit "the file listing is empty" diagnostic. Gates: fmt, clippy -D warnings, cargo test --workspace, rustdoc -D warnings, check_en_us.sh, markdownlint -- all green.
Adopted from the third Antigravity review of #206. `publish` established the (bytes, dims) pair that this whole type exists to keep consistent, and checked nothing -- so an inconsistent pair was invisible here and would surface far away as an out-of-bounds slice in whatever presents `out[..w * h * 4]`. It found a real bug immediately: publishing_twice_between_takes_yields_the_newest offered 8 bytes for (4, 1), a 16-byte frame. That test had been passing since the port, and an eyeball audit of the nine publish call sites in this file missed it -- which is the argument for the assert rather than against it. Kept as a debug_assert, not a release branch. The release alternatives are to drop the frame silently or to clamp the geometry, and both hide the caller's bug while producing a wrong picture. The release-mode defense belongs at the consumer, which will bound its upload by the bytes it actually received when the present path is wired up. The expected length is computed with saturating_mul so the check itself cannot overflow on a 32-bit usize. The should_panic test is #[cfg(debug_assertions)] -- without that gate it would fail under `cargo test --release`, since the assert compiles out. Verified: the release run passes with the test absent rather than failing. Gates: fmt, clippy -D warnings, cargo test --workspace, rustdoc -D warnings, check_en_us.sh -- all green.
Adjudication of the third Antigravity reviewBlocking:
|
Antigravity review (Gemini via Ultra)Ports a decoupled triple-buffer lock-free/short-mutex SPSC framebuffer handoff ( Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
… pacer yield (GUI stall, steps 0b+0c) (#208) * perf(frontend): take the present path off the emu mutex, and make the pacer yield Steps 0b and 0c of the GUI-stall fix. #206 added the PresentBuffer; this wires it in and replaces the pacer. Two independent halves of one defect, and fixing only the first would have left the emu thread hogging the mutex in a tight loop. 0b -- the UI stops taking the emu mutex --------------------------------------- App::snapshot took the emu mutex every UI frame merely to clone the framebuffer, while the emu thread held that same mutex across an entire emulated frame. It is replaced by App::take_present, which reads the handoff and takes no emu lock at all. The only emu-lock takers left on the UI thread are the menu actions that genuinely mutate the core, which happen on a click rather than per frame. Producers publish through one path, EmuCore::publish_into, so the emu-thread and the emu-thread-off build cannot drift about what a published frame is. It sends only the w*h*4 prefix, not the 1,228,800-byte FB_MAX backing store that produce_frame never resizes. The consumer bounds its upload by the bytes it actually received. That is the release-mode half of the contract PresentBuffer::publish debug-asserts, so a short buffer is skipped rather than sliced out of bounds. 0c -- the pacer --------------- The old fell-behind branch was `next = now` with no sleep and no yield. This core is ~6.5x slower than real time, so it was taken every iteration: the thread re-locked the emu mutex immediately and held it ~100% of the time. Ported from RustyNES: a bounded catch-up burst then a snap forward, then block_until_native -- capped 2 ms naps down to a 2 ms margin, then a precise spin. The nap cap is load-bearing; with one long sleep an OS oversleep blows past the target and the spin never engages. The pacing decisions are extracted into a `Schedule` so they are testable with synthetic instants rather than a real clock -- no timing flake. Deviation from the port: the emu lock is taken PER FRAME, not across a whole burst, because an N64 frame costs this core ~100 ms and holding across three would be a ~300 ms UI stall. Measured -------- A new #[ignore]d stopwatch times a competing UI thread's read both ways against a live emu thread (release, 120 samples, development machine): UI read via the handoff p50 894 ns p99 163 us max 163 us UI read via the emu mutex p50 97.8 ms p99 113.6 ms max 118 ms The same run reported 60 snap-forwards over 60 produced frames -- the core is behind on every iteration, which confirms directly that the old no-yield branch was always the one taken. Known cost, recorded rather than tuned away: because the core is behind every iteration, the snap imposes a full period of wait after each frame, so the effective rate is 1/(frame_cost + period) -- about 8.1 FPS against a ~9.3 FPS core ceiling, ~13%. That is the ported behavior and it is what buys the UI its window. Whether a shorter yield is better is a question for the perf work with measured data, not a constant to adjust by feel. Thread-priority elevation is deliberately not ported: it is the only unsafe in RustyNES's frontend and should wait for measurement. Both new guards are mutation-checked: removing the catch-up cap makes frames_due report 225,001; removing the publish_into call takes the end-to-end test red. Determinism (ADR 0004) is untouched -- this moves where already-produced bytes are copied and when wall-clock waits happen, neither of which the core observes. Gates: fmt, clippy -D warnings, cargo test --workspace, rustdoc -D warnings, check_en_us.sh, markdownlint, no_std thumbv7em -- all green. * fix(frontend): blanking the handoff must blank the presented frame too Adopted from the Antigravity review of #208, and it is another instance of this repo's recurring failure mode: the comment I wrote at the reset site claimed the window would "show black until the new ROM produces", and the code did not do it. Two independent traps, and each one alone leaves the previous ROM's last frame on screen: 1. PresentBuffer::reset clears the HANDOFF, but take_into then returns None, so App::frame_staging keeps its old bytes and keeps being uploaded. This is what the review found. 2. Merely EMPTYING frame_staging is also not enough, because the upload is then skipped -- and Gfx::upload_framebuffer writes into a persistent wgpu texture that render() keeps sampling. The stale image would stay on the GPU. So the new App::blank_presentation fills the staging buffer with actual black at the default geometry, which the next upload writes over the texture, and it is a named helper rather than three call sites precisely so the two halves cannot drift apart again. load_rom and dispatch take &mut self to reach it. Also documents that SPIN_MARGIN and SLEEP_CHUNK being equal is coincidental -- they answer different questions and the port defines them separately -- so nobody collapses them into one constant. Gates: fmt, clippy -D warnings, cargo test --workspace, rustdoc -D warnings, check_en_us.sh -- all green. Smoke-tested by running the release binary against tests/roms/homebrew/render_fill.z64 for 12 s: no panic, clean exit on SIGTERM. * fix(frontend): blank the presentation even when the ROM load fails Adopted from the CodeRabbit and Antigravity reviews of #208. A failed ROM load skipped the blanking entirely via the `?`, and that is not harmless: EmuCore::load_rom performs its warm reset BEFORE the fallible boot, so a rejected image has already discarded the previous machine. The UI was left showing the old ROM's picture, describing a core that no longer exists. The load now blanks unconditionally and propagates the error afterwards. Also from the reviews: - publish_into gained a debug_assert on the bytes/dims guard. It stays tolerant in release, but presentable_geometry makes the guard unreachable today, so a trip is a core-side bug that deserves a panic in development rather than a silently frozen picture. - blank_presentation uses saturating_mul, matching the other two framebuffer length computations. The defaults cannot overflow; one form everywhere is cheaper to audit than three. - docs/frontend.md: two missing commas and en dashes for the two ranges. Gates: fmt, clippy -D warnings, cargo test --workspace, rustdoc -D warnings, check_en_us.sh, markdownlint -- all green.
1 participant
Motivation — a user-reported, measured GUI stall
Reported symptoms: menu clicks taking 15-45 seconds, roughly one presented frame every 30-60 seconds, Super Mario 64 showing a frame then hanging, and audio cycling ~1 s on / ~1 s off.
Diagnosed to three separable defects, of which this PR lands the primitive for the first.
1. Frontend lock starvation — the catastrophic one
emu_thread.rs:70-80holds the emu mutex across its entirecoordinator.step():Its comment says "under a brief lock." It is the whole frame. And when it falls behind it resets the phase and re-acquires immediately — so it holds the mutex ~100% of the time in a tight loop.
app.rs:144 snapshot()needs that same lock every UI frame merely to clone the framebuffer, so the UI thread starves on an unfair mutex.2. The core is independently ~6.5x too slow
Free measurement from the real-PIF corpus census: 462 s of emulated time took 2,985 s wall in release — ~9.3 FPS equivalent. So even with the lock fixed, that is the ceiling. Tracked as task #55.
3. Audio hiccup is probably downstream
The ~1 s period matches the
AudioRingcapacity filling in bursts and draining to empty — what a starved producer looks like. Fixing it before #1/#2 would be tuning against a starved producer rather than a real defect.What this PR does — a port, not an invention
The sibling projects already solve #1, and
RustySNES/.../present_buffer.rs(itself ported from RustyNES) documents this exact bug in its module header:RustyN64 shipped the
emu-threadfeature without the handoff, so it does precisely what that module documents as wrong. Ported here — same author throughout, so licence-clean.Triple-buffer SPSC: the producer writes the slot neither side reads (
back) and publishes by swappingback<->ready; the consumer swapsfront<->ready. The packed index's three 2-bit fields stay a permutation of{0,1,2}, so producer and consumer always touch disjoint slots, and the small dedicated mutex is held only for one memcpy — never across emulation.N64-specific adaptations:
FB_LENis the PAL worst case (720x576) and is a sizing hint only — the N64's scan-out is variable (625x237 typical NTSC), so slots resize per publish rather than zero-padding a small frame up.H_VIDEO = 0mid-VI-reprogram — exactly the mismatch a separately-queried dims read produces.master_ticks, loaded, paused) publish beside the frame as plain atomics, because they were the other thingsnapshot()took the emu lock for. Decoupling only the bytes would have left the starvation in place for the sake of a status line.Determinism (ADR 0004) unaffected: this moves where already-produced deterministic bytes are copied. No emulated state, no audio.
Tests
Six, each aimed at a way the index could be wrong rather than at coverage:
publish_then_take_roundtripsa_take_with_nothing_new_leaves_the_previous_frameNonetake clobberingout(would flash black)publishing_twice_between_takes_yields_the_newestdims_travel_with_their_bytes_across_a_size_changethe_slot_index_stays_a_permutationreset_clears_the_published_stateDeliberately NOT wired in
This lands the primitive with its tests so the behavior-changing half — emu_thread publishing outside the lock,
app.rsceasing to take the emu lock, and the missing yield when behind — is reviewable on its own rather than buried under a 350-line port. The exact wiring recipe (with the verifiedEmuCoreaccessor names) is recorded in task #54 so it is not re-derived.Next after that: RustyNES's
perf_log.rs(588 lines) before any core optimization, since this project's rule is profile-before-optimize and #55 must not be guessed at; thenresampler.rsfor the audio; then thecargo full-build/full-runaliases (task #57 — RustyN64 has no.cargo/config.tomlat all).Gates
cargo fmt --all --check·cargo clippy --workspace --all-targets -- -D warnings·cargo test --workspace·RUSTDOCFLAGS="-D warnings" cargo doc·check_en_us.sh— all green.🤖 Generated with Claude Code