docs: close APU Workstream D — one adoption, five closed levers - #386
Conversation
Workstream D of the v2.3.6 APU round, adjudicated and rejected. Recorded with its numbers per the standing convention — the reason this campaign can skip so many settled dead ends is that the rejections were written down as carefully as the adoptions. D1 gated the DMC end-of-cycle tick behind an idle predicate; D6 replaced `LengthCounter::reload`'s unconditional halt store with a compare. Both were byte-identical and both are reverted. Measured as one adoption unit across two independent runs. The headline is that run 1 looked like a textbook win — up to -3.81% at p = 0.00 on a shipped `_fast` workload — and was entirely an artifact. Its order-bias control, which re-benches the reference against ITSELF, drifted -3.4% to -4.3% on all four workloads. The apparent -3.81% is accounted for almost exactly by a -3.73% drift measured with no code change at all. The mechanism is recorded because it will recur: `ab_check.sh` builds the reference in a throwaway worktree and benches it immediately afterwards, so on this workspace the reference measures on a machine still hot from a 44.9-second fat-LTO compile across all cores, while the candidate runs once thermals have settled. Run 2 removed the confound — cached reference build, `AB_MEASUREMENT_TIME=25` — and the control's drift fell from ~4% to ~1%. In that better-conditioned run the candidate tracks the control to within a few tenths of a percent on every workload, and the raw sign has flipped positive. Two runs, drift-corrected, both say zero. Also recorded: why a null was the *expected* result, which matters because a null with no mechanism is just a failed measurement. Under `lto = "fat"` with `codegen-units = 1`, `dmc_tick_end` is already inlined into `cpu_clock` and LLVM can common-subexpression the loads across the blocks the guard skips; the branches D1 elides were always-not-taken and so perfectly predicted. Trading nine predictable not-taken branches for nine loads, an OR-reduction and one branch is arithmetically a wash. "Inert on almost every cycle" predicts a large win only if the work is actually executed, and under fat LTO with perfect prediction it largely is not. That reasoning applies to D2, D4 and D5 as well, and is written down so it tempers them rather than being rediscovered three more times. Reverted rather than kept as a simplification. D1's correctness argument is a nine-term case analysis sited in the DMA timing that the `$500`/`$520`/`$540` implicit-abort battery exists to pin; carrying that in the least testable part of the emulator for an effect indistinguishable from zero is the same bad trade v2.2.3 P3 and v2.3.6 D3 declined. D5 is additionally recorded as declined WITHOUT measurement, on inspection: `add_sample` cannot know whether expansion audio is live, so keying its finite-check on the caller's `external` argument swaps one per-cycle branch for another rather than removing one. D2 and D4 remain unmeasured, and D3's now-stale "still open" list is corrected to point here. Byte-identity is separately established and not in question: `dmc_dma` 1/1, `dma_timing_pin` 11/11, the APU unit suite 143/143, AccuracyCoin 141/141 on the authoritative RAM decoder, nestest 0-diff, and a full `--features test-roms` sweep across 127 test binaries. The rejection is on evidence of benefit, not on correctness.
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Updates the performance documentation to record the v2.3.6 APU workstream items D1 and D6 as measured and rejected, preserving the measurement context (including order-bias control failure mechanism) so the same pitfalls aren’t rediscovered in later campaigns.
Changes:
- Added a new “v2.3.6 D1 + D6” section documenting the attempted optimizations, measurement results, and rejection rationale.
- Documented the observed order-bias/control drift mechanism tied to benchmarking immediately after a hot fat-LTO build.
- Updated the “v2.3.6 D3” section to reflect that D1/D6 were later measured and rejected (and D5 declined on inspection), leaving D2/D4 as the remaining unmeasured items.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Workstream C/D — "the APU is 18.7% of frame time, recover some of it" — is closed. Recorded in `docs/performance.md` as a scoped decision rather than left to lapse, and `docs/STATUS.md`'s v2.3.1 entry, which still listed the APU as an unmeasured lead, now points at the outcome. The 18.7% figure itself stands and is not in dispute. It is a correct subsystem attribution from v2.3.1, recoverable only because that pass attributed by source file — fat LTO inlines the APU into `cpu_clock` and hides it from a symbol profile entirely. What is settled is narrower and more useful: the figure is not recoverable by GATING PER-CYCLE BOOKKEEPING, which is the only strategy this workstream ever tried. The tally is one adoption and five closed items. C1, the default-configuration mix specialization, shipped in v2.3.5 at -3.3% to -4.2%. D1, D3 and D6 were each measured on a quiet host across two independent runs and each rejected. D5 was declined on inspection without measurement. D2 and D4 are left unmeasured deliberately. The reason to close rather than continue is that the three rejections share one mechanical cause, which generalises to the two remaining levers. Under `lto = "fat"` with `codegen-units = 1` the code these levers guard is already inlined into `cpu_clock`, its loads are already common-subexpressioned, and the branches being elided are always-not-taken and so perfectly predicted. Swapping predictable not-taken branches for an equivalent count of loads and a predicate is arithmetically a wash. Stated as the rule worth keeping: "this work is inert on almost every cycle" predicts a win only if the work is actually EXECUTED, and under fat LTO with perfect prediction it largely is not. D2 and D4 are structurally the same shape as the three that failed, so their prior is now a null rather than an unknown. Measuring them would spend two more quiet-host A/B pairs confirming what three data points already indicate, and D4 additionally carries a `snapshot_schema_audit` registration plus a recompute-on-restore obligation — the same two standing costs that counted against D3. Recorded as a decision so a later reader does not mistake them for oversights. Three specific conditions that would justify reopening are written down, and none is a variation on per-cycle gating: a structural change to how the APU is clocked or synthesized (C1, the one lever that paid, worked by specializing an entire code path rather than skipping bookkeeping); a profile on hardware or a codegen configuration where the fat-LTO and perfect-prediction premises do not hold; or an instrument better than the +/-1-2% this host resolves, which is the floor that made three of these calls "not demonstrated" rather than "demonstrably zero".
|
Scope extended: this PR now also closes APU Workstream D. Recording the D1+D6 rejection made the workstream-level conclusion unavoidable, so it is written down as a scoped decision rather than left to lapse. The tally: one adoption, five closed items.
The 18.7% figure is not in dispute. It is a correct subsystem attribution from v2.3.1, visible only because that pass attributed by source file — fat LTO inlines the APU into Why close rather than continue. The three rejections share one mechanical cause that generalises to the two remaining levers. Under The rule worth keeping: "this work is inert on almost every cycle" predicts a win only if the work is actually executed — under fat LTO with perfect prediction it largely is not. D2 and D4 are structurally the same shape as the three that failed, so their prior is a null rather than an unknown. They are left unmeasured deliberately, not by oversight, and that is recorded so a later reader does not spend two quiet-host A/B pairs confirming it — D4 additionally carrying the same Three conditions that would justify reopening are written into |
doublegate
changed the title
Two review points on the Workstream D write-up, both fair.
A missing comma after the subordinate clause in the D1 description
("...on a cartridge not running a DMC sample all of it is inert...") made
the sentence garden-path on first read.
"common-subexpression" was used as a verb in two places. It is a noun
phrase naming an optimization pass; the sentences now say that
common-subexpression elimination merged the loads, and that LLVM can
apply it across the blocks the guard skips.
Neither changes the technical content, but this section exists to be read
later by someone deciding whether to reopen the workstream, and a
statement about compiler behaviour is worth stating in the compiler's own
vocabulary.
|
Both points taken — fixed in Nitpick, "common-subexpression" as a verb: correct, and worth fixing rather than waving through. It is a noun phrase naming an optimization pass. The two sentences now read "common-subexpression elimination has already merged its repeated loads" and "LLVM can apply common-subexpression elimination to the field loads across the blocks the guard skips". This section exists to be read later by someone deciding whether to reopen the workstream, and a claim about compiler behaviour should be stated in the compiler's own vocabulary or it invites doubt about whether the claim was understood. Suggestion, communicating the closed status to anyone still referencing the v2.3.4 Workstream C list: already handled, in the two places that pointed at it. The §D3 section carried a "Still open from the v2.3.4 Workstream C list" enumeration naming D1, D2, D4, D5 and D6; it is now marked superseded and redirects here. And The remaining mentions in |
Self-inflicted, and caught in review. The previous commit rewrote two sentences in place without re-wrapping the paragraphs around them, leaving one 120-character line and one sentence hard-wrapped mid-clause. Both are reflowed to the surrounding width. Long lines that remain in this section are tables and headings, which `.markdownlint.json` exempts by design — MD013 is disabled precisely so the wide measurement tables can stay readable as tables.
|
Both wrapping issues were self-inflicted by the previous commit and are fixed in the latest push — thanks for catching them. The prose fix rewrote two sentences in place without re-wrapping the paragraphs around them, leaving one 120-character line and one sentence hard-wrapped mid-clause. Both are reflowed to the surrounding width; the only long lines left in the section are tables and headings, which On the minus sign: keeping U+2212, because it is the established convention here. Checked rather than assumed — Worth stating why the convention exists, since "default to ASCII" is reasonable advice in general: this file is dense with signed percentages where the sign carries the entire meaning of the sentence, and U+2212 renders at the same width as the digits while an ASCII hyphen renders narrow and reads as a dash. In a column of |
The substantive one: "Three levers were measured and three produced nothing" was ambiguous, because C1 was also measured — and adopted. A reader arriving at the closure to decide whether to reopen it could reasonably have read that sentence as covering every measurement in the workstream, which would misstate the record in the one direction that matters. The sentence now names D1, D3 and D6 and says in the same breath that C1 was measured and is the one that paid. The rest is prose. "**The changes**, measured together as one adoption unit because..." was a sentence fragment; it now follows the "**The change.**" label-then-sentence pattern the D3 section already uses. A run-on parenthetical in `docs/STATUS.md` is split into three sentences, which also lets the second lead — `range.rs` inside `Ppu::tick` at 1.52%, still unmeasured — stand on its own rather than trailing a clause about the APU. And a comma pair around "separately" that interrupted the flow of the byte-identity sentence is dropped. Paragraphs reflowed after each edit rather than left ragged, which is the mistake the previous two commits each made in turn.
|
Four of five applied — thanks, the second one was a real ambiguity rather than a style point. Naming the three levers: fixed, and it mattered. "Three levers were measured and three produced nothing" could reasonably be read as covering every measurement in the workstream, when C1 was also measured — and adopted at −3.3% to −4.2%. Since this section exists to be read by someone deciding whether to reopen the workstream, that misstates the record in the one direction that matters. It now names D1, D3 and D6 and says in the same breath that C1 was measured and is the one that paid. Sentence fragment: fixed. It now follows the "The change." label-then-sentence pattern the §D3 section already uses, so the two rejection write-ups read the same way.
Comma after "separately": taken. Minus sign: declining, same as the previous pass — this was already checked. |
Antigravity review (Gemini via Ultra)This PR updates the project's status and performance documentation to formally close APU Workstream D and record the rejection of several per-cycle gating optimizations. Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
doublegate
changed the title
|
Taken — title updated to You are right that the old trailing clause narrowed it: the PR dispositions all six levers, not the two that were measured last. D1, D3 and D6 measured and rejected; D5 declined on inspection; D2 and D4 deliberately left unmeasured with the reasoning recorded; C1 noted as the one adoption. The new title carries that shape without enumerating it. Worth more than tidiness here — this repo squash-merges, so the PR title becomes the commit subject and is what Merging. Three consecutive passes with no blocking issues, CI green, all threads resolved. |
Records v2.3.6 workstream D1 + D6 as measured and rejected. Documentation only — no code ships, and the implementations are reverted.
What was measured
D1 —
Apu::dmc_tick_endruns every CPU cycle at 1.789 MHz and was the largest untouched component of the APU's 18.7% of frame time (~23% of per-cycle cost). Exactly two things in it are unconditional hardware — the byte-timer clock and the get/put parity flip; everything else is DMA corner-case bookkeeping gated on some piece of DMC state being non-idle. D1 added an idle guard between the two.Deliberately not the maintained summary flag the v2.3.4 note proposed: that would need updating at each of ~30 mutation sites across
apu.rsanddmc.rs, and one missed site is a silent accuracy regression in the least testable corner of the emulator. The guard instead reads the same fields the skipped blocks branch on, so byte-identity was structural rather than asserted.D6 —
LengthCounter::reload, called four times per cycle, was doing an unconditional store ofnew_haltover an already-identicalhalt. Four redundant stores per cycle, each writing a byte back onto itself.Why rejected
Two independent runs of
scripts/perf/ab_check.sh --base origin/main, on a host verified quiet first (self-hosted review runner idle, load 2.06/20 cores, no cargo running).nes_run_frame_nestestnes_run_frame_flowing_palettenes_run_frame_nestest_fastnes_run_frame_flowing_palette_fastRun 1 looked like a textbook win and was entirely an artifact. Its order-bias control re-benches the reference against itself, and drifted −3.4% to −4.3% on all four workloads — the apparent −3.81% is accounted for almost exactly by a −3.73% drift measured with no code change at all.
The mechanism is recorded because it will recur: the script builds the reference in a throwaway worktree and benches it immediately after, so on this workspace the reference measures on a machine still hot from a 44.9 s fat-LTO compile across all cores. Run 2 removed the confound (cached build,
AB_MEASUREMENT_TIME=25) and the drift fell from ~4% to ~1%. In that better-conditioned run the candidate tracks the control to within a few tenths of a percent on every workload, and the raw sign has flipped positive.Three independent grounds, any one sufficient: the sign flips between runs; neither control was clean and run 1's failed outright; and in the well-conditioned run the candidate is indistinguishable from the drift.
Why a null was the expected result
Recorded because a null with no mechanism is just a failed measurement. Under
lto = "fat"withcodegen-units = 1,dmc_tick_endis already inlined intocpu_clockand LLVM can CSE the field loads across the blocks the guard skips. The branches D1 elides were always-not-taken and perfectly predicted, so trading ~9 predictable not-taken branches for 9 loads, an OR-reduction and one branch is arithmetically a wash."Inert on almost every cycle" predicts a large win only if the work is actually executed — under fat LTO with perfect prediction it largely is not. That applies to D2, D4 and D5 too, and is written down so it tempers them rather than being rediscovered three more times.
Correctness is not what failed
Byte-identity was established and is not in question:
dmc_dma1/1,dma_timing_pin11/11, APU unit suite 143/143, AccuracyCoin 141/141 on the authoritative RAM decoder, nestest 0-diff, and a full--features test-romssweep across 127 test binaries. The rejection is on evidence of benefit.D1 is reverted rather than kept as a simplification: its correctness argument is a nine-term case analysis sited in the DMA timing the
$500/$520/$540battery exists to pin, and carrying that for a zero effect is the trade v2.2.3 P3 and v2.3.6 D3 already declined.Also recorded
D5 declined without measurement, on inspection:
add_samplecannot know whether expansion audio is live, so keying its finite-check on the caller'sexternalargument swaps one per-cycle branch for another rather than removing one. D2 and D4 remain unmeasured; D4 would add derived state toPulse, incurring the samesnapshot_schema_auditand recompute-on-restore obligations that counted against D3. D3's now-stale "still open" list is corrected to point here.