perf(rdp): skip the split-borrow move when the RDP needs no bus (1.157x)

[doublegate]

Motivation

Bus::rdp_tick took the whole Rdp out of the Bus with core::mem::take on every RCP step — 344 bytes read and written, plus a fresh Default written into the vacated slot — purely so tick could borrow its owner. #218 measured that shuffle at ~1.07 GB a frame.

On most steps the RDP is frozen, stalling, or looking at an empty command FIFO and needs no bus at all, so it bought nothing.

What changes

Rdp::tick_without_bus answers those cases from the struct's own fields, and Bus::rdp_tick only takes when it returns false:

pub fn rdp_tick(&mut self) {
    if self.rdp.tick_without_bus() { return; }
    let mut rdp = core::mem::take(&mut self.rdp);
    rdp.tick_with_bus(self);
    self.rdp = rdp;
}

The predicate comes from Rdp::tick's own early-outs rather than from intuition, and lives beside them — it is a statement about this chip's state machine, so if they change it changes with them, in the same file. Rdp::tick calls the same helper, so there is one implementation and the two cannot drift apart.

The partial-command case (cmd_end - cmd_current < len_bytes) stays on the far side of the split: its length comes from an opcode in RDRAM, so it genuinely needs the bus.

Measured — Super Mario 64, --release, two runs each

	before	after
frame	125.32 / 125.16 ms	108.18 / 108.27 ms
FPS	7.98	9.24
scan-out	7.88 ms	7.98 / 8.04 ms (untouched)

1.157x.

This beats its own predicted ceiling, which means the model was wrong

docs/performance.md derives 1.056x as the ceiling for the entire split-borrow — RDP and audio. This is the RDP half alone and it measured 1.157x.

A result that exceeds its ceiling is a broken model, not a windfall. The cause: the profile attributed only the read_via_copy / write_via_move intrinsics inside mem::replace, and not the cost of constructing the Rdp::default() that take writes into the vacated slot — which is inlined into rdp_tick and charged elsewhere in the attribution. The document now records that correction rather than quietly banking the win.

Verification

cargo fmt --all --check, cargo clippy --workspace --all-targets -- -D warnings, cargo test --workspace, cargo test --workspace --features test-roms (the Angrylion .rvec and VI conformance vectors, byte-for-byte), RUSTDOCFLAGS="-D warnings" cargo doc, the no_std build, check_en_us.sh, and markdownlint. All green.

A new test pins the stall countdown through the skip path, mutation-checked: moving the empty-FIFO check ahead of the stall turns it red, because a stalled RDP with nothing queued would then never count down and the pipeline would hang — which no other test in the suite notices.

One correction made on the way

An earlier revision of my own comment claimed the bus-using half had to be a separate method or stall would decrement twice. Mutation-testing disproved it: rdp_tick only reaches that half when tick_without_bus returned false, which implies stall == 0, so a second pass finds nothing to decrement. The split is a redundant-work optimization, not a correctness requirement, and the comment now says which — the claim had been written from the shape of a hazard rather than from the code.

[coderabbitai]

Warning

Review limit reached

@doublegate, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 12 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 0e9fcfc0-4eb6-407e-a549-1cf23256ddc0

📥 Commits

Reviewing files that changed from the base of the PR and between 0eec97b and 9453111.

📒 Files selected for processing (4)

• crates/rustyn64-core/src/bus.rs
• crates/rustyn64-rdp/src/lib.rs
• docs/performance.md
• docs/rdp.md

📝 Walkthrough

Summary by CodeRabbit

• Performance
  • Improved rendering pipeline performance by avoiding unnecessary RDP state handling during idle, stalled, or frozen conditions.
  • Reduced overhead by only performing bus-backed RDP command processing when required.
• Bug Fixes
  • Fixed RDP stall behaviour so the stall counter decrements exactly once per RCP step, preventing wraparound.
• Documentation
  • Updated RDP and performance documentation with the revised stepping approach, measured results, and benchmarking guidance.

Walkthrough

RDP ticking now separates bus-free early-outs from bus-backed command decoding. Bus::rdp_tick only moves RDP state when bus access is required. Tests cover early-out conditions and stall countdown timing, while documentation records revised performance measurements.

Changes

RDP split ticking

