docs(perf): anatomize the remaining in-model targets, and pin Latch's size in code - #218
Conversation
…bly-empty bytes ADR 0011 recorded the latch copies as ~16% of runtime and "not safely removable", from a perf annotate view. Per-LINE attribution of the post-6a6adfa capture, plus size_of on the struct, says something more specific. Six copy sites in pipeline.rs total 14.66%, and Latch is 120 bytes whose fields sum to exactly 120 -- so 0011's zero-padding claim holds. But 72 of those bytes are write_back, mem and cop0, all produced at EX. In ic_rf and rf_ex they are structurally always empty, so those two latches copy 72 bytes of nothing, twice each per cycle: 6.01% of the frame. That is NOT the hazard 0011 ruled out. That one is specific to the DC path, where dc_stage's error branch re-reads ex_dc after abort_with stamps it. The upstream pair carries no such entanglement. Recorded as an untested hypothesis with an upper bound of 1.064x, and deliberately not acted on: it is worth doing after the dispatch question is settled rather than instead of it, and any attempt needs the golden-log 0-diff and n64-systemtest, not just cargo test -- this repository records four pipeline changes that compiled, passed everything, and did nothing.
|
Warning Review limit reached
Next review available in: 39 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 (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds compile-time layout guards for ChangesPerformance analysis
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 8 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (8 passed)
Comment |
core::mem::replace is 5.32% of the frame on its own (mem/mod.rs:930 at 4.21%, :929 at 1.11%), about 6.7 ms of 125, and all of it is rdp_tick and audio_tick taking their chip out and putting it back. Rdp is 344 bytes and Audio is 88, size_of-measured, so each tick touches ~3x size_of and the scheduler runs both EVERY RCP step -- ~1.04 M a frame, or ~1.35 GB moved to satisfy the borrow checker. The fix pattern is already here: Bus::rsp_tick used to do this and no longer does, and its comment records that the take was worse than the "no allocation" claim above it -- take needs Default, and constructing an Rsp allocated 8 KiB of DMEM and IMEM every RCP step. Rdp and Audio were never converted. Recorded with both candidate routes as hypotheses and an upper bound of 1.056x. Not acted on: the "take only when there is work" route turns on an exact idle predicate, which is a claim about the RDP state machine rather than about performance, and the Angrylion vectors plus the audio goldens are what would catch it being wrong.
…e padding question Antigravity review of PR #218. Line numbers drift, so each site now carries its enclosing function and its statement, with the line marked as being from the capture. Verifying those attributions found two of my own wrong: 2061 is in ex_stage rather than dc_stage, and 913 is in dc_stage rather than wb_stage. Grouped by stage the shares are ex_stage 6.26%, dc_stage 4.97%, ic_stage 2.15%, rf_stage 1.28%. On padding: size_of::<Latch>() INCLUDES alignment padding and the component figures are each size_of of that component, so their summing to the whole is exactly the evidence that none is left over -- the six scalars occupy 30 bytes, which is also their naive sum, so the two bools sit in gaps rather than being padded out. There is no field ordering that makes this struct smaller.
|
All three adopted — and checking the first one found two attributions of mine were wrong. 1. "Hardcoded line numbers will drift; reference function or stage names" — ADOPTED, and it caught a defect. Each site now carries its enclosing function and its statement, with the line number marked as being from the capture. Verifying those function names against the file found two of mine were wrong: 2. "The field table sums to 120 without accounting for alignment padding" — ADOPTED, and the summing is the padding evidence. Stated explicitly now: 3. Nitpick: "separate the line numbers into their own column" — ADOPTED, they have one. Also added since you last saw this branch: a second measured section on the Bus split-borrow. |
Antigravity round two on PR #218. The reviewer asks whether 1.064x and 1.056x are cumulative or isolated -- they are isolated, and they address disjoint shares (6.01% and 5.32%), so collecting both entirely gives 1/(1-0.1133) = 1.128x: 125.24 ms down to ~111 ms, or 9.0 FPS, against a 7.5x gap. Saying so makes the case for sequencing them after the dispatch question stronger than leaving two unrelated-looking numbers on the page. Also disambiguates that mem/mod.rs is the Rust standard library inside the toolchain rather than a file in this repository, and swaps a unicode ellipsis inside a code span for ASCII.
|
Round two, all three adopted — and the nitpick was the most useful of them. 1. "Unicode ellipsis inside a code span" — ADOPTED, ASCII 2. "Disambiguate whether 3. Nitpick: "are 1.064x and 1.056x cumulative or isolated?" — ADOPTED, and composing them sharpens the section's whole point. Isolated, and they address disjoint shares — 6.01% for the latch payload, 5.32% for the split-borrow — so both collected entirely give Against a 7.5x gap. Two unrelated-looking numbers on a page invite someone to hope they add up to something; stated together they make the sequencing argument themselves — these are worth doing after the dispatch question, not instead of it. |
Antigravity round three on PR #218: the latch table lost its filename when the sites were regrouped by function, and the split-borrow section named its functions without saying which file they are in. Both carry repo-relative paths now, and rsp_tick -- the one already converted -- is located too, since it is the thing a reader would want to open next.
|
Round three, both adopted. Both are the same defect in two places, and it was introduced by adopting your previous round. 1. "The split-borrow section omits repo-relative paths for 2. Nitpick: "the stage table omits the filename" — ADOPTED. Worth noting how that one happened: the table had the path, in the column header, and lost it when I restructured the columns to adopt your round-one suggestion about function names. A tidy-up that quietly drops information is the failure mode of tidy-ups, and it is the second time in this batch that adopting a suggestion cost something elsewhere — both caught by the next round, which is a reasonable argument for the loop. |
…e ones with nothing in them The section named the two concentrated targets and left a reader to wonder about the RSP's 11.4%. Per-line it has no target: the hottest attributable line is 0.65% and the largest entry is 1.17% of inlined code with no line, so it is thinly spread instruction execution -- which is why ADR 0011 scopes the fast path to the VR4300 first. Recording the null result is what makes the enumeration a claim rather than a selection.
|
One addition since the last round, unprompted, because the section was making a claim it had not earned. It named the two concentrated targets — the latch payload and the split-borrow — and said nothing about the RSP's 11.4%, leaving a reader to assume it had been skipped. Per line, it has no target: the hottest attributable line is 0.65%, and the largest single entry is 1.17% of inlined code with no line at all. It is thinly spread instruction execution, which is exactly why ADR 0011 scopes the fast path to the VR4300 first and leaves the RSP for later. The remainder of the CPU's 41.0%, once the 14.66% of latch copying is set aside, is the same shape. The section now says so. Recording the null result is what turns "here are the two targets" into "every bucket above 3% was examined and these two are what is in them" — a claim rather than a selection, and the difference matters for a document whose job is to stop the next person re-deriving this. |
…ing conflated Antigravity round four on PR #218, and the reviewer caught an inconsistency of mine: the text said the split cuts the two upstream copies by 60% -- the share of bytes those latches carry for nothing -- and then quoted 1.064x, which assumes the whole 6.01% disappears. Those are different claims. 60% of 6.01% is 3.61%, so the EXPECTED gain is 1.037x. The 1.064x is the ceiling if those copies vanished outright, which nothing proposed here achieves, since the front half still has to move. Both are stated, labelled, and composed with the split-borrow figure in a table: expected 1.098x (~114 ms, 8.8 FPS), ceiling 1.128x (~111 ms, 9.0 FPS). Against a 7.5x gap either way. The snippet also notes that take needs Rdp: Default and writes a fresh default in place, which is why it is a read AND a write rather than a move.
|
Round four. Two adopted, one rejected — and the first is a real inconsistency of mine. 1. "The text says a 60% cut but the bound assumes 100% elimination" — ADOPTED. They were two different claims wearing one number. The 60% is the share of bytes those two latches carry for nothing (72 of 120). The 1.064x assumed the whole 6.01% disappears. Both are now stated and labelled, with your arithmetic:
Composed with the split-borrow figure in a table rather than in prose:
Against a 7.5x gap either way — which is the conclusion, and it survives the correction. But an optimistic figure standing next to the mechanism that contradicts it is exactly what this document exists to prevent. 2. "The snippet does not indicate 3. Nitpick: "replace exact line numbers with offsets or function names" — REJECTED, they already are, and the numbers earn their place. Every site carries its enclosing function, its statement, and its file, adopted from your round-one and round-three suggestions; the line number is an extra column explicitly marked "at capture". Removing it would break the one thing it is for: re-deriving these shares from the retained |
…uition Antigravity round five on PR #218 asked which RDP state an idle predicate must validate. Reading Rdp::tick's own early-outs answers it exactly: the tick returns having touched nothing in two cases, both pure reads of self -- status & (FREEZE|XBUS) != 0, and cmd_current >= cmd_end. The other two do not qualify and it matters why. stall > 0 decrements stall, so it is not idle -- but it does not need the bus either, so it can be handled before the take rather than being a reason to keep it. The partially-written multi-word case is NOT decidable without the bus, because it reads the opcode from RDRAM to learn the length, so that one must still take. Audio::tick is different: it writes self.last_tick = now unconditionally on entry, so it is never idle by this definition and needs the return-a-request route instead. That is the kind of asymmetry a predicate written from intuition would have missed. Also marks the bus.rs line numbers as drifting, and says the 41.0% is the whole CPU share including the latch copies.
|
Round five, all three adopted — and the second turned a hand-wave into an actual specification. 1. "Note that the 2. "Document the specific RDP state a sound idle predicate must validate" — ADOPTED, and reading The tick returns having touched nothing at all in two cases, and both are pure reads of
The other two early-outs do not qualify, and why matters more than the fact:
And the asymmetry a predicate written from intuition would have missed: Your framing is why that got derived rather than asserted — "which state must be validated" is answerable from the code, and "is this risky" is not. 3. Nitpick: "is 41.0% inclusive or exclusive of latch copies?" — ADOPTED. Inclusive: it is the whole |
…the code at it Antigravity round six on PR #218. A stdlib line number is only citable because this workspace pins an EXACT toolchain -- 1.96.0, per rust-toolchain.toml -- so the section now says which one, and names what is actually at 929-930: the read_via_copy / write_via_move pair inside replace. On any other toolchain, look for that pair rather than for the numbers.
|
Round six, adopted. "Specify the toolchain version alongside the stdlib line references" — ADOPTED, and it is the reason those numbers are citable at all. A stdlib line number would normally be useless in a document, and it is usable here only because this workspace pins an exact toolchain rather than a minimum — It also names what is actually at those lines, which is the part that survives a toolchain bump: let result = crate::intrinsics::read_via_copy(dest);
crate::intrinsics::write_via_move(dest, src);the |
Antigravity has raised line-number drift three times, and the third framing -- reference a commit SHA so the numbers stay resolvable as permalinks -- is the one that settles it rather than mitigating it. They are the lines of 2abc817, the tree the profile was captured on, so they stay correct forever at that ref and the function names remain the durable part in main.
|
Round seven, adopted — and this framing is the one that settles the point rather than mitigating it. Line-number drift has come up three times now (rounds one, three, and this one). My previous answers added the enclosing function, then the filename, then a "will drift" disclaimer — all of which reduce the damage without removing it. Referencing the commit does remove it. The numbers are now stated as those of The function names stay as the durable pointer for someone reading current That is the last change I plan to make here. Every comment across seven rounds has been adjudicated and answered, CI is green, and the document now says: where the frame goes after the memo, the two in-model targets that remain with expected and ceiling figures for each, the exact idle predicate one of them needs derived from the code rather than guessed, and the null result for every other bucket above 3%. |
…t decay Antigravity round eight on PR #218, and the suggestion is right that a repr(Rust) layout claim in a document is only true of the toolchain that measured it. A const assert next to the struct makes it enforced instead: const _: () = assert!(core::mem::size_of::<Latch>() == 120, ...); Mutation-checked -- changed to 121, cargo check fails with the message, restored. The message tells the next person to RE-MEASURE rather than change the number, because either a field was added or the layout algorithm moved, and the "no padding is wasted" conclusion needs re-deriving before it is re-quoted. Also answers the reviewer's question about the RDP command length: it is NOT cached. Rdp holds only cmd_start / cmd_current / cmd_end, and tick re-reads word0_hi from RDRAM every time, so the partial-command check genuinely needs the bus and that case must still take. Plus the crates/ prefix and the 4 KiB + 4 KiB split.
|
Round eight. All four adopted, and the first turns a documentation claim into an enforced one — the best suggestion this PR has had. 1. "
const _: () = assert!(
core::mem::size_of::<Latch>() == 120,
"Latch changed size; docs/performance.md's copy-cost breakdown must be re-measured"
);Mutation-checked, not assumed: changed to 121, The message is deliberately an instruction rather than a fact. If it fires, the fix is to re-measure, not to change the number — either a field was added, or the layout algorithm moved, and in the second case the "no padding is wasted" conclusion has to be re-derived before anyone re-quotes it. That is the difference between a guard and a speed bump. 2. "Verify whether the command length is already cached in
Worth asking, though: if it had been cached, the predicate would have covered a third case. 3. Nitpick: " 4. Nitpick: "clarify the 8 KiB as 4 KiB DMEM + 4 KiB IMEM" — ADOPTED, confirmed against Gates re-run after the code change: fmt, clippy |
doublegate
changed the title
…replace Antigravity round nine on PR #218. The section attributed 5.32% to core::mem::replace while the snippet below showed core::mem::take, with nothing saying why a reader looking at profile frames sees one and code the other: mem::take(x) IS mem::replace(x, Default::default()), and that default is where the second write comes from. Said outright. The read_via_copy / write_via_move pair now leads and the line numbers follow it, since the names are what survives a toolchain bump and the numbers are only quotable because this workspace pins an exact one.
|
Round nine. Two adopted, one rejected on repository convention. 1. "Rely primarily on function names rather than stdlib line numbers" — ADOPTED. The 2. "The narrative says Now stated outright: 3. Nitpick: "full commit SHA permalinks rather than plain short hashes" — REJECTED, on convention. Every commit reference in this file and in the ADRs is a bare short hash — That is where I stop pushing on this PR. Nine rounds, every comment adjudicated and answered, CI green. The document now records: where the frame goes after the memo, the two in-model targets that remain with expected and ceiling figures for each, the exact idle predicate one of them needs derived from |
Antigravity round ten asks whether the const assert breaks a 32-bit build. It does not, and the workspace already proves that rather than assuming it: the no_std gate builds this crate for thumbv7em-none-eabihf, a 32-bit target, and a divergence would fail it. Every field is fixed-width -- no usize, no references, no pointers -- so 120 holds on both widths. Deliberately not #[cfg]-gated on target_pointer_width, and the comment now says so: a gate would switch the guard OFF on exactly the target where a pointer-sized field would first change the answer, which is the opposite of what the suggestion is trying to protect.
|
Round ten. One adopted as an explanation, two rejected as already answered. 1. "The const assertion assumes 64-bit layout; consider It does not break a 32-bit build, and that is proven rather than assumed: the Gating it would be actively worse: 2 & 3. "Line numbers in tables / stdlib line numbers are fragile" — REJECTED, fourth and fifth time. Every durable form of this has been adopted across rounds one, three, seven and nine: each site carries its enclosing function, its statement, and its file; the stdlib reference now leads with What the numbers still buy is the one thing the names cannot: re-deriving these shares from the retained Stopping here, as stated last round. CI green, every comment across ten rounds adjudicated and answered. |
… assert Antigravity round eleven proposes #[repr(C)] as the way to get layout stability. Measured instead of argued: repr(C) lays fields out in declaration order, so the two bools can no longer occupy alignment gaps and Latch becomes 128 bytes rather than 120. On a struct copied four times per emulated cycle that is ~1.2 ms a frame ADDED, to exactly the copies this work is trying to shrink -- the suggestion would regress the thing it appears next to. The const assert already provides the stability it is reaching for, and costs nothing. Found by bisecting the assert against repr(C) applied temporarily, then reverting.
|
Round eleven. The "If deterministic layout and size preservation are required, annotate
Found by applying Eight bytes more, on a struct copied four times per emulated CPU cycle. That takes the copy share from 14.66% to ~15.6% — about +1.2 ms a frame, added to precisely the copies this PR exists to measure and shrink. The suggestion would regress the thing it appears next to. And the stability it is reaching for is already there: the Both the code comment and the document now record the 128, so the next person does not have to re-derive it. "Line numbers will drift" — REJECTED, sixth time. Every durable form has been adopted across rounds one, three, seven, nine and ten: enclosing function, statement, file, stdlib symbol names leading, and the numbers pinned to commit Stopping, as stated. Every comment across eleven rounds adjudicated; the last three have re-raised settled points and this one would have cost 1.2 ms a frame. |
![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 `@docs/performance.md`:
- Around line 494-502: Revise the prose in the performance documentation around
the split-borrow discussion: make the sentence beginning “At 125 ms a frame”
grammatically complete, and replace the non-idiomatic phrase “what it wants
done” with clearer wording while preserving the technical meaning about
Rsp::tick returning its requested operation.
- Around line 428-432: Correct the byte-breakdown explanation in the section
“What the byte breakdown adds to 0011's analysis” so it reconciles the three
cited sites (:2351, :2147, and :1903) with the stated copy count. Either
describe the exact three-copy path supported by the measurements or update the
cited rows and 6.01% figure to match four copies; keep the claim reproducible
and falsifiable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 77268d48-8b64-4d67-a8d1-98f5dca5a4a6
📒 Files selected for processing (2)
crates/rustyn64-cpu/src/pipeline.rsdocs/performance.md
| **What the byte breakdown adds to 0011's analysis.** The last three fields — **72 of the | ||
| 120 bytes** — are *produced at `EX`*. In `ic_rf` and `rf_ex` they are structurally always | ||
| `None`/`WriteBack::None`, so those two latches copy 72 bytes of provably-empty payload, | ||
| twice each per cycle. That is the `:2351`, `:2147`, and `:1903` rows above — **6.01%** of | ||
| the frame, moving nothing. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reconcile the copy count with the measured rows.
The table identifies three sites (:2351, :2147, and :1903) totalling 6.01%, but “twice each per cycle” implies four copies. State the exact three-copy path, or adjust the cited rows and percentage; otherwise the byte-volume explanation is not reproducible.
Suggested wording
- so those two latches copy 72 bytes of provably-empty payload, twice each per cycle.
+ so the three copy sites in the ic_rf → rf_ex → EX path each move 72 bytes of
+ provably empty payload per cycle.As per path instructions, measured claims must remain honest and falsifiable.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| **What the byte breakdown adds to 0011's analysis.** The last three fields — **72 of the | |
| 120 bytes** — are *produced at `EX`*. In `ic_rf` and `rf_ex` they are structurally always | |
| `None`/`WriteBack::None`, so those two latches copy 72 bytes of provably-empty payload, | |
| twice each per cycle. That is the `:2351`, `:2147`, and `:1903` rows above — **6.01%** of | |
| the frame, moving nothing. | |
| **What the byte breakdown adds to 0011's analysis.** The last three fields — **72 of the | |
| 120 bytes** — are *produced at `EX`*. In `ic_rf` and `rf_ex` they are structurally always | |
| `None`/`WriteBack::None`, so the three copy sites in the `ic_rf` → `rf_ex` → `EX` path each | |
| move 72 bytes of provably empty payload per cycle. That is the `:2351`, `:2147`, and | |
| ` :1903` rows above — **6.01%** of the frame, moving nothing. |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~430-~430: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ..., so those two latches copy 72 bytes of provably-empty payload, twice each per cycle. That is ...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
🤖 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 428 - 432, Correct the byte-breakdown
explanation in the section “What the byte breakdown adds to 0011's analysis” so
it reconciles the three cited sites (:2351, :2147, and :1903) with the stated
copy count. Either describe the exact three-copy path supported by the
measurements or update the cited rows and 6.01% figure to match four copies;
keep the claim reproducible and falsifiable.
Source: Path instructions
| At 125 ms a frame that is ~10.8 GB/s of memory traffic to satisfy the borrow checker, | ||
| which is consistent with the 5.32% the profile attributes to `core::mem`. | ||
|
|
||
| **The fix pattern is already in this repository.** `Bus::rsp_tick` (same file, line 519 | ||
| at that commit) | ||
| used to do exactly this and no longer does; its comment records that the `take` was worse than the "no | ||
| allocation" claim above it, because `take` needs `Default` and constructing an `Rsp` | ||
| allocated its 8 KiB of scratch — 4 KiB DMEM and 4 KiB IMEM — **every RCP step**. `Rsp::tick` now *returns* what it | ||
| wants done instead of borrowing its owner. `Rdp` and `Audio` were not converted. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Tighten the split-borrow prose.
“At 125 ms a frame that is…” is grammatically incomplete, and “what it wants done” is non-idiomatic.
Suggested wording
- At 125 ms a frame that is ~10.8 GB/s of memory traffic to satisfy the borrow checker,
+ At 125 ms per frame, that is ~10.8 GB/s of memory traffic to satisfy the borrow checker,
- `Rsp::tick` now *returns* what it wants done instead of borrowing its owner.
+ `Rsp::tick` now *returns a request* instead of borrowing its owner.Based on learnings, LanguageTool prose findings are independent of Markdown-structure checks.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| At 125 ms a frame that is ~10.8 GB/s of memory traffic to satisfy the borrow checker, | |
| which is consistent with the 5.32% the profile attributes to `core::mem`. | |
| **The fix pattern is already in this repository.** `Bus::rsp_tick` (same file, line 519 | |
| at that commit) | |
| used to do exactly this and no longer does; its comment records that the `take` was worse than the "no | |
| allocation" claim above it, because `take` needs `Default` and constructing an `Rsp` | |
| allocated its 8 KiB of scratch — 4 KiB DMEM and 4 KiB IMEM — **every RCP step**. `Rsp::tick` now *returns* what it | |
| wants done instead of borrowing its owner. `Rdp` and `Audio` were not converted. | |
| At 125 ms per frame, that is ~10.8 GB/s of memory traffic to satisfy the borrow checker, | |
| which is consistent with the 5.32% the profile attributes to `core::mem`. | |
| **The fix pattern is already in this repository.** `Bus::rsp_tick` (same file, line 519 | |
| at that commit) | |
| used to do exactly this and no longer does; its comment records that the `take` was worse than the "no | |
| allocation" claim above it, because `take` needs `Default` and constructing an `Rsp` | |
| allocated its 8 KiB of scratch — 4 KiB DMEM and 4 KiB IMEM — **every RCP step**. `Rsp::tick` now *returns a request* instead of borrowing its owner. `Rdp` and `Audio` were not converted. |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~494-~494: Possible missing comma found.
Context: ... | | total | ~1.35 GB | At 125 ms a frame that is ~10.8 GB/s of memory tr...
(AI_HYDRA_LEO_MISSING_COMMA)
[formatting] ~500-~500: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ... than the "no allocation" claim above it, because take needs Default and constructing...
(COMMA_BEFORE_BECAUSE)
[style] ~502-~502: The double modal “wants done” is nonstandard (only accepted in certain dialects). Consider “to be done”.
Context: ...Rsp::tick now returns what it wants done instead of borrowing its owner. Rdp a...
(NEEDS_FIXED)
🤖 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 494 - 502, Revise the prose in the
performance documentation around the split-borrow discussion: make the sentence
beginning “At 125 ms a frame” grammatically complete, and replace the
non-idiomatic phrase “what it wants done” with clearer wording while preserving
the technical meaning about Rsp::tick returning its requested operation.
Sources: Learnings, Linters/SAST tools
Antigravity round twelve: /// on `const _: () = assert!(...)` is inert -- rustdoc does not emit documentation for an unnamed item, so twenty-five lines of reasoning were addressed to a reader who would never see them. // now. A small instance of the inert-API hazard docs/engineering-lessons.md §3.2 describes: it compiled, the rustdoc gate passed, and the text was simply never going anywhere.
doublegate
changed the title
|
Round twelve. Both adopted — the second is a small instance of a hazard this repository has a lesson about. 1. " 2. " Twenty-five lines of reasoning — why the size matters, what to do if the assert fires, why it is not That is the shape |
|
Both items in this round were already applied in the commit that landed after the review ran: the doc comments became |
…6 is 108 Antigravity round thirteen was right and my previous answer was wrong. I verified the assert on thumbv7em-none-eabihf and generalised that to "32-bit", which does not follow: what varies is u64 ALIGNMENT, not pointer size. On thumbv7em and wasm32 u64 aligns to 8 and Latch is 120; on 32-bit x86 it aligns to 4 and Latch is 108 -- measured by bisecting the assert against i686-linux-android. So "every field is fixed-width, therefore the layout is width-independent" was a plausible-sounding inference from one confirming case, which is the shape this repository has a lesson about. The assert stays ungated, and the comment now says why in the corrected terms: all three supported targets (x86_64, thumbv7em, wasm32) are built in CI and all three are 120, and on a newly added ABI the assert firing is the guard WORKING -- that is exactly when the copy-cost breakdown has to be re-measured before it is re-quoted. A #[cfg] would make the new target the one place the numbers go unchecked.
|
Round thirteen. You are right and my previous answer was wrong. Measured. Last round I said the assert is safe on 32-bit because every field is fixed-width. I had verified that on Bisected the assert against
So "fixed-width fields ⇒ width-independent layout" was a plausible inference from one data point, which is the failure shape this repository keeps a lessons file about. Corrected in both the comment and the document, with the number. The assert stays ungated, and the reasoning changes rather than the code:
Second item — "mention the enclosing function names alongside line numbers" — they have been there since round one: the table's first column is the function ( |
Antigravity round fourteen's objection is fair: a hard build failure on i686,
with a message about a docs file, is a poor experience for someone who only
wants to cross-compile. Defending the guard was the wrong response; there is a
design that satisfies both sides.
Two assertions now, and neither breaks a cross-compile:
1. NO PADDING, universally -- size_of::<Latch>() equals the sum of its
components. That is the property the copy-cost breakdown actually rests on
("no field ordering would make this smaller"), and it is verified to hold on
x86_64, thumbv7em-none-eabihf, wasm32-unknown-unknown AND i686-linux-android.
2. 120 WHERE A u64 ALIGNS TO 8, written as align_of::<u64>() != 8 || size == 120
rather than as a #[cfg]. This pins the documented figure on every ABI the
figure describes and makes no claim on the ones it does not -- which is the
real mechanism, rather than pointer width.
Both mutation-checked: breaking either produces its own message.
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-cpu/src/pipeline.rs`:
- Around line 281-283: Correct the latch-copy comment near the inter-stage latch
to match the six measured copy sites documented in docs/performance.md: two in
EX, two in DC, one in RF, and one in IC. If the existing four-copy figure refers
to a narrower operation, explicitly define that scope instead of leaving the
broader claim ambiguous.
In `@docs/performance.md`:
- Around line 435-438: Update the measured timing claim in the section
discussing #[repr(C)] to include reproducible provenance: identify the target
hardware/platform, toolchain, benchmark or capture commit, and measurement
method used to obtain the 128-byte layout and approximately 1.2 ms/frame result.
Keep the existing comparison intact while ensuring the values are not presented
as unsupported facts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 257706bd-1446-4599-9c0a-8e481e458910
📒 Files selected for processing (2)
crates/rustyn64-cpu/src/pipeline.rsdocs/performance.md
| // The inter-stage latch is **copied four times per emulated CPU cycle**, so its size is | ||
| // a performance fact and not merely a layout detail: at ~1.56 M steps a frame those | ||
| // copies are ~15% of the frame (`docs/performance.md` §"The latch copies, anatomized"). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reconcile the latch-copy count with the measured sites.
The comment says the latch is copied four times per CPU cycle, but docs/performance.md identifies six copy sites: two in EX, two in DC, one in RF, and one in IC. Correct the count or explicitly define the narrower operation being counted.
As per path instructions, comments that contradict measured behaviour are correctness hazards. Based on learnings, documentation and comments must match the implementation.
🤖 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-cpu/src/pipeline.rs` around lines 281 - 283, Correct the
latch-copy comment near the inter-stage latch to match the six measured copy
sites documented in docs/performance.md: two in EX, two in DC, one in RF, and
one in IC. If the existing four-copy figure refers to a narrower operation,
explicitly define that scope instead of leaving the broader claim ambiguous.
Sources: Path instructions, Learnings
| **`#[repr(C)]` would be the wrong way to get that stability**, measured: it lays fields | ||
| out in declaration order, so the two `bool`s can no longer occupy alignment gaps and the | ||
| struct becomes **128 bytes**. On something copied four times per emulated cycle that adds | ||
| ~1.2 ms a frame — to the very copies this section is about. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add provenance for the measured timing claim.
“Measured” is not sufficient provenance for the 128-byte layout or the approximately 1.2 ms/frame cost. Record the target, toolchain, benchmark or capture commit, and measurement method so this value can be reproduced rather than treated as an unsupported fact.
As per coding guidelines, measured timing values require provenance and must not be presented as undocumented hardware or performance facts.
🧰 Tools
🪛 LanguageTool
[style] ~435-~435: To form a complete sentence, be sure to include a subject or ‘there’.
Context: ...wn must be re-measured. *#[repr(C)] would be the wrong way to get that stability...
(MISSING_IT_THERE)
[uncategorized] ~437-~437: Possible missing comma found.
Context: ...omething copied four times per emulated cycle that adds ~1.2 ms a frame — to the very...
(AI_HYDRA_LEO_MISSING_COMMA)
🤖 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 435 - 438, Update the measured timing claim
in the section discussing #[repr(C)] to include reproducible provenance:
identify the target hardware/platform, toolchain, benchmark or capture commit,
and measurement method used to obtain the 128-byte layout and approximately 1.2
ms/frame result. Keep the existing comparison intact while ensuring the values
are not presented as unsupported facts.
Source: Coding guidelines
|
Round fourteen. Your objection is fair and I stopped defending the guard. There is a design that satisfies both positions, and it is better than either. A hard build failure on Two assertions now, and neither can break a cross-compile:
Both mutation-checked: breaking either produces its own distinct message, and all four targets build clean. The result is strictly better than where this started. The universal invariant is the one that would actually catch a regression, and the previous single assert was pinning a number instead of a property — which is the weaker of the two things to protect. On the line numbers, sixth raising: the function names are the primary identifiers and have been since round one — the table's first column is |
Antigravity round fifteen. Both items checkable, both checked. The scalar term was literals -- 1 + 8 + 4 + 1 + 8 + 8 -- which desynchronizes if a field type changes and would then MASK the padding it exists to detect. It is size_of of each field's type now. The second asks whether keying the 120 on align_of::<u64>() == 8 could fail on a 32-bit ARM target, where u64 is 8-byte aligned but inner-type layout might differ. Built for armv7-linux-androideabi: it passes, i.e. Latch is 120 there. That is the exact combination the implication is keyed on -- 32-bit with an 8-byte-aligned u64 -- so it is the case that would have failed first, and the table now lists it as measured rather than assumed.
|
Round fifteen. Both adopted, both checkable, both checked. 1. " It would not merely go stale; it would mask the very padding the assert exists to detect — a wrong constant on both sides of an equality is indistinguishable from a correct one. It is now 2. " That is precisely the case that would have failed first — 32-bit with an 8-byte-aligned Current state, all built and passing:
Five targets, two asserts, no |
Antigravity review (Gemini via Ultra)This PR documents remaining performance bottlenecks in CPU pipeline latch copies and bus split-borrows, while adding compile-time assertions in Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
|
Round sixteen. Both rejected, and the second has a direct answer that I should have given earlier. 1. "The implication may fire on a future toolchain or a valid target" — that is the guard, not a defect. Yes, and deliberately: firing means re-measure before re-quoting, which is what its message says. What changed in round fourteen is the blast radius, which was your earlier objection and was fair — the universal no-padding assert now carries the property worth protecting on every ABI, and the 120 is claimed only where it is documented. Verified on five targets: A guard that cannot fire is decoration. The alternative — deleting it — is what leaves a stale layout claim in a document, which is the failure this whole PR is about. 2. "The manual type list can silently desynchronize if a field type changes" — it cannot be silent, and that is checkable. Consider the case you name: Demonstrated rather than argued: I mutated You are right that it is not type-checked — Rust has no stable way to take This is where I stop on this PR: sixteen rounds, every comment adjudicated and answered, CI green. The last several have re-raised settled points or asked for guarantees Rust cannot express, while the earlier ones materially improved it — the two-assert redesign, the five-target verification, and the correction of a false claim I had made all came from this thread. |
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 (1)
docs/performance.md (1)
420-429: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the stale assertion description.
The text still says
pipeline.rscontains an unconditionalsize_of::<Latch>() == 120assertion, but the implementation now has a universal component-size assertion plus a conditional 120-byte assertion based onalign_of::<u64>(). Describe both assertions accurately.🤖 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 420 - 429, Update the assertion description in the performance documentation to accurately reflect the implementation in pipeline.rs: mention the universal no-padding/component-size assertion and the conditional size_of::<Latch>() == 120 assertion that applies when align_of::<u64>() is 8, rather than describing the 120-byte check as unconditional.Sources: Coding guidelines, 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.
Outside diff comments:
In `@docs/performance.md`:
- Around line 420-429: Update the assertion description in the performance
documentation to accurately reflect the implementation in pipeline.rs: mention
the universal no-padding/component-size assertion and the conditional
size_of::<Latch>() == 120 assertion that applies when align_of::<u64>() is 8,
rather than describing the 120-byte check as unconditional.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9deaa04a-42a8-4ac1-8059-3aee0139a8e3
📒 Files selected for processing (2)
crates/rustyn64-cpu/src/pipeline.rsdocs/performance.md
1 participant
Motivation
ADR 0011 recorded the pipeline latch copies as "~16% of total runtime" and "not safely removable", with a 1.19x ceiling — from a
perf annotateview. That was enough to rule the target out, and not enough to say which copies were the problem.Per-line attribution of the post-
6a6adfarender capture, plussize_ofon the struct, says something more specific — and it changes what a future attempt should aim at.What the measurement shows
Six copy sites in
pipeline.rstotal 14.66% of the frame (plus 3.77% inlined with no line, so ~15–18% — about 19–22 ms of 125, more than the whole VI scan-out cost after the memo).Latchis 120 bytes, and its fields sum to exactly 120 — so ADR 0011's "zero-padding-optimal" claim holds, measured rather than assumed:occupied,pc,word,in_delay_slot,rs_val,rt_valdecodedabort: Option<Exception>write_backmem: Option<MemOp>cop0: Option<Cop0Access>The new part: those last three — 72 of the 120 bytes — are produced at
EX. Inic_rfandrf_exthey are structurally alwaysNone/WriteBack::None. So those two latches copy 72 bytes of provably-empty payload, twice each per cycle: 6.01% of the frame moving nothing.That is not the hazard ADR 0011 ruled out. That hazard is specific to the
DCpath —dc_stage's error branch re-readsself.ex_dcafterabort_withhas stamped it, entangling the:848/:2061/:913copies with abort propagation. The upstream pair has no such entanglement.What this PR deliberately does NOT do
It does not split the struct. The finding is recorded as an untested hypothesis with an upper bound of 1.064x, and left for after the dispatch question is settled — 1.064x against a 7.5x gap is not the thing to spend the next session on, and saying so with a number is better than leaving the target unexamined.
It also carries the warning any attempt needs: this repository records four pipeline changes that compiled, passed every test, and did nothing, so this one needs the CPU golden-log 0-diff and n64-systemtest rather than
cargo test.Verification
Documentation only.
scripts/check_en_us.shandpre-commit run markdownlint --all-filespass. The sizes come from a temporarysize_ofprobe inrustyn64-cpu(this crate isno_std, so the values were surfaced through a deliberately failingassert_eq!); the probe was removed andcargo test -p rustyn64-cpu --libis green with the tree back tomain's content.