perf(core): wire fast-exec through the scheduler — 1.53x on a real frame - #233
Conversation
Step 3 of task #64, and the first number from ADR 0013's second execution mode. `System::run_until_exec` inverts who sets the pace: the CPU executes one instruction, reports what it cost in PCycles, `master_ticks` advances by that many CPU periods, and the RCP runs every one of its edges in the span that just elapsed. The CPU no longer lands on a derived edge, which is the relaxation stated plainly. ADR 0006 STILL HOLDS. `master_ticks` is still the only incremented counter and every other position is still derived from it; what changed is how far it moves per step, not who owns it. MEASURED, A-B-A in one sitting, Super Mario 64, 120 timed frames: A 100.582 / 99.847 B 64.822 / 65.321 A 99.712 / 99.598 The legs do not overlap — every A above 99.5 ms, every B below 65.4, while the four A readings span 0.99%, the ordinary within-session spread. Conservative pairing (best A over worst B): 99.598 -> 65.321 ms, **1.525x**, 10.04 -> 15.31 FPS. The return legs matter for the usual reason: two legs would have reported 1.55x from A1, and this project has been wrong that way before. Note the accurate baseline is ~99.6 ms, not the 93.06 ms recorded earlier — that figure was measured WITH `fast-scheduler` on, and its accurate pairing was 98.12 ms. Comparing a featured build against an unfeatured one is the mistake the note in docs/performance.md exists to prevent. THE DIVERGENCE IS MEASURED AND RECORDED, as ADR 0013 section 4 requires: the two modes retire 173,254,496 vs 171,471,972 instructions over the same 120 frames, +1.04%. Ledger C-16 carries the method, why it is above zero (the load-delay interlock the accurate path charges and this does not), and what would move it. Three design points, each documented where it lives: - It can land PAST `target`, by at most one instruction's cost, because a cost is only known after the instruction has run. Nothing drifts: the next call's target is absolute. - A HALTED CPU advances on RCP edges. A failed real-PIF boot checksum freezes the CPU while the RCP keeps running; with no instruction to time the advance, the loop steps to the next RCP edge. Deliberately NOT a bail-out — ADR 0011 section 6 sanctions a test-only seam only where a boundary genuinely cannot be reached, and this one can simply be handled. - `fast-exec` therefore adds NO new BailOut variant. Saying so is better than inventing an exit to justify the machinery. `run_frame` now picks one of three entry points, with fast-exec taking precedence where both features are on (ADR 0013 section 1), settled here rather than left to whichever cfg is written first. All four configurations build. `full` still excludes both, because promoting an execution mode into a shipped artifact is an ADR decision rather than a build-configuration one. CI gains five entries (core + frontend clippy, the both-features configuration, the core no_std build), because clippy runs exactly once and a cfg arm nobody compiles is a cfg arm that rots. Gates: fmt, clippy (workspace + every feature combination), test --workspace, the fast-exec CPU gate, the fast-scheduler differential gate, rustdoc, no_std, en-US, markdownlint. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 38 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 PR adds a feature-gated ChangesFast-exec execution mode
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant run_frame
participant System_run_until_exec
participant CPU
participant RCP
run_frame->>System_run_until_exec: execute frame target
System_run_until_exec->>CPU: step instruction
CPU-->>System_run_until_exec: return PCycle cost
System_run_until_exec->>RCP: process elapsed RCP edges
System_run_until_exec-->>run_frame: return FastRunReport
Possibly related PRs
🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 @.github/workflows/ci.yml:
- Around line 114-121: Add a CI step alongside the existing rustyn64-core clippy
command to run cargo test for rustyn64-core with the fast-exec feature enabled.
Ensure the test target covers System::run_until_exec, including RCP-edge,
overshoot, and halted-CPU paths; add feature-specific scheduler tests only if
those paths are not already covered.
In `@crates/rustyn64-core/src/scheduler.rs`:
- Around line 576-580: Make Scheduler::next_edge_after fallible when no
representable later edge exists, avoiding tick + 1 overflow at u64::MAX. Update
every caller, including the RCP loop using self.step_rcp, to stop processing
when edge discovery returns no value while preserving existing edge handling for
valid results.
- Around line 545-565: Update FastRunReport usage in scheduler.rs around
run_until_exec so blocks remains a count of whole scheduler periods, while
instruction execution increments a separately named instruction counter; update
all report construction and consumers as needed to preserve consistent public
units across run_until_fast and run_until_exec. In
crates/rustyn64-core/src/lib.rs lines 27-29, revise the module rustdoc to
document the explicit report units.
In `@docs/scheduler.md`:
- Around line 343-345: Update the comparison table’s fast-exec entry to state
that ADR 0006 remains unchanged, removing the claim about per-domain deficit
counters. Keep the surrounding documentation consistent with the implementation
and the later statement that master_ticks remains the sole incremented counter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 319c30d0-eda9-414c-83c9-03119bcba1b9
📒 Files selected for processing (9)
.github/workflows/ci.ymlcrates/rustyn64-core/Cargo.tomlcrates/rustyn64-core/src/lib.rscrates/rustyn64-core/src/scheduler.rscrates/rustyn64-frontend/Cargo.tomlcrates/rustyn64-frontend/src/emu.rsdocs/accuracy-ledger.mddocs/performance.mddocs/scheduler.md
…ccessor All four adopted; the third is the one worth reading. 1. CI LINTED THE CORE WIRING WITHOUT EXECUTING IT. `run_until_exec` had no test at all -- the CPU crate's differential gate cannot reach it. Added crates/rustyn64-core/tests/fast_exec_scheduler.rs (5 tests: engagement, the bounded overshoot swept across an RCP period, the no-op, the RCP catch-up, and both modes honoring the same target contract) plus the missing `cargo test -p rustyn64-core --features fast-exec` CI entry. 2. `FastRunReport::blocks` MEANT TWO DIFFERENT THINGS. `run_until_fast` counts whole edge periods; `run_until_exec` counted instructions. Same public field, two units, and a consumer could not read it consistently. Renamed to `work_units`, with a table naming the unit per mode and an explicit "do not compare across modes" -- it answers ADR 0012's *did it engage*, for which any positive count means the same thing, and a magnitude comparison between two units means nothing. 3. THE RCP CATCH-UP TEST WAS VACUOUS, and mutation is what showed it. The first version asserted `System::rcp_cycles` advanced. Deleting the entire RCP catch-up loop left it GREEN -- because `rcp_cycles` is a DERIVED ACCESSOR off `master_ticks` (ADR 0006's whole point), so it advances whether or not a single RCP step ran. The witness has to be RETAINED state. `VI_V_CURRENT` is incremented by `Vi::tick`, which only `step_rcp` calls, so it stays put if the RCP never steps. The mutation now fails with exactly that message. The general lesson, recorded in the test: in a codebase that derives every position from one counter, most of the obvious progress indicators cannot witness that work happened. Ask what is STORED, not what is REPORTED. 4. RCP EDGE-SEARCH OVERFLOW. `saturating_add` could put `end` at u64::MAX WITHOUT the machine ever having run there, and `next_edge_after` then evaluates `tick + 1` at u64::MAX -- a panic in debug, and in release a wrap to a low tick that keeps `rcp <= end` true forever. That is the difference from the accurate loop, whose top-of-range is unreachable because reaching it means emulating three thousand years. Now `checked_mul` + `checked_add`, and overflow ends the run. Also: docs/scheduler.md's comparison table claimed `fast-exec` amends ADR 0006 with per-domain deficit counters. The implementation has none -- that is the deficit-counter scheduler, still ahead. Corrected to say ADR 0006 is unchanged AS IMPLEMENTED, with the authorization falling due when the counters arrive. The halted-CPU branch is still untested, and the test file records why rather than leaving a hole: `boot_nmi_halt` is latched only by a real-PIF boot with a failing IPL2 checksum, which needs images that are never committed. A test-only seam on `Bus` for one three-line branch is not what ADR 0011 section 6 sanctions. Gates: fmt, clippy (workspace + every feature), test --workspace, both feature test suites, the CPU gate, rustdoc, no_std, en-US, markdownlint. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Antigravity flagged a hang if `step_instruction_at` ever returns 0: `end` stays at `master_ticks`, no RCP edge steps, and the outer loop spins. It is NOT reachable today -- `Pipeline::step_instruction` charges one PCycle to issue before adding anything -- but that guarantee lives in ANOTHER CRATE, and a reader of this loop cannot see it. The hazard is the remoteness, not the absence, and the finding is right on those grounds. A `debug_assert` is the real check. The `max(1)` beside it is a floor so a release build makes progress rather than hanging; it is deliberately not a fix, since a zero cost is a defect either way, but running one PCycle fast is a far better way to report a defect than a hang. Also adds the `cargo test -p rustyn64-frontend --features fast-exec` CI entry. The core one and the FastRunReport unit documentation were already in 10dadcd, which the review predates. Gates: fmt, clippy (workspace + every feature), test --workspace, both feature suites, the frontend feature suite, rustdoc, en-US, markdownlint. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Adjudication of the Antigravity (Gemini) review. One blocking finding, three suggestions — answered in BLOCKING — infinite loop on a zero-cost instruction. Adopted, and the finding is right for a reason slightly different from the one given. A Now: debug_assert!(cost > 0, "an instruction cost 0 PCycles, which cannot happen");
let Some(end) = u64::from(cost.max(1)) …The Suggestion — CI missing Suggestion — Nitpick — no tests validating RCP stepping and termination. Already adopted in |
Antigravity review (Gemini via Ultra)Wires the Blocking issues
Suggestions
Nitpicks
Automated first-pass review by |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/rustyn64-core/src/scheduler.rs (1)
585-602: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRCP catch-up loop can still overflow
next_edge_afteratu64::MAX.The
checked_mul/checked_addguard onend(lines 585-590) is correct, but it only bounds the outer computation.endcan legitimately equalu64::MAX. Whenrcp == end == u64::MAX, the loop body at line 601 still callsSelf::next_edge_after(rcp, ...), which computestick + 1unchecked and overflows — a panic in debug, a wrap to a low tick in release.The comment on lines 598-600 states this "cannot be asked for an edge past
u64::MAX" because "the loop condition fails at or before it." That claim is incorrect: thewhile rcp <= endcondition is checked before the iteration that computes the overflowing value, not after it, sonext_edge_after(u64::MAX, ...)is reachable whenend == u64::MAX. Treat the comment and the code as independent claims — the comment does not match what the code actually does.This is the same class of hazard the prior review raised for this function ("Prevent RCP edge-search overflow... Apply that contract to all callers of
next_edge_after"). That request was only partially completed:end's derivation is now checked, butnext_edge_afteritself remains unchecked at the point where it matters.As per path instructions, "A comment that asserts what the code does but disagrees with it... this project has been bitten by that four times and no test failed any of them."
🛡️ Proposed minimal guard
let mut rcp = Self::next_edge_after(self.master_ticks, self.phases.rcp, RCP_DIVIDER); while rcp <= end { self.master_ticks = rcp; self.step_rcp(); - // `end` is a real tick because it came from `checked_add` above, so - // this cannot be asked for an edge past `u64::MAX`: the loop - // condition fails at or before it. - rcp = Self::next_edge_after(rcp, self.phases.rcp, RCP_DIVIDER); + // `next_edge_after` overflows if asked for the edge after + // `u64::MAX` itself; stop rather than compute it. + if rcp == u64::MAX { + break; + } + rcp = Self::next_edge_after(rcp, self.phases.rcp, RCP_DIVIDER); }🤖 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-core/src/scheduler.rs` around lines 585 - 602, Update the RCP catch-up loop in the scheduler around next_edge_after so it does not call that helper when rcp is already u64::MAX; terminate the loop after processing the final valid edge. Remove or revise the existing comment asserting overflow is impossible so it accurately describes the guard, while preserving normal catch-up behavior for all other ticks.Source: Path instructions
🤖 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.
Duplicate comments:
In `@crates/rustyn64-core/src/scheduler.rs`:
- Around line 585-602: Update the RCP catch-up loop in the scheduler around
next_edge_after so it does not call that helper when rcp is already u64::MAX;
terminate the loop after processing the final valid edge. Remove or revise the
existing comment asserting overflow is impossible so it accurately describes the
guard, while preserving normal catch-up behavior for all other ticks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2da1a839-f541-48cb-a1fb-6690932ea029
📒 Files selected for processing (6)
.github/workflows/ci.ymlcrates/rustyn64-core/src/fastpath.rscrates/rustyn64-core/src/scheduler.rscrates/rustyn64-core/tests/fast_exec_scheduler.rscrates/rustyn64-core/tests/fast_scheduler_differential.rsdocs/scheduler.md
1 participant
The number
1.53× on a real frame — the first result in this project that comes from changing what is computed rather than when.
A-B-A in one sitting,
main+ this branch, Super Mario 64, 120 timed frames after the VI comes up,examples/frame_bench.rs:--features fast-exec--features fast-execThe legs do not overlap. Every A is above 99.5 ms and every B below 65.4 ms, while the four A readings span 0.99% — the ordinary within-session spread this repo records elsewhere. The gap is thirty times that.
Conservative pairing (best A over worst B), as
docs/performance.mdquotes throughout:The return legs earn their keep: two legs would have reported 1.55× from A1, and "measure A-B-A, not before/after" is in this repo because a two-leg comparison has been wrong here before.
Why the accurate baseline reads ~99.6 ms and not the 93.06 ms recorded in
performance.md. That figure was measured withfast-schedulerenabled; its accurate pairing was 98.12 ms. So ~99.6 ms is that baseline plus cross-session drift, not a regression — and comparing a featured build against an unfeatured one is exactly the mistake the note now inperformance.mdexists to prevent.What the wiring does
System::run_until_execinverts who sets the pace. The CPU executes one instruction, reports what it cost inPCycles,master_ticksadvances by that many CPU periods, and the RCP runs every one of its edges in the span that just elapsed. The CPU no longer lands on a derived edge — that is the relaxation, stated plainly.ADR 0006 still holds.
master_ticksremains the only incremented counter and every other position is still derived from it. What changed is how far it moves per step, not who owns it.The divergence is measured, not asserted away
ADR 0013 §4 requires the divergence to be measured, bounded, and recorded before it ships. It is:
fast-execRecorded as C-16 in
docs/accuracy-ledger.mdwith its method, why it is above zero (the load-delay interlock the accurate path charges and this does not), what would narrow it, and when it must be re-measured. Both figures are stable to the digit across runs — the counter is deterministic, only the wall clock varies — so this is a reading, not a sample.Three design points
target, by at most one instruction's cost, because a cost is only known after the instruction has run. Nothing drifts: the next call's target is absolute, so an overshoot means less work next time.fast-exectherefore adds no newBailOutvariant. Saying so plainly is better than inventing an exit to justify the ADR 0012 machinery; the enumeration exists so that a real one cannot be added silently.Feature plumbing
run_framenow selects one of three entry points bycfg, withfast-exectaking precedence where both features are enabled — settled here per ADR 0013 §1 rather than left to whichevercfghappens to be written first. All four configurations build (neither, either, both).fullstill excludes both, because promoting an alternate execution mode into a shipped artifact is an ADR decision rather than a build-configuration one.CI gains five entries: core and frontend clippy with the feature, the both-features configuration, and the core
no_stdbuild. Clippy runs exactly once in this repo, so acfgarm nobody compiles is acfgarm that rots.Verification
One guarded conditional, no pipes:
cargo fmt --all --check;cargo clippy --workspace --all-targets -- -D warnings; the same for-p rustyn64-core,-p rustyn64-cpuand-p rustyn64-frontendwith--features fast-exec;cargo test --workspace;cargo test -p rustyn64-cpu --features fast-exec; thefast-schedulerdifferential gate;RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps; theno_stdbuilds;scripts/check_en_us.sh;pre-commit run markdownlint --all-files.The accurate path is untouched by this PR — it adds a second entry point and changes nothing the default build reaches.
What is still open
60 FPS needs 16.67 ms; at 65.3 ms this is 3.92× short. The CPU pipeline was the largest single bucket and has now been addressed, so what remains is in buckets that were never the biggest. The next measurement is a fresh profile of the
fast-execframe — the shares inperformance.mdwere taken on the accurate path and no longer describe what this build spends its time on. Guessing from stale shares is how this project has misallocated effort before.🤖 Generated with Claude Code