Layer / File(s)	Summary
RDP tick phases crates/rustyn64-rdp/src/lib.rs	NeedsBus gates RDRAM-backed decoding; freeze, XBUS, stall, and empty-FIFO conditions are handled by tick_without_bus(), with dedicated branch coverage.
Bus integration and stall timing crates/rustyn64-core/src/bus.rs	Bus::rdp_tick performs the bus-free phase before conditionally taking and restoring RDP state; a regression test verifies one decrement per stall tick.
Performance and implementation documentation docs/performance.md, docs/rdp.md	Documentation records corrected modelling, measured timing, A-B-A inline results, and the updated RDP stepping flow.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Bus
  participant Rdp
  participant RDRAM
  Bus->>Rdp: tick_without_bus()
  alt bus access required
    Rdp-->>Bus: NeedsBus
    Bus->>Rdp: tick_with_bus(NeedsBus, Bus)
    Rdp->>RDRAM: read command words
    RDRAM-->>Rdp: command data
  else early-out
    Rdp-->>Bus: no bus access
  end

Loading

Possibly related PRs

• doublegate/RustyN64#56: Changes RDP FIFO decoding and stall behaviour in the same Rdp::tick path.
• doublegate/RustyN64#57: Changes RDP stall and tick gating behaviour intersecting the refactored countdown flow.
• doublegate/RustyN64#44: Changes freeze handling in the RDP tick path.

🚥 Pre-merge checks | ✅ 9 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Changelog Entry For User-Visible Changes	⚠️ Warning	FAIL: this PR's user-visible RDP/API change touched no CHANGELOG.md; the [Unreleased] block has no entry for it.	Add an [Unreleased] CHANGELOG entry for the public RDP bus-split/token change before merging.

✅ Passed checks (9 passed)

Check name	Status	Explanation
Title check	✅ Passed	It follows the Conventional Commits pattern, stays within 72 characters, and matches the perf optimisation.
Description check	✅ Passed	The description is clearly about the same RDP bus-splitting optimisation and performance validation.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Oracle Number Is Stated	✅ Passed	PASS: docs/rdp.md says the split-borrow change’s oracle effect was not measured and could not change the n64-systemtest count.
Docs-As-Spec Sync	✅ Passed	PASS: the RDP behaviour change in crates/rustyn64-rdp and bus.rs has matching docs/rdp.md and docs/performance.md updates, and the PR body explains the docs rationale.
Measured, Never Tuned	✅ Passed	PASS: the new hardware timings (25/50/33 GCLK) are cited to N64brew, and the ms figures are host benchmarks documented in docs/performance.md, which explicitly excludes them from the accuracy ledger.
Unsafe Stays Out Of The Chip Crates	✅ Passed	Core and every chip crate still have #![forbid(unsafe_code)], and the repo scan found no actual Rust unsafe constructs or frontend unsafe blocks.

---

_{Comment @coderabbitai help to get the list of available commands.}

[doublegate]

All three adopted.

1. "tick_with_bus is pub and relies on unstated preconditions; add debug_assert!" — ADOPTED, and it is the right catch for a method that crossed a crate boundary carrying invariants only its comment mentioned.

Three asserts now — unfrozen, stall == 0, non-empty FIFO — each naming the fix (call tick_without_bus first). Without them, an out-of-order call would decode a command out of an empty FIFO in silence, which is the failure mode worth making loud.

Mutation-checked: removing the tick_without_bus guard from Bus::rdp_tick makes nine tests fail on those messages, so they are reachable rather than decorative.

2. "Remove the historical narrative about an earlier incorrect comment" — ADOPTED as compression.

You are right that a doc comment describes what is. What I kept is the sentence that documents current behavior — that the split is an optimization, not a correctness requirement, because reaching the bus half implies stall == 0 — since without it the next reader may "restore" a correctness constraint that does not exist. The account of how I got it wrong belongs in the commit that corrected it, and stays there.

3. Nitpick: same for the test docstring — ADOPTED. Down to the two facts a maintainer needs: the stall is the only thing the skip path mutates, and checking the FIFO first would leave a stalled RDP counting down forever — which no vector notices, because it changes when a command retires rather than whether it matches.

Gates re-run: clippy -D warnings, cargo test --workspace, --features test-roms (Angrylion .rvec + VI vectors), rustdoc. All green.

[doublegate]

Round two. Two adopted, one rejected — and the first is the best comment on this PR.

1. "debug_assert! compiles out; in release an out-of-order call silently decodes from an empty FIFO" — ADOPTED.

Correct, and it is the difference between a diagnostic and a guarantee. The same three conditions are now a release guard as well: three predictable branches on fields already in cache, against a 344-byte move — not measurable, and not the redundant work this split removed.

It deliberately does not decrement stall. That belongs to tick_without_bus, and doing it in both is the one way this pair could burn two GCLK in a step — the hazard I wrongly claimed existed in an earlier revision, which putting a decrement here would have made real.

Measured after the guard: 105.67 / 105.39 ms against 108.18 / 108.27 before it. So it costs nothing — but it should not make the frame faster, and I have no mechanism for the 2.5%. That is recorded in docs/performance.md as unexplained and not counted; the conservative figure stays 108.22 ms. A speed-up without a mechanism is a coincidence waiting to be explained.

2. "Rename tick_without_bus to step_without_bus since it mutates" — REJECTED, on this codebase's vocabulary.

tick is the project's verb for advance this chip by one step — rsp_tick, rdp_tick, audio_tick, pi_tick, Cpu::tick_at. A tick_* method mutating is the expectation, not a surprise; step_without_bus beside tick and tick_with_bus would suggest the three do different kinds of thing.

Your underlying point is fair — a -> bool does read like a query. What carries that is the doc's first line, which says it returns true when it finished the step, and the stall decrement is called out explicitly two paragraphs down.

3. Nitpick: conversational tone — ADOPTED. "is now history" → "is superseded". I kept "turned out to be wrong", which is a plain statement about a model that was.

[doublegate]

Round three. Both adopted, and together they pointed at a better design than either patch.

1. "Debug and release disagree on a precondition failure; a stalled Rdp would hang in release" — ADOPTED, by removing the disagreement rather than choosing a side.

You are right on both counts: debug_assert panics, the release guard returns silently, and a caller who only ever calls tick_with_bus on a stalled RDP would never count stall down. My previous round created that inconsistency by adding the guard, so picking assert! or dropping the guard would have been choosing which of two wrong behaviours to keep.

The precondition is now carried by the type:

pub struct NeedsBus(());                                    // private field, no constructor
pub fn tick_without_bus(&mut self) -> Option<NeedsBus>;
pub fn tick_with_bus<B>(&mut self, _proof: NeedsBus, bus: &mut B);

Calling the halves out of order does not compile. There is no release-versus-debug behaviour left to reconcile, no silent hang, and no forged token — the field is private and the type has no constructor. The debug_asserts stay as executable documentation of what the token means; they are no longer the guarantee.

2. "Redundant branch evaluation in the hot path" — ADOPTED, and resolved by the same change: the release guard is gone, because the type now does its job.

---

This also settles the unexplained 2.5% from last round. Three builds now exist — no guard, release guard, token — which differ from each other in real work, and the last two both measure 105.5-105.8 ms against the first's 108.22. The delta tracks when the measurement was taken, not what was measured: it is cross-session drift, exactly what the method section warns about. docs/performance.md now says that instead of "no mechanism I can offer", and still quotes the conservative 108.22 ms.

Gates: clippy -D warnings, cargo test --workspace, --features test-roms (Angrylion .rvec + VI vectors), rustdoc, no_std, en-US, markdownlint. All green.

[doublegate]

Round four. All three adopted, and the second is a real defect that I have now made twice in the same file.

1. "The doc comments still say true/false but the signature returns Option<NeedsBus>" — ADOPTED. Now "returning None when it finished the step on its own and Some(NeedsBus) when work remains". A doc comment that contradicts its own signature is worse than none, because it is what a reader trusts before the type.

2. "The new test's doc comment attached itself to the preceding test's block" — ADOPTED, and thank you for catching it.

Exactly right: inserting before scanout_scaled_geometry_and_truncating_convert put my doc between that test's doc block and its #[test], so the older test silently lost its documentation to the newer one. Moved back.

Worth recording that I hit this same thing adding the memoization test in #216, fixed it there, and then reproduced it here. The mechanism is the trap: inserting before an anchor whose doc comment precedes it, where grepping for the anchor shows nothing wrong and every gate stays green. It is invisible to cargo test, to clippy, and to rustdoc, because both items still parse.

3. Nitpick: "the debug_assert!s are redundant now that NeedsBus cannot be constructed elsewhere" — ADOPTED, and it is more than redundancy.

They cannot fire. An assertion that cannot fire is dead code that reads like a safeguard — the inert-API hazard docs/engineering-lessons.md §3.2 describes, and the same shape as the ///-on-an-unnamed-item you caught on #218. I had justified them as "executable documentation" when they were the guarantee; the token made them neither. Removed, with the preconditions stated in prose where they belong.

Gates: clippy -D warnings, cargo test --workspace, --features test-roms, rustdoc, no_std. All green.

[doublegate]

Round five. Both adopted — and the reason there was a round five is worth naming.

Changing tick_without_bus from bool to Option<NeedsBus> left three prose references behind. Last round I fixed the one you pointed at and did not grep for the others, so the same defect came back in a different location. rg 'returns \false`'` would have found both in one pass; fixing the cited instance instead of the class is what cost the extra round.

1. tick_with_bus's docstring — now "once Rdp::tick_without_bus has handed back a NeedsBus".

2. docs/rdp.md — now says the stronger and more useful thing: Bus::rdp_tick takes when it gets a NeedsBus, and the bus half cannot be called out of order at all, because the token has no public constructor. "Only takes when it returns false" described a mechanism that no longer exists and understated the guarantee that replaced it.

Verified: zero remaining occurrences of a false return in either file. rustdoc -D warnings, clippy, en-US and markdownlint all green.

[doublegate]

Round six. One measured and rejected, one rejected structurally, one adopted.

1. "Consider #[inline] on tick_without_bus — cross-crate, hot path, depends on LTO" — MEASURED, and it is neutral. The measurement is more interesting than the answer.

leg		frame
A	without #[inline]	105.84 / 105.64 ms
B	with #[inline]	107.50 / 107.57 ms
A	without, again	107.35 / 107.45 ms

A before/after pair would have reported your suggestion as a 1.7% regression, and it is nothing of the kind — the third leg matches B, so the machine drifted ~1.7% slower partway through the sitting and stayed there. #[inline] is neutral, the expected answer under lto = "fat": LLVM already has the callee's body across the crate boundary, so the hint has nothing to add.

Not applied, per this repo's rule that anything measuring neutral is reverted rather than shipped. The A-B-A is now its own section in docs/performance.md, because the protocol is the transferable part: back-to-back runs agree to 0.05-0.13%, but a session drifts 1-2% over tens of minutes — the same size as a real optimization.

This is also the second independent "an inline hint does nothing here" result, after #[inline(always)] on the VI leaf readers made the scan-out 36% worse. The general form is now recorded: this workspace's release profile has already done the inlining.

2. "Evaluate whether the two-phase protocol should be pub or pub(crate)" — REJECTED; pub is structurally required.

rustyn64-core is a separate crate, and Rust has no cross-crate pub(crate). The chip crates exist as separate crates deliberately — the one-directional graph is what makes each independently fuzzable and benchmarkable — so anything the Bus calls has to be pub. Downstream consumers depend on rustyn64-core, which re-exports the chip types, rather than on rustyn64-rdp directly.

#[doc(hidden)] would hide the pair from rustdoc, but it hides rather than restricts, and NeedsBus is genuinely part of how a caller drives this chip — a reader of Rdp::tick_with_bus should be able to find out what the token means.

3. Nitpick: "rdp_tick's doc describes the take as happening on every RCP step — the pre-change behavior" — ADOPTED. It now says that is what it used to do and that the point of the code below is that it no longer does. A doc comment in the present tense describing behavior a change removed is the exact confusion worth avoiding.

[doublegate]

Round seven, all three adopted — and the middle one is the most valuable suggestion on this PR.

1. "Add direct unit tests for every early-out branch in rustyn64-rdp" — ADOPTED, and you are right that coverage through Bus::rdp_tick in another crate was not enough.

Each early-out is a reason a step can skip the 344-byte take, so:

• a branch that stopped firing is a silent performance regression;
• a branch that fires when it should not is a skipped command — a correctness regression.

Neither necessarily shows in the conformance vectors, as long as the totals happen to work out. All five cases are pinned in the RDP crate now: frozen (and not touching stall, since a frozen pipeline does not burn a GCLK), XBUS, stalling (one GCLK per step, twice), empty FIFO, and the queued-command case that genuinely needs the bus.

Mutation-checked: deleting the freeze/XBUS early-out turns it red.

2. Nitpick: "document that _proof is by value to enforce single-use" — ADOPTED, and it is the load-bearing detail. Relaxing NeedsBus to a reference would let a caller hold one and re-enter after the state it attested to had changed — which is the entire property the token buys. Now stated where a future refactor would read it.

3. "the answer here is true`` contradicts the Option return" — ADOPTED, the fourth such reference.

Worth naming why there was a fourth: last round I said the lesson was to fix the class rather than the cited instance, then grepped for returns `false` — one phrasing — and missed is `true`. So I repeated the exact mistake I had just written up, one round later. This time I grepped both `true` and `false` across all three files; the remaining hits are unrelated (Option-free booleans in the texture-mode decode).

Gates: clippy -D warnings, cargo test --workspace, --features test-roms, rustdoc, no_std, en-US. All green.

[coderabbitai]

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

docs/performance.md (1)

563-569: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Remove or relabel the superseded projections.

This table still presents 1.056x and the combined 1.098x/1.128x figures as current split-borrow projections, while the following section states that the model was wrong and the measured result is 1.157x. Mark these values explicitly as pre-measurement estimates or replace them with projections based on the measured result.

🤖 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 `@docs/performance.md` around lines 563 - 569, Update the performance
projections table in the “Both are isolated, and they compose” section to
clearly label the 1.056x, 1.098x, and 1.128x figures as pre-measurement
estimates, or replace them with projections derived from the measured 1.157x
split-borrow result. Ensure the table no longer presents superseded values as
current.

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.

Inline comments:
In `@crates/rustyn64-rdp/src/lib.rs`:
- Around line 1575-1577: Bind NeedsBus to the FIFO state it authorizes by
storing the validated position in the private token, and update the bus-dispatch
path around NeedsBus and tick_with_bus to reject tokens whose position no longer
matches the current runnable state before calculating command ranges. Ensure
stale proofs cannot dispatch after consuming a Sync Pipe or after DPC writes
invalidate cmd_current <= cmd_end, and add regression coverage for two proofs
followed by Sync Pipe then Sync Full.

In `@docs/performance.md`:
- Around line 624-628: Update the prose in the `#[inline]` discussion by adding
a comma after “Also”, so the sentence begins “Also, `#[inline]` on
`Rdp::tick_without_bus`”.

In `@docs/rdp.md`:
- Around line 851-862: Update the evidence paragraph describing the RDP stepping
optimization to cite the direct RDP early-out and queued-command tests plus the
Angrylion .rvec suite. Remove VI conformance vectors from evidence for the
scheduler split, and retain them only with scan-out verification.

---

Outside diff comments:
In `@docs/performance.md`:
- Around line 563-569: Update the performance projections table in the “Both are
isolated, and they compose” section to clearly label the 1.056x, 1.098x, and
1.128x figures as pre-measurement estimates, or replace them with projections
derived from the measured 1.157x split-borrow result. Ensure the table no longer
presents superseded values as current.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 3a96d026-57c6-4bab-8024-1136f792602b

📥 Commits

Reviewing files that changed from the base of the PR and between b6916d5 and 143b58e.

📒 Files selected for processing (4)

• crates/rustyn64-core/src/bus.rs
• crates/rustyn64-rdp/src/lib.rs
• docs/performance.md
• docs/rdp.md

[doublegate]

Round eight. One adopted, one rejected on idiom — and I checked the rejection rather than assuming it.

1. "Replace _proof with proof plus an explicit drop(proof)" — REJECTED, on idiom rather than feasibility.

I expected clippy::drop_non_drop to make this un-adoptable, since NeedsBus is a ZST with no Drop impl — so I tried it. It passes -D warnings cleanly, and the suggestion is available. Rejecting it is a judgment call, not a constraint:

• _proof is the idiomatic Rust signal for a parameter that is consumed but unread, and every Rust reader parses it that way.
• drop() of a type with no Drop impl is a no-op that reads as though something happens at that point. In a method whose whole subject is when work does and does not occur, that is the wrong kind of noise.
• The semantics the suggestion wants made explicit are now stated in the doc, one line above the signature, including why it is by value and not by reference.

2. Nitpick: "note that Copy/Clone are intentionally omitted" — ADOPTED, and it is the natural companion to the by-value note.

Either derive reopens exactly the hole taking the token by reference would: a caller could keep it past the step it authorized and present it again after the state it attested to had changed. NeedsBus's doc now says so, so a future refactor reaching for #[derive(Clone)] reads the reason first. Debug stays — it cannot duplicate the value.

Gates green: clippy -D warnings, rustdoc, cargo test --workspace.

[coderabbitai]

♻️ Duplicate comments (1)

crates/rustyn64-rdp/src/lib.rs (1)

1575-1582: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Bind NeedsBus to the state it authorises.

This remains the previously reported issue: omitting Copy/Clone prevents duplication, but not retention or multiple proofs. A caller can obtain two tokens, consume one command with the first, then reuse the second after cmd_current, cmd_end, or stall has changed. Because tick_with_bus() ignores the token and repeats none of the early-out checks, it can dispatch while stalled or evaluate cmd_end - cmd_current after the FIFO state has become invalid.

Store the validated FIFO state in NeedsBus and reject stale tokens before reading RDRAM; add the two-proof regression described in the earlier review. This is required for the stated compile-time ordering contract.

Also applies to: 1844-1862, 1877-1887

🤖 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-rdp/src/lib.rs` around lines 1575 - 1582, Bind NeedsBus to
the validated FIFO state by storing the relevant cmd_current, cmd_end, and stall
values when the token is created, then have tick_with_bus verify that state
before reading RDRAM or dispatching. Reject stale tokens, including a second
token obtained before the first command is consumed, while preserving the
existing early-out behavior for invalid FIFO or stalled states. Add the
two-proof regression covering reuse of the second token after the first token
changes the state; update the NeedsBus construction and tick_with_bus call sites
consistently.

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-rdp/src/lib.rs`:
- Around line 1575-1582: Bind NeedsBus to the validated FIFO state by storing
the relevant cmd_current, cmd_end, and stall values when the token is created,
then have tick_with_bus verify that state before reading RDRAM or dispatching.
Reject stale tokens, including a second token obtained before the first command
is consumed, while preserving the existing early-out behavior for invalid FIFO
or stalled states. Add the two-proof regression covering reuse of the second
token after the first token changes the state; update the NeedsBus construction
and tick_with_bus call sites consistently.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: caf0b6da-40dd-416e-866f-ab24ea1a1283

📥 Commits

Reviewing files that changed from the base of the PR and between 143b58e and 0eec97b.

📒 Files selected for processing (1)

• crates/rustyn64-rdp/src/lib.rs

[doublegate]

Round nine. All three adopted — with the second's premise corrected, because the code says otherwise.

1. "tick_without_bus sounds like an inspection method but mutates" — ADOPTED as an explicit note.

Second round to stop on this name, so the summary now says it outright: this advances state, burning one GCLK on the stalling path. Still tick_* rather than is_* or try_step_*, because tick is this codebase's verb for advance by one step — Bus::rsp_tick, Cpu::tick_at, audio_tick — and renaming one of a trio (tick, tick_without_bus, tick_with_bus) would suggest the three do different kinds of thing.

2. "If a caller drops the token, self.stall has already been decremented without completing the step" — the concern is real; that specific invariant is not.

The token is handed out only on the final path, after stall > 0 has already returned None. So stall == 0 whenever a NeedsBus exists: the decrement and the token are mutually exclusive, and a dropped token cannot leave a half-applied stall.

What it does cost is the step's work — the RDP does not advance, and the next rdp_tick starts the same step over. Nothing is corrupted; a step is lost. That is now documented on NeedsBus, in those terms, with #[must_use] named as what makes ignoring one loud.

Worth having asked: the type carries a real operational invariant and it was undocumented. The version now written is the one the code supports.

3. Nitpick: "hardcoding Rdp is 344 bytes goes stale" — ADOPTED.

Especially here: Rdp is #[non_exhaustive] and its own doc says the render state grows every sprint, so an exact figure in a neighbouring crate's comment is a stale number waiting to happen. Now "was 344 bytes when this was measured and grows most sprints", with the measurement itself living in docs/performance.md where it is dated.

Note the contrast with Latch in #218, which is pinned by a const assert: that struct's size is load-bearing for a published breakdown and does not grow by design. Rdp's does.

[doublegate]

Round ten. Your first item caught a claim of mine that was outright false, and the second was right that my test proved only half of what it needed to.

1. "#[must_use] on NeedsBus does not propagate through Option<NeedsBus>" — CORRECT, and I had documented the opposite.

I wrote on NeedsBus that "#[must_use] is what makes ignoring one loud". Probed it — a discarding call, rdp.tick_without_bus();:

• before: cargo clippy --all-targets reports nothing. No lint at all.
• after moving the attribute to the function: warning: unused return value of Rdp::tick_without_bus that must be used.

So the guarantee I documented did not exist. Two reasons, and I had the second wrong in my head: an attribute on T does not propagate through Option<T>, and Option — unlike Result — is not #[must_use] itself.

#[must_use] is now on the function, with a message naming the consequence, and the doc says which attribute does the work and why the obvious one does not.

2. "The test only exercises the early-out; add one for a complete command through tick_without_bus returning Some" — ADOPTED, and the gap was real.

The stall test proves the skip path fires. On its own that is satisfied by a predicate that always skips — a dead RDP that renders nothing. Only the conformance vectors would have caught that, and only if they happen to cover it.

New test drives a Sync Pipe (0x27) through Bus::rdp_tick end to end: command consumed (commands_processed incremented), cmd_current advanced past it, documented stall applied. Mutation-checked by making the predicate always skip — it turns red.

3. Nitpick: "remove the leading underscore from _proof" — held, third raising.

_proof is the idiomatic signal for consumed but unread. The stated risk — that it "weakens compiler verification if the function body is ever modified" — inverts: a future body that uses the token renames it as part of using it, and the unused-variable lint has nothing to catch in a body that correctly ignores a proof-of-precondition.

Gates: clippy -D warnings, cargo test --workspace, --features test-roms, rustdoc, no_std. All green.

[doublegate]

Round eleven. One adopted, two rejected — the second on a workspace setting that removes the failure mode.

1. "Mark NeedsBus #[non_exhaustive] as well" — REJECTED, it would add nothing.

#[non_exhaustive] stops downstream crates constructing with a literal and matching exhaustively. A tuple struct with a private field already does both: outside rustyn64-rdp you can neither write NeedsBus(()) nor destructure it, because the field is not nameable. The attribute is for types whose fields are public and may grow; this one has exactly one field that will never be public, since its whole purpose is to be unforgeable.

2. "A panic inside tick_with_bus leaves self.rdp as Default permanently" — REJECTED for this workspace, and it predates the change.

Correct as a general Rust observation, and worth raising. Two reasons it is not live here:

• panic = "abort" in the release profile (Cargo.toml), so there is no unwinding to observe the intermediate state through. The process ends.
• In test/debug builds a panic in the RDP fails the test that provoked it, and the Bus is dropped with it.

It is also not introduced by this PR — rdp_tick has had the take/restore shape since the split-borrow was written; this change makes it run less often. A scope guard would be the fix if the profile ever unwinds, and that is a decision about the profile rather than about this method.

3. Nitpick: "note that pointer wrapping is handled before cmd_end is set" — ADOPTED, with the actual reason.

Checked rather than asserted: these are RDRAM addresses, not ring indices. DPC_START / DPC_END are latched through DPC_ADDR_MASK in dpc_write, cmd_current only ever advances by a decoded command's length, and the hardware has no wrap — a driver that wants to restart writes DPC_START again. So there is no wrapped case for a plain >= to get wrong, and the comment now says so instead of leaving the next reader to re-derive it.

[doublegate]

Round twelve. Two adopted, one rejected — plus a red push of my own to report.

1. "Document whether Rdp::tick is deprecated or fallback-only" — ADOPTED. It is neither: it is the whole-step entry point, used by anything that already holds the bus and has nothing to decide. Only Bus::rdp_tick benefits from the split, because only it must know whether the step needs a bus before arranging one — and arranging one means taking the whole struct. Now stated, since "why does this wrapper still exist" is a fair question to ask of it.

2. Nitpick: "rdp_tick's doc duplicates benchmark detail from docs/performance.md" — ADOPTED. Trimmed to the contract plus a pointer. A doc comment should say what the method guarantees and where the measurement lives; copying dated figures into a second place is how the two disagree later.

3. "#[must_use] on NeedsBus is redundant" — held, and already explained in the type's own doc.

Correct that it does nothing today: the function's attribute is what fires, and the doc says exactly that, including why the obvious one does not. It stays because it is dormant rather than misleading — if the return shape ever changes to a bare NeedsBus, it activates and is correct. That is different from the inert debug_asserts I removed two rounds ago, which claimed to check something that could not happen.

---

Disclosure: I pushed a red commit. The doc I added in this round linked [Bus::rdp_tick] from inside rustyn64-rdp — Bus lives in rustyn64-core, which this crate must not depend on under the one-directional crate graph, so rustdoc -D warnings failed with an unresolved link.

It reached the branch because I ran the gates with ; separators and echoed their exit codes, then committed unconditionally — the exact failure CLAUDE.md describes, in a session where I had already cited that rule twice. Fixed in the follow-up, which ran every gate inside the conditional that guards the commit. It also illustrates why cross-crate intra-doc links are avoided here: the graph makes them unresolvable by construction.

[doublegate]

Round thirteen. Four adopted — and the first two are both cases of a document claiming more than the code earns.

1. CodeRabbit: "Separate RDP evidence from VI evidence" — ADOPTED, and it was a real overstatement.

docs/rdp.md said the split was "verified by the Angrylion .rvec and VI conformance vectors". The .vivec vectors feed the VI directly from RDRAM and never step the RDP — they would pass exactly as convincingly with the stepping entirely broken. Citing them made the evidence base look twice as wide as it is, and the paragraph immediately below already attributes them to the scan-out, so the doc was crediting one oracle to two claims.

Now: the .rvec suite (the oracle that does drive an RDP command list) plus the unit tests added in this PR, and an explicit sentence saying why the VI vectors are not evidence here. This is the same class as the vacuous-pass lesson in docs/engineering-lessons.md — a green oracle that never reached the code under test.

2. Antigravity: "dropping a token defers the step, it does not lose it" — ADOPTED, and correct on the mechanism.

The Some path mutates nothing: it is reached only once stall is zero, so no decrement has occurred, and no FIFO pointer moves until the bus half runs. The command is still pending; the next rdp_tick retries the identical step. "Abandons that step's work" was wrong.

Adopted with one sharpening, because "deferred/retried" understates it in the other direction: the RDP made no progress during that GCLK and is one cycle late from then on. That is a timing divergence with no wrong state anywhere — correct-but-late, which no state comparison can detect. It is the same failure shape ADR 0011 names for the fast-scheduler bailout invariant, so it is worth saying precisely rather than softening.

3. Antigravity nitpick: "doc comments name an outer-crate caller, coupling rustyn64-rdp to rustyn64-core" — ADOPTED in part.

The load-bearing sentence now leads with the pattern — a caller that cannot lend this struct without first moving it, so it must know whether the step needs a bus before paying for the move — with the Bus named parenthetically as the instance. The concrete name stays in Rdp::tick's doc, where the question being answered ("why does this wrapper still exist") has no useful generic answer.

Worth noting the premise is stronger than a style preference here: a link across that edge does not compile, which is what I pushed red last round. Plain prose does compile, so it goes stale silently instead — which is the worse of the two failure modes.

4. CodeRabbit nitpick: comma after a sentence-initial "Also" — adopted.

---

No functional code changed; the implementation has been fixed since round three. Gates run in one conditional, no pipes: fmt, clippy -D warnings, test --workspace, rustdoc -D warnings, check_en_us.sh, markdownlint — ALL-GATES-OK.

[github-actions]

Antigravity review (Gemini via Ultra)

This PR optimizes RDP stepping by splitting Rdp::tick into a bus-free phase (tick_without_bus) and a bus-dependent execution phase (tick_with_bus), avoiding expensive core::mem::take struct moves when the RDP is idle, frozen, or stalling.

Blocking issues

None found.

Suggestions

• crates/rustyn64-rdp/src/lib.rs (lines 1588–1592): The doc comment for NeedsBus claims that #[must_use] on a type does not propagate through Option<T>. In modern Rust compilers, type-level #[must_use] does propagate through standard library wrappers like Option and Result. Keeping #[must_use] on tick_without_bus is still good practice, but the comment rationale is inaccurate for current Rust.
• crates/rustyn64-core/src/bus.rs (line 543): rdp_tick still performs core::mem::take(&mut self.rdp) on active steps, which constructs Rdp::default() in the vacated slot. If Rdp grows in size or acquires heap-allocated members in future iterations, this default instantiation on active cycles could become a secondary hotspot. Consider wrapping rdp in Option<Rdp> on Bus if Rdp::default() complexity increases.

Nitpicks

• crates/rustyn64-rdp/src/lib.rs (line 1845): Rdp::tick retains its full bus: &mut B signature despite delegating to tick_without_bus() first. While reasonable for test convenience, consider adding #[inline] to tick so callers outside bus.rs don't pay redundant wrapper overhead.

_{Automated first-pass review by agy on a self-hosted runner -- not a human review.}