perf(frontend): take the present path off the emu mutex, and make the pacer yield (GUI stall, steps 0b+0c) - #208
Conversation
… 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.
|
Warning Review limit reached
Next review available in: 36 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 (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe frontend replaces mutex-protected framebuffer snapshots with a ChangesFrontend presentation handoff
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant EmuThread
participant EmuCore
participant PresentBuffer
participant App
participant Renderer
EmuThread->>EmuCore: step emulation frame
EmuCore->>PresentBuffer: publish video and status
App->>PresentBuffer: take latest presentation
App->>Renderer: upload staged framebuffer
Possibly related PRs
🚥 Pre-merge checks | ✅ 6 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (6 passed)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/rustyn64-frontend/src/app.rs (1)
231-231: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale doc wording: "snapshot" no longer exists.
The unchanged doc comment above
redrawstill reads "input -> snapshot -> egui pass...", butApp::snapshotwas replaced bytake_presentin this PR. Worth a one-word update while touching this function.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/rustyn64-frontend/src/app.rs` at line 231, Update the doc comment above App::redraw to replace the stale “snapshot” stage with “take_present,” matching the current frame pipeline and the renamed method.
🤖 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/app.rs`:
- Around line 143-146: Update App’s presentation reset flow by adding or using
reset_presentation to reset the handoff and stage a zero-filled framebuffer at
the default dimensions, rather than only clearing PresentBuffer or clearing the
staging buffer. In load_rom, ensure this presentation reset occurs even when
core.load_rom fails, while preserving the error return; update the CloseRom and
Reset dispatch arms to call reset_presentation and widen dispatch mutability as
needed. Keep framebuffer upload behavior intact so the staged blank frame
replaces the old GPU texture.
In `@docs/frontend.md`:
- Around line 48-96: The documentation contains minor grammar and typography
issues: add the comma after “time” before “Against,” replace the hyphens in
“15-45” and “30-60” with en dashes, and add the comma before “and” in the
“ported behavior” sentence. Limit changes to these wording fixes in the defect
and known-cost sections.
---
Outside diff comments:
In `@crates/rustyn64-frontend/src/app.rs`:
- Line 231: Update the doc comment above App::redraw to replace the stale
“snapshot” stage with “take_present,” matching the current frame pipeline and
the renamed method.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 56f3dfa0-20d8-46c9-b059-34f9d2c3241a
📒 Files selected for processing (4)
crates/rustyn64-frontend/src/app.rscrates/rustyn64-frontend/src/emu.rscrates/rustyn64-frontend/src/emu_thread.rsdocs/frontend.md
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.
Adjudication of the Antigravity reviewBlocking: stale framebuffer after ROM load / reset — ADOPTED (1bb378a)Correct, and worth naming what kind of mistake it was: the comment I wrote at that Your diagnosis is exact: One correction to the prescribed fix, though — The fix is Suggestion: status is read independently of the frame — REJECTED, it would make the flags stale when they matterThe skew is real but bounded and already accepted: Gating the status update on a successful Suggestion:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/rustyn64-frontend/src/emu_thread.rs (1)
245-263: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not charge a poisoned
emu.lock()as a produced frame. If the mutex is poisoned,coordinator.stepandcore.publish_intonever run, butschedule.advance()andproduced += 1still do. That lets the pacer report progress while the published frame stays stale. Break the thread or recover the poisoned guard before counting the frame.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/rustyn64-frontend/src/emu_thread.rs` around lines 245 - 263, The emulation loop must not advance pacing or count a frame when acquiring the emulator mutex fails due to poisoning. Update the flow around the emu.lock() call so a poisoned lock either terminates the thread or recovers a valid guard and completes the frame work before reaching schedule.advance() and produced += 1; preserve counting only for successfully processed frames.crates/rustyn64-frontend/src/app.rs (1)
6-15: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winScope the mutex claim to the emulation-thread build.
The new documentation says mutex access is limited to menu actions, but the
#[cfg(not(feature = "emu-thread"))]redraw path still locksself.emuon every frame at Lines 264-272. Document both execution modes instead of asserting a global contract.As per path instructions, comments that disagree with the implementation are correctness hazards. Based on learnings, documentation does not enforce behaviour and this mismatch must be corrected.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/rustyn64-frontend/src/app.rs` around lines 6 - 15, The module documentation incorrectly makes a global mutex-access claim while the non-emu-thread redraw path still locks self.emu each frame. Update the documentation near App::snapshot to distinguish the emu-thread mode from the #[cfg(not(feature = "emu-thread"))] mode, accurately describing mutex usage in each execution path without changing behavior.Sources: Path instructions, Learnings
🤖 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.
Outside diff comments:
In `@crates/rustyn64-frontend/src/app.rs`:
- Around line 6-15: The module documentation incorrectly makes a global
mutex-access claim while the non-emu-thread redraw path still locks self.emu
each frame. Update the documentation near App::snapshot to distinguish the
emu-thread mode from the #[cfg(not(feature = "emu-thread"))] mode, accurately
describing mutex usage in each execution path without changing behavior.
In `@crates/rustyn64-frontend/src/emu_thread.rs`:
- Around line 245-263: The emulation loop must not advance pacing or count a
frame when acquiring the emulator mutex fails due to poisoning. Update the flow
around the emu.lock() call so a poisoned lock either terminates the thread or
recovers a valid guard and completes the frame work before reaching
schedule.advance() and produced += 1; preserve counting only for successfully
processed frames.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 80c4928f-e349-4a65-a5a6-0dbc159e7d84
📒 Files selected for processing (2)
crates/rustyn64-frontend/src/app.rscrates/rustyn64-frontend/src/emu_thread.rs
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.
Adjudication of the third Antigravity reviewBlocking:
|
Antigravity review (Gemini via Ultra)Decouples the frontend presentation path from the emulator core mutex by reading frames and status from a Blocking issues
Suggestions
Nitpicks
Automated first-pass review by |
1 participant
Motivation
Steps 0b and 0c of the GUI-stall fix. #206 added the
PresentBuffer; thiswires it in and replaces the pacer.
The reported symptom was that the emulator is unusable: menu clicks taking 15-45
seconds, roughly one frame presented per 30-60 seconds, audio cycling ~1 s on /
~1 s off. That turned out to be two independent defects in the frontend, and
fixing only the first would have left the emu thread hogging the mutex in a tight
loop:
App::snapshottook the emu mutex every UI frame merely to clone theframebuffer, while the emu thread held that same mutex across an entire
emulated frame.
next = nowwith no sleep and no yield.This core is ~6.5x slower than real time, so it was taken on every iteration
— the thread re-locked immediately and held the mutex ~100% of the time.
Measured
A new
#[ignore]d stopwatch(
measure_ui_read_latency_through_the_handoff_versus_the_emu_mutex) times acompeting UI thread's read both ways against a live emu thread.
--release, 120samples, development machine:
The same run reported 60 snap-forwards over 60 produced frames — the core is
behind on every iteration, confirming directly that the old no-yield branch was
always the one taken.
Changes
0b — the UI stops taking the emu mutex
App::snapshot->App::take_present: reads the handoff, takes no emu lock.The only emu-lock takers left on the UI thread are the menu actions that
genuinely mutate the core (open / close / pause / reset) — on a click, not per
frame.
EmuCore::publish_intois the single producer path, so the emu-thread and theemu-thread-off build cannot drift about what a published frame is. It publishesonly the
w * h * 4prefix, not the 1,228,800-byteFB_MAXbacking store thatproduce_framenever resizes.release-mode half of the contract
PresentBuffer::publishdebug-asserts.present.reset()on ROM load, CloseRom and Reset; deliberately not onTogglePause, since a pause must keep showing the frame it paused on.
EmuThread::spawntakes anEmuThreadParamsstruct: the eighth argument wouldhave tripped
clippy::too_many_arguments.0c — the pacer
Ported from RustyNES: a bounded catch-up burst (
MAX_CATCHUP_FRAMES = 3) then asnap forward, then
block_until_native— capped 2 ms naps down to a 2 msmargin, 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
Scheduleso they are testable withsynthetic 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. An N64 frame costs this core ~100 ms, so holding across three would be
a ~300 ms UI stall.
Known cost, recorded rather than tuned away
Because the core is behind every iteration, the snap imposes a full frame period of
wait after each frame, so the effective rate is
1 / (frame_cost + period)— about8.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
unsafein RustyNES's frontend and should wait for measurement.
What this does NOT fix
The core is still ~6.5x slower than real time. That is a separate, real defect
(P1), and it should not be attacked until
perf.rssays where the time goes.The audio hiccup (P2) is most likely downstream of producer starvation and
should be re-measured after this, not tuned against a starved producer.
Determinism (ADR 0004)
Untouched. This moves where already-produced bytes are copied and when
wall-clock waits happen — neither of which the deterministic core observes.
Verification
Both new guards are mutation-checked:
frames_duereport 225,001 frames due;publish_intocall takes the end-to-end test red.Gates run locally:
cargo fmt --all --check,cargo clippy --workspace --all-targets -- -D warnings,cargo test --workspace,RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps,scripts/check_en_us.sh,pre-commit run markdownlint --all-files, and theno_stdthumbv7em build — allgreen.
docs/frontend.mdis updated in the same change (the defect, the measurementtable, the pacer, and the known cost).