perf(bench): report host cycles per emulated instruction - #258
Open
doublegate wants to merge 24 commits into
Open
perf(bench): report host cycles per emulated instruction#258doublegate wants to merge 24 commits into
doublegate wants to merge 24 commits into
Conversation
Every performance conclusion this project has reached was expressed as an FPS number or a profile share, and both hide the thing that decides whether the emulator is fast: what one emulated instruction costs. FPS hides how much work the ROM asked for. A profile share hides absolute cost entirely -- a subsystem is 40% of a frame whether the frame is fast or catastrophically slow, which is how "no bucket is 84% of a frame, therefore 60 FPS is unreachable" got written down. The missing question was never "which subsystem do we delete", it was "why does one instruction cost 228 host cycles when a competent interpreter costs 20-50 and a recompiler costs 2-10". frame_bench now prints insns/frame, MIPS, cycles/insn, and what 60 FPS would require. The figures are directly comparable to what other emulators and the literature quote, which the previous metrics were not. HOST_GHZ is an ASSUMED clock, not a measured one, and the doc comment says so rather than letting a derived number look sourced. Reading the real TSC frequency (or taking the count from perf stat) is the correct fix and this is the honest interim; the approximation errs optimistic, since a core running below the assumed clock has a LOWER true cost than reported.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe pull request adds frame cost metrics, a cen64 comparison benchmark, CPU commit census instrumentation, RDRAM dispatch invariant coverage, and measured performance and accuracy findings. ChangesBenchmarking and performance analysis
CPU commit census
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant BenchmarkScript
participant cen64
participant perf
User->>BenchmarkScript: Provide ROM and duration
BenchmarkScript->>cen64: Run headless benchmark
BenchmarkScript->>perf: Measure host cycles
cen64-->>BenchmarkScript: Emit frame-rate samples
perf-->>BenchmarkScript: Return cycle count
BenchmarkScript-->>User: Report steady-state metrics
Possibly related PRs
🚥 Pre-merge checks | ✅ 6 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (6 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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-frontend/examples/frame_bench.rs`:
- Around line 131-133: Update the documentation attached to TARGET_FPS to
describe it as the target display rate of 60 frames per second. Remove the
unrelated VR4300 clock and instruction-count claims; do not add hardware
constants unless they are moved to an appropriate item with a manual or wiki
citation.
- Around line 105-123: Update the metric wording in the documentation around the
headline and comparison table to say “estimated host cycles per emulated
instruction” instead of presenting host cycles as measured or spent. Preserve
the existing explanation and warning that the value is derived from the assumed
HOST_GHZ constant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: be345138-30ce-4c47-85ec-700930fffc2b
📒 Files selected for processing (1)
crates/rustyn64-frontend/examples/frame_bench.rs
Comment on lines
+105
to
+123
| /// The headline number: **host cycles spent per emulated instruction.** | ||
| /// | ||
| /// FPS hides how much work the ROM asked for, and a profile share hides absolute | ||
| /// cost entirely — a subsystem can be 40% of a frame whether the frame is fast or | ||
| /// catastrophically slow. This figure has neither problem, and it is directly | ||
| /// comparable to what other emulators and the literature quote: | ||
| /// | ||
| /// | | host cycles / instruction | | ||
| /// | --- | --- | | ||
| /// | a recompiler | 2–10 | | ||
| /// | a competent interpreter | 20–50 | | ||
| /// | **60 FPS on this host** | **~58** | | ||
| /// | ||
| /// `HOST_GHZ` is the *assumed* clock, not a measured one, and everything derived | ||
| /// from it inherits that. It is stated rather than hidden because the alternative | ||
| /// — reading the actual TSC frequency, or `perf stat`'s cycle count — is the | ||
| /// right long-term fix and this is the honest interim. A boosting CPU makes this | ||
| /// an approximation in the optimistic direction: if the core is running below | ||
| /// `HOST_GHZ`, the true cycles/instruction is *lower* than reported. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Label this metric as an estimate.
Line 105 says host cycles are “spent”, but the value is derived from the assumed HOST_GHZ constant. This presents an estimate as a measurement.
Change the headline and table labels to “estimated host cycles per emulated instruction”. Preserve the existing assumption warning.
Proposed fix
-/// The headline number: **host cycles spent per emulated instruction.**
+/// The headline number: **estimated host cycles per emulated instruction.**As per path instructions, do not present HOST_GHZ-derived estimates as measured cycles.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/rustyn64-frontend/examples/frame_bench.rs` around lines 105 - 123,
Update the metric wording in the documentation around the headline and
comparison table to say “estimated host cycles per emulated instruction” instead
of presenting host cycles as measured or spent. Preserve the existing
explanation and warning that the value is derived from the assumed HOST_GHZ
constant.
Source: Path instructions
Comment on lines
+131
to
+133
| /// The VR4300 runs at 93.75 MHz and retires close to one instruction per | ||
| /// cycle, so a full-speed frame is this many instructions. | ||
| const TARGET_FPS: f64 = 60.0; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Correct the TARGET_FPS documentation.
Lines 131-132 attach to TARGET_FPS, but they describe the VR4300 clock and an instruction count per frame. They also add the uncited 93.75 MHz hardware claim.
Document TARGET_FPS as the target display rate. Remove the unrelated claim, or move it to a relevant item with a manual or wiki citation.
Proposed fix
- /// The VR4300 runs at 93.75 MHz and retires close to one instruction per
- /// cycle, so a full-speed frame is this many instructions.
+ /// Target display rate for the performance estimate.
const TARGET_FPS: f64 = 60.0;Based on learnings, documentation and implementation are independent claims and must agree. As per path instructions, cite new hardware constants to a manual or wiki article.
📝 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.
Suggested change
| /// The VR4300 runs at 93.75 MHz and retires close to one instruction per | |
| /// cycle, so a full-speed frame is this many instructions. | |
| const TARGET_FPS: f64 = 60.0; | |
| /// Target display rate for the performance estimate. | |
| const TARGET_FPS: f64 = 60.0; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/rustyn64-frontend/examples/frame_bench.rs` around lines 131 - 133,
Update the documentation attached to TARGET_FPS to describe it as the target
display rate of 60 frames per second. Remove the unrelated VR4300 clock and
instruction-count claims; do not add hardware constants unless they are moved to
an appropriate item with a manual or wiki citation.
Sources: Path instructions, Learnings
There was a problem hiding this comment.
Pull request overview
Adds a host-cycles-per-emulated-instruction headline metric to the frame_bench example so performance results can be compared in the same units commonly used in emulator literature (cycles/insn), complementing existing FPS-style reporting.
Changes:
- Print per-frame instruction count, MIPS, and an estimated host
cycles/insnusing an assumed host clock (HOST_GHZ). - Print the MIPS and
cycles/insnbudget required to hit a target real-time rate (60 FPS) and the implied speedup factor.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+121
to
+123
| /// right long-term fix and this is the honest interim. A boosting CPU makes this | ||
| /// an approximation in the optimistic direction: if the core is running below | ||
| /// `HOST_GHZ`, the true cycles/instruction is *lower* than reported. |
Comment on lines
+131
to
+133
| /// The VR4300 runs at 93.75 MHz and retires close to one instruction per | ||
| /// cycle, so a full-speed frame is this many instructions. | ||
| const TARGET_FPS: f64 = 60.0; |
Every performance conclusion this project has reached was RustyN64 measured against RustyN64. Share arithmetic can say which subsystem dominates a frame; it cannot say whether the whole frame is four times more expensive than it needs to be. Only a competitor can say that, and none had ever been run. cen64 is the right subject rather than the convenient one: it is CYCLE-ACCURATE like our default path, so the gap cannot be explained away as the price of accuracy. It also runs headless and prints its own frame rate, so the measurement needs no display, no overlay injection, and no assumption about what the frame rate must be. BSD-3, so it is readable and vendorable. First result, Super Mario 64, i9-10850K, measured at load 6.97 (which penalizes cen64, so it is conservative): cen64 36.5 FPS 131 M cycles/frame 92 cycles/insn RustyN64 accurate 10.0 FPS 501 M cycles/frame 350 cycles/insn RustyN64 fast 15.9 FPS 314 M cycles/frame 218 cycles/insn 60 FPS needs 60.0 FPS 83 M cycles/frame 58 cycles/insn 3.8x, in the same accuracy class. Our 10 FPS is not what cycle accuracy costs. stdbuf -oL is REQUIRED and the script enforces it: cen64's stdout is block-buffered to a pipe, so the first 45-second run reported zero frames while burning 215 G cycles -- output lost in a buffer that died with the process, which reads as "rendered nothing" rather than as lost output. Two guards, both earned. A flock, because two overlapping sweeps once measured each other and killed each other's processes, leaving empty counter files that looked like legitimate zeros. And a load check, because a competing multi-core job inflated a RustyN64 frame from 100 ms to 189 ms -- 1.9x, larger than any optimization being evaluated. The header records why gopher64 (Slint GUI, never advances headlessly), ares (no CLI frame counter, 2x CPU variance between samples) and MangoHud/RetroArch were not usable here, so nobody re-derives it.
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@scripts/bench_reference_emulators.sh`:
- Around line 82-90: Replace the system-wide /proc/loadavg threshold in the
benchmark host-isolation check with a CPU-aware availability check, preferably
pinning the benchmark to an isolated CPU. Record the selected CPU affinity and
its current frequency before measurement, while preserving BENCH_FORCE=1 as the
explicit override for unsuitable conditions.
- Around line 111-122: Update the cen64 measurement block in the awk script so
CYCLES and VI/s cover the same post-warm-up window. Exclude boot and warm-up
cycles from the cycles calculation, or stop trimming VI/s and use the full-run
average; ensure the reported cycles/frame and cycles/instruction use matching
intervals.
- Around line 97-99: Align the perf event and parsing logic for the cen64
benchmark with the reported metric: use the total-cycle event and update any
parser matching or extracting cycles to recognize it, or consistently label and
document the output as user-mode cycles if that is the intended scope. Anchor
changes to the perf invocation and its associated cycles parser.
- Around line 101-102: Update the benchmark logic around CYCLES and SAMPLES to
capture and check the perf stat command status, reject missing or non-positive
numeric CYCLES values, and validate that every matched VI/s value is positive
before performing arithmetic. Ensure invalid counters cause the script to fail
rather than producing a zero-based successful report.
- Line 59: Validate the SECS value immediately after its default assignment and
before invoking timeout, accepting only positive integer strings; reject zero,
negative values, decimals, and duration suffixes with an error and nonzero exit.
Preserve the existing timeout flow for valid SECS values.
- Around line 41-42: Update scripts/bench_reference_emulators.sh to validate the
supplied ROM against the expected Super Mario 64 SHA-256 before using the
1,430,000 instruction denominator, rejecting mismatches instead of reporting a
misleading result; alternatively require a validated instructions_per_frame
value per ROM. Preserve full ROM provenance and the 120-frame measurement in
docs/accuracy-ledger.md.
- Around line 123-126: Update the RustyN64 comparison block in
scripts/bench_reference_emulators.sh to use reproducible frame_bench
measurements for the supplied ROM, or explicitly load a pinned baseline instead
of presenting unverifiable values. Record the baseline commit, feature set, ROM
hash, and measurement provenance in docs/accuracy-ledger.md, and ensure the
displayed FPS values match that documented source.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5892a920-3522-4d14-91d3-acda3febf097
📒 Files selected for processing (1)
scripts/bench_reference_emulators.sh
Comment on lines
+41
to
+42
| # where 1.43 M is Super Mario 64's emulated VR4300 instructions per frame, measured | ||
| # by `examples/frame_bench.rs`. That last figure is what RustyN64 optimises against. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -u
rg -n -C 3 '1_430_000|instructions_per_frame|Super Mario 64|sha256' \
crates/rustyn64-frontend/examples/frame_bench.rs \
docs/accuracy-ledger.md \
scripts/bench_reference_emulators.shRepository: doublegate/RustyN64
Length of output: 18142
🏁 Script executed:
#!/usr/bin/env bash
set -u
printf '%s\n' '--- scripts/bench_reference_emulators.sh ---'
cat -n scripts/bench_reference_emulators.sh
printf '%s\n' '--- frame_bench symbol and call sites ---'
rg -n -C 6 'instructions|retired|frames|Super Mario|rom|ROM|arg|clap|env' \
crates/rustyn64-frontend/examples/frame_bench.rs
printf '%s\n' '--- accuracy-ledger entries for frame_bench and 1.43M ---'
rg -n -C 8 '1\.43|1_430_000|frame_bench|instructions retired|VR4300' \
docs/accuracy-ledger.mdRepository: doublegate/RustyN64
Length of output: 50377
🏁 Script executed:
#!/usr/bin/env bash
set -u
python3 - <<'PY'
from pathlib import Path
import re
script = Path("scripts/bench_reference_emulators.sh").read_text()
ledger = Path("docs/accuracy-ledger.md").read_text()
print("fixed_denominator:", bool(re.search(r'cycles/instruction.*1430000|1_430_000', script)))
print("accepts_rom_argument:", bool(re.search(r'ROM="\$\{1:', script)))
print("hash_used_for_validation:", bool(re.search(
r'(if|test|\[|case|grep|awk|sha256sum).*(sha256|hash)|'
r'(sha256|hash).*(if|test|\[|case|grep|awk)',
script, re.I
)))
print("hash_only_in_output:", bool(re.search(
r'echo .*sha256sum.*cut -c1-16', script
)))
print("ledger_records_super_mario_64_frame_bench:",
"Super Mario 64" in ledger and "examples/frame_bench.rs" in ledger)
counts = re.search(
r'\| accurate .*?\| ([0-9,]+) \|\s*\n'
r'\| `fast-exec` .*?\| ([0-9,]+) \|',
ledger,
re.S,
)
if counts:
accurate = int(counts.group(1).replace(",", ""))
fast = int(counts.group(2).replace(",", ""))
print("accurate_instructions_per_frame:", accurate / 120)
print("fast_exec_instructions_per_frame:", fast / 120)
PYRepository: doublegate/RustyN64
Length of output: 410
Bind the instruction-count denominator to the ROM.
1_430_000 is a rounded Super Mario 64 baseline, but the script accepts any ROM and only prints a truncated hash. A different or patched ROM can therefore produce an invalid cycles/instruction result while reporting success. C-16 records the underlying 120-frame Super Mario 64 measurement, not a denominator for arbitrary ROMs.
Require the expected ROM SHA-256 and reject mismatches, or require a validated instructions_per_frame value for each ROM. Keep the measurement and ROM provenance in docs/accuracy-ledger.md.
🤖 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 `@scripts/bench_reference_emulators.sh` around lines 41 - 42, Update
scripts/bench_reference_emulators.sh to validate the supplied ROM against the
expected Super Mario 64 SHA-256 before using the 1,430,000 instruction
denominator, rejecting mismatches instead of reporting a misleading result;
alternatively require a validated instructions_per_frame value per ROM. Preserve
full ROM provenance and the 120-frame measurement in docs/accuracy-ledger.md.
Source: Coding guidelines
|
|
||
| ROM="${1:?usage: $0 <rom.z64> <pifdata.bin> [seconds]}" | ||
| PIF="${2:?usage: $0 <rom.z64> <pifdata.bin> [seconds]}" | ||
| SECS="${3:-100}" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -u
timeout --help 2>&1 | grep -E 'DURATION|duration' || true
awk 'BEGIN { print ("1m" + 0) }'Repository: doublegate/RustyN64
Length of output: 159
🏁 Script executed:
#!/usr/bin/env bash
set -u
printf '%s\n' '--- script context ---'
sed -n '1,130p' scripts/bench_reference_emulators.sh
printf '%s\n' '--- relevant command usage ---'
rg -n -C 4 'SECS|timeout|awk|usage|SECONDS|elapsed' scripts/bench_reference_emulators.shRepository: doublegate/RustyN64
Length of output: 8488
🏁 Script executed:
#!/usr/bin/env bash
set -u
printf '%s\n' '--- timeout implementation ---'
command -v timeout
timeout --version 2>&1 | head -n 2
printf '%s\n' '--- accepted duration forms ---'
for value in 1 1m 1.5 0 -1; do
if timeout "$value" true >/dev/null 2>&1; then
printf '%s -> accepted\n' "$value"
else
printf '%s -> rejected (status %s)\n' "$value" "$?"
fi
done
printf '%s\n' '--- awk numeric conversion ---'
awk 'BEGIN {
for (i = 1; i <= ARGC - 1; i++)
printf "%s -> %.17g\n", ARGV[i], ARGV[i] + 0
}' 1 1m 1.5 0 -1Repository: doublegate/RustyN64
Length of output: 473
🌐 Web query:
GNU coreutils timeout duration suffixes zero duration behaviour manual
💡 Result:
In GNU Coreutils, the timeout command accepts a duration argument which is a floating-point number followed by an optional suffix [1][2]. The supported suffixes are: 's' for seconds (default) [1][3][2] 'm' for minutes [1][3][2] 'h' for hours [1][3][2] 'd' for days [1][3][2] A duration value of 0 is treated as a special case that disables the timeout [1][2]. When the duration is set to 0, the command will run without a time limit [1][4][2]. Similarly, if the optional --kill-after (or -k) duration is set to 0, that secondary kill-after timeout is also disabled [2].
Citations:
- 1: https://manpages.debian.org/unstable/coreutils/timeout.1.en.html
- 2: https://www.gnu.org.cach3.com/software/coreutils/manual/html_node/timeout-invocation.html
- 3: https://github.com/coreutils/coreutils/blob/master/src/timeout.c
- 4: https://pubs.opengroup.org/onlinepubs/9799919799/utilities/timeout.html
Validate SECS as a positive integer before invoking timeout.
If SECS is 1m, timeout runs for 60 seconds while awk converts it to 1. If it is 0, timeout disables the limit. This violates the determinism requirement. Reject non-positive or non-integer values before use.
🤖 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 `@scripts/bench_reference_emulators.sh` at line 59, Validate the SECS value
immediately after its default assignment and before invoking timeout, accepting
only positive integer strings; reject zero, negative values, decimals, and
duration suffixes with an error and nonzero exit. Preserve the existing timeout
flow for valid SECS values.
Comment on lines
+82
to
+90
| # Refuse to measure on a busy machine. A competing multi-core job inflated a | ||
| # RustyN64 frame from 100 ms to 189 ms during this work: 1.9x, larger than any | ||
| # optimization being evaluated, and invisible in the result. | ||
| LOAD=$(cut -d' ' -f1 /proc/loadavg) | ||
| if awk -v l="$LOAD" 'BEGIN {exit !(l > 3.0)}'; then | ||
| echo "load average is $LOAD -- too busy to measure (want < 3.0)" >&2 | ||
| [ "${BENCH_FORCE:-0}" = "1" ] || { echo "set BENCH_FORCE=1 to override" >&2; exit 1; } | ||
| echo "BENCH_FORCE=1 -- results are NOT comparable" >&2 | ||
| fi |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial
Strengthen the host-isolation check.
/proc/loadavg is system-wide and does not show whether the CPU used by cen64 is available. A load of 2.9 can saturate one relevant CPU, and the same value has different meaning on hosts with different CPU counts.
Pin the benchmark to an isolated CPU and record CPU affinity and frequency, or replace this threshold with a CPU-aware check.
🤖 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 `@scripts/bench_reference_emulators.sh` around lines 82 - 90, Replace the
system-wide /proc/loadavg threshold in the benchmark host-isolation check with a
CPU-aware availability check, preferably pinning the benchmark to an isolated
CPU. Record the selected CPU affinity and its current frequency before
measurement, while preserving BENCH_FORCE=1 as the explicit override for
unsuitable conditions.
Comment on lines
+97
to
+99
| perf stat -e cycles:u -o "$OUT/cen64.perf" \ | ||
| timeout -k 5 -s TERM "$SECS" stdbuf -oL \ | ||
| "$CEN64" -headless "$PIF" "$ROM" > "$OUT/cen64.log" 2>&1 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -u
for event in cycles cycles:u; do
printf '%s\n' "$event"
perf stat -e "$event" -- true 2>&1 || true
doneRepository: doublegate/RustyN64
Length of output: 249
🏁 Script executed:
#!/usr/bin/env bash
set -u
printf '%s\n' '--- script structure ---'
ast-grep outline scripts/bench_reference_emulators.sh --match '$_' --view compact 2>/dev/null || true
printf '%s\n' '--- relevant script section ---'
sed -n '1,160p' scripts/bench_reference_emulators.sh
printf '%s\n' '--- metric references ---'
rg -n -C 3 'host cycles|cycles per|per frame|per instruction|cen64\.perf|perf stat|cycles:u' scripts docs README.md 2>/dev/null || trueRepository: doublegate/RustyN64
Length of output: 50375
🏁 Script executed:
#!/usr/bin/env bash
set -u
python3 - <<'PY'
from pathlib import Path
p = Path("scripts/bench_reference_emulators.sh")
text = p.read_text()
for i, line in enumerate(text.splitlines(), 1):
if "perf stat" in line or "cycles" in line or "frame" in line or "instruction" in line:
print(f"{i}: {line}")
PY
printf '%s\n' '--- all script references to perf output parsing ---'
rg -n -C 5 'perf|\.perf|awk|sed|grep|cycles' scripts/bench_reference_emulators.shRepository: doublegate/RustyN64
Length of output: 7266
Align the event and parser with the reported metric.
cycles:u measures user-mode cycles only. If the report requires total host cycles, use cycles and update the parser from cycles:u to cycles. Otherwise, label the values as user-mode cycles and document that scope.
🤖 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 `@scripts/bench_reference_emulators.sh` around lines 97 - 99, Align the perf
event and parsing logic for the cen64 benchmark with the reported metric: use
the total-cycle event and update any parser matching or extracting cycles to
recognize it, or consistently label and document the output as user-mode cycles
if that is the intended scope. Anchor changes to the perf invocation and its
associated cycles parser.
Comment on lines
+101
to
+102
| CYCLES=$(grep -oE '^[ ]*[0-9,]+[ ]+cycles:u' "$OUT/cen64.perf" | tr -dc '0-9') | ||
| SAMPLES=$(grep -c 'VI/s' "$OUT/cen64.log") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -u
value=$(awk -v c="" -v s=100 -v f=60 \
'BEGIN { printf "%.0f\n", c / s / f }')
printf 'empty counter result: %s\n' "$value"Repository: doublegate/RustyN64
Length of output: 181
🏁 Script executed:
#!/usr/bin/env bash
set -u
printf '%s\n' '--- file outline ---'
ast-grep outline scripts/bench_reference_emulators.sh --match '$$$' --view summary 2>/dev/null || true
printf '%s\n' '--- relevant source ---'
sed -n '1,170p' scripts/bench_reference_emulators.sh
printf '%s\n' '--- related references ---'
rg -n "CYCLES|SAMPLES|perf stat|VI/s|cen64\\.perf|cen64\\.log" scripts README.md docs 2>/dev/null || trueRepository: doublegate/RustyN64
Length of output: 8457
🏁 Script executed:
#!/usr/bin/env bash
set -u
tmp=$(mktemp -d)
trap 'rm -rf "$tmp"' EXIT
# Simulate a missing perf counter and five syntactically valid VI/s samples.
printf '%s\n' \
' Performance counter stats for ...:' \
' <not counted> cycles:u' >"$tmp/cen64.perf"
printf '%s\n' \
'VI/s 60' \
'VI/s 60' \
'VI/s 60' \
'VI/s 60' \
'VI/s 60' >"$tmp/cen64.log"
cycles=$(grep -oE '^[ ]*[0-9,]+[ ]+cycles:u' "$tmp/cen64.perf" | tr -dc '0-9')
samples=$(grep -c 'VI/s' "$tmp/cen64.log")
printf 'extracted cycles=%q samples=%s\n' "$cycles" "$samples"
set +e
awk -v c="$cycles" -v s=100 '
/VI\/s/ { v[n++] = $2 }
END {
lo = (n > 20) ? n - 20 : 0
for (i = lo; i < n; i++) { sum += v[i]; m++ }
fps = sum / m
printf "fps=%.1f cycles_per_frame=%.0f\n", fps, c / s / fps / 1e6
}' "$tmp/cen64.log"
awk_status=$?
set -e
printf 'awk status=%s overall sample-path status=%s\n' "$awk_status" "$awk_status"
printf '%s\n' '--- accepted counter shapes ---'
for raw in \
' 123456 cycles:u' \
' 123,456 cycles:u' \
' <not counted> cycles:u' \
' 0 cycles:u'; do
printf '%s\n' "$raw" | grep -oE '^[ ]*[0-9,]+[ ]+cycles:u' | tr -dc '0-9' | \
awk '{ printf "raw=%s extracted=%q\n", ENVIRON["RAW"], $0 }' RAW="$raw"
doneRepository: doublegate/RustyN64
Length of output: 343
Reject missing perf counters instead of reporting a successful zero.
If perf stat fails or emits no numeric counter, CYCLES is empty and awk converts it to zero. Five matching VI/s lines can then produce a successful but invalid report. Capture the perf stat status, require a positive numeric CYCLES value, and validate positive VI/s values before arithmetic.
🤖 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 `@scripts/bench_reference_emulators.sh` around lines 101 - 102, Update the
benchmark logic around CYCLES and SAMPLES to capture and check the perf stat
command status, reject missing or non-positive numeric CYCLES values, and
validate that every matched VI/s value is positive before performing arithmetic.
Ensure invalid counters cause the script to fail rather than producing a
zero-based successful report.
Comment on lines
+111
to
+122
| # The tail only: cen64's first samples are boot, where there is nothing to render | ||
| # and it briefly reports 150-200 VI/s. Averaging those in would overstate it. | ||
| awk -v c="$CYCLES" -v s="$SECS" ' | ||
| /VI\/s/ { v[n++] = $2 } | ||
| END { | ||
| lo = (n > 20) ? n - 20 : 0 | ||
| for (i = lo; i < n; i++) { sum += v[i]; m++ } | ||
| fps = sum / m | ||
| printf "cen64 (cycle-accurate, headless)\n" | ||
| printf " steady-state : %.1f VI/s (mean of last %d samples of %d)\n", fps, m, n | ||
| printf " cycles/frame : %.0f M\n", c / s / fps / 1e6 | ||
| printf " cycles/instruction : %.0f\n", c / s / fps / 1430000 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Use matching measurement windows for cycles and VI/s.
The tail calculation removes boot samples from fps, but CYCLES still covers process start, boot, and warm-up. The numerator and denominator therefore describe different intervals. The reported value is not a steady-state cycles-per-frame metric.
Measure cycles after warm-up, or report a full-run average using matching samples.
🤖 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 `@scripts/bench_reference_emulators.sh` around lines 111 - 122, Update the
cen64 measurement block in the awk script so CYCLES and VI/s cover the same
post-warm-up window. Exclude boot and warm-up cycles from the cycles
calculation, or stop trimming VI/s and use the full-run average; ensure the
reported cycles/frame and cycles/instruction use matching intervals.
The plan called a page table "expected to be the largest single win", reasoning that every access pays a segment walk, a TLB lookup, a cache-line lookup and an MMIO match chain. Measured, that entire surface is addr.rs 4.41% + cache.rs 2.78% = 7.2% -- a ceiling of 1.08x for a PERFECT page table. It is not the biggest item and it cannot close a 3.8x gap. Phase 1 and Phase 2 swap places. Also built and reverted: hoisting the RDRAM check above read_u32's ten register tests. Those tests sit on the instruction-fetch and load path, so nearly every access pays them, which reads as obviously worth fixing. A-B-A-B says 64.096 / 63.874 / 63.659 / 63.858 ms -- both hoisted legs inside the baseline spread, 0.34% SLOWER on the conservative pairing. Each test is a mask-and-compare against a constant that is always false for RDRAM, and the branch predictor gets it right every time. The invariant that ordering was silently defending IS worth having, so it is now asserted rather than implied: rdram_window_is_disjoint_from_ every_register_block sweeps all 8 MiB against all eleven range predicates across three address aliases. Mutation-checked -- making is_ri_register claim an RDRAM address turns it red. A third lead dissolved on reading rather than measuring: vi.rs:208 shows 3.58% on a line containing a 64-bit division, an obvious memoization target. Vi::tick early-outs above it and a half-line elapses on about one call in 1,980, so that divide runs ~500 times a frame -- order 0.007%. The 3.58% is attribution, the same shape as the Latch refutation. What the profile does say is that it is FLAT: after one attribution-suspect line at 10.4%, nothing exceeds 3.6%. There is no hot spot, which is why the last program's slices kept returning 1-3%. The large thing is per-instruction driver overhead -- fastexec 15.74% + pipeline 8.07% + decode 4.89% + scheduler 5.09% = 33.8% -- all of it work a block-oriented design does once per block instead.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/bench_reference_emulators.sh (2)
125-125: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCorrect the
fast-execcycles-per-instruction baseline.With the displayed
314 Mcycles/frame and1_430_000instructions/frame, the result rounds to220, not218. If218uses another measured instruction count, record that count and use it consistently. The provenance also namesfast-exec,fast-scheduler, but this row names onlyfast-exec.🤖 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 `@scripts/bench_reference_emulators.sh` at line 125, Update the fast-exec benchmark row in the printf output to use the correctly rounded cycles-per-instruction value of 220 based on 314 M cycles/frame and 1,430,000 instructions/frame. Keep the displayed metrics consistent, or document and apply the alternate instruction count if retaining 218; also align the row’s provenance with the named fast-exec,fast-scheduler measurement.
55-55: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPropagate benchmark and parser failures.
Capture and check the statuses from
perf stat, theCYCLESextraction, and the finalawk. Accept only the expected timeout status. Return non-zero for every other failure. The trailingechocurrently permits invalid benchmark data to exit successfully, which violates the repository’s determinism requirement.🤖 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 `@scripts/bench_reference_emulators.sh` at line 55, Update the benchmark pipeline in scripts/bench_reference_emulators.sh to capture and validate the exit statuses from perf stat, the CYCLES extraction, and the final awk calculation. Permit only the expected timeout status, propagate every other failure as non-zero, and ensure the trailing echo cannot mask invalid benchmark data.
🤖 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-core/src/bus.rs`:
- Around line 4093-4106: Update the documentation for the invariant test near
the RDRAM overlap assertion to accurately state that the RDRAM fast path remains
after the MMIO dispatch branches. Remove claims that it is tested first or that
the invariant permits moving it, and describe only the current ordering and the
non-overlap invariant it protects.
- Around line 4127-4154: Update the RDRAM/register disjointness test around the
existing sampling loop to provide complete coverage: check every aligned RDRAM
address for each CPU alias, or assert non-intersection using the canonical
decoded address ranges, rather than relying on fixed samples. At
crates/rustyn64-core/src/bus.rs lines 4127-4154, retain the invariant checks for
Bus::rdram_offset and the register predicates while ensuring coverage is
exhaustive; at lines 2155-2158, state that the invariant assertion runs only
after complete coverage has been performed.
In `@docs/performance.md`:
- Around line 2764-2780: Update the “The fast-exec profile, by source file”
table to account for the missing 18.33% profile share by adding an `other` or
`unresolved` row, or explicitly label the table as showing only top
contributors. Ensure the documented percentages sum to approximately 100% and
preserve the existing attribution entries.
- Around line 2738-2740: Expand the performance provenance entry in
docs/performance.md to include the exact ref-proj/cen64 revision, CMake/build
configuration, and complete ROM hash used for the 36.5 FPS measurement.
Alternatively, update the benchmark script to print and validate these values,
then reference that captured provenance in the record.
- Around line 2754-2757: Update the performance comparison around the “3.8x” and
“load 6.97” claims to remove forced-load results as comparative evidence. Rerun
the benchmark below the load-3.0 guard and use those results, or explicitly
label the existing run exploratory while removing the unsupported causal claim
that load penalised cen64 and the resulting 3.8x conclusion.
- Around line 2804-2808: Update the paragraph describing the Vi::tick 64-bit
division to label the frequency and cost figures as inferred unless measurement
provenance is available. Show the derivation connecting one call in 1,980 to
approximately 500 calls per frame and 0.007% of frame time, or replace those
figures with documented measurement evidence; distinguish the evidence status
for every accuracy claim.
---
Outside diff comments:
In `@scripts/bench_reference_emulators.sh`:
- Line 125: Update the fast-exec benchmark row in the printf output to use the
correctly rounded cycles-per-instruction value of 220 based on 314 M
cycles/frame and 1,430,000 instructions/frame. Keep the displayed metrics
consistent, or document and apply the alternate instruction count if retaining
218; also align the row’s provenance with the named fast-exec,fast-scheduler
measurement.
- Line 55: Update the benchmark pipeline in scripts/bench_reference_emulators.sh
to capture and validate the exit statuses from perf stat, the CYCLES extraction,
and the final awk calculation. Permit only the expected timeout status,
propagate every other failure as non-zero, and ensure the trailing echo cannot
mask invalid benchmark data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 29e2f832-0b0c-4db3-ae1d-e072780bf61e
📒 Files selected for processing (3)
crates/rustyn64-core/src/bus.rsdocs/performance.mdscripts/bench_reference_emulators.sh
Comment on lines
+2738
to
+2740
| **Provenance.** Super Mario 64 (`17ce0773…`), i9-10850K, `rustc 1.96.0`, | ||
| `--release`, `fast-exec,fast-scheduler`, tree at `1cc7dce`, load < 3.0 unless | ||
| stated. Differential over a post-warm-up window in every case. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Record the complete reference-emulator provenance.
The provenance names the RustyN64 tree but not the ref-proj/cen64 revision or CMake configuration. The script builds whichever checkout is present, so the 36.5 FPS result is not reproducible from this record. Record the cen64 revision, build flags, and full ROM hash, or make the script print and validate them.
As per coding guidelines, measured performance values require provenance describing how they were measured.
🤖 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 2738 - 2740, Expand the performance
provenance entry in docs/performance.md to include the exact ref-proj/cen64
revision, CMake/build configuration, and complete ROM hash used for the 36.5 FPS
measurement. Alternatively, update the benchmark script to print and validate
these values, then reference that captured provenance in the record.
Source: Coding guidelines
Comment on lines
+2764
to
+2780
| ### The `fast-exec` profile, by source file | ||
|
|
||
| `perf record -F 999 -e cycles:u -D 4000`, source-line attribution. | ||
|
|
||
| | share | file | | | ||
| | --- | --- | --- | | ||
| | 18.44% | `bus.rs` | memory + MMIO | | ||
| | 15.74% | `fastexec.rs` | the per-instruction driver | | ||
| | 8.07% | `pipeline.rs` | still 8% with the timing model bypassed | | ||
| | 7.77% | `uint_macros.rs` | stdlib `saturating_add` / `wrapping_add` / `bswap` | | ||
| | 5.30% | `vu.rs` | RSP vector | | ||
| | 5.09% | `scheduler.rs` | | | ||
| | 4.89% | `decode.rs` | | | ||
| | 4.72% | `su.rs` | RSP scalar | | ||
| | 4.46% | `vi.rs` | | | ||
| | **4.41%** | **`addr.rs`** | **all address translation** | | ||
| | **2.78%** | **`cache.rs`** | **all cache simulation** | |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Account for the unlisted profile share.
The listed shares total 81.67%, leaving 18.33% unaccounted. Add an other or unresolved row, or state clearly that this is a top-contributors table.
Based on learnings, performance attribution tables must include all relevant categories or an explicit remainder, and the percentages should total approximately 100%.
🤖 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 2764 - 2780, Update the “The fast-exec
profile, by source file” table to account for the missing 18.33% profile share
by adding an `other` or `unresolved` row, or explicitly label the table as
showing only top contributors. Ensure the documented percentages sum to
approximately 100% and preserve the existing attribution entries.
Source: Learnings
fast-exec builds a 120-byte Latch for every instruction and runs it through the accurate path's wb_stage. That is deliberate -- it is what keeps COP0, COP1, TLB and retirement semantics identical in both modes without a second implementation -- and it is also the largest per-instruction cost in the profile. A direct commit could skip it, but only for instructions that touch none of that machinery. Before writing one, this measures how many there are, because "most instructions are simple ALU ops" is an assumption and this project has four reverted changes that started as one. Super Mario 64, 221,219,587 retired instructions: SIMPLE (GPR or nothing) 96.81% MEM (has a DC access) 3.11% COP (COP0/COP1) 0.07% HILO (mul/div) 0.01% So a direct path serving SIMPLE only covers 96.81%, and the subtle cases keep going through wb_stage untouched -- which is the version worth building, since the duplicated logic reduces to "write this value to this register" and cannot silently diverge on COP0, FP or TLB semantics because it never handles them. 3.11% MEM is low enough for MIPS to be worth disbelieving: loads and stores are normally 20-30% of a stream. The classifier is therefore witnessed rather than trusted. A load and a store both land in MEM, a GPR write and a bare branch both land in SIMPLE, and a COP access OUTRANKS a memory op -- an instruction with both is not something a direct path may serve, and counting it as MEM would understate the class that must keep using wb_stage. ADR 0006: counters only, nothing schedules against them, #[serde(skip)] so the save-state layout is unchanged.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 789-794: Move the WB rustdoc block from the `commit_census`
accessor to immediately before `fn wb_stage`, restoring its association with
that method. Keep only the retired-instruction census documentation on
`commit_census`, preserving its existing signature and behavior.
- Around line 443-454: Add a work-counters instrumentation contract section to
docs/cpu.md covering the feature gate, the commit-class classification scope,
cumulative lifetime during execution, and reset-on-save-state-restore behavior
resulting from #[serde(skip)].
- Around line 428-431: Correct the `OTHER` documentation and related test
expectations around the `count_commit` classification: remove the claim that
`OTHER` includes aborts, and rename the test to describe only the currently
reachable `SIMPLE` and `HILO` classes. Do not add an `OTHER` case unless
`count_commit` gains a real retired variant for it.
In `@crates/rustyn64-cpu/src/pipeline/fastexec.rs`:
- Around line 273-277: Move the work-counters increment from the pre-latch
location to the successful retirement boundary beside self.retired, after
wb_stage completes, so faulting MemOp and trapping COP1/CTC1 instructions are
excluded while both accurate and fast paths remain counted consistently. Add
tests verifying neither a faulting memory operation nor a trapping COP1
operation increments the census.
In `@crates/rustyn64-frontend/examples/work_bench.rs`:
- Around line 169-218: Update report_commit_census to snapshot commit_census
before the timed FRAMES window, alongside the existing *_before metrics, then
subtract each class’s baseline from the post-window census before computing
totals and percentages. Use the timed CPU retired delta for the consistency
assertion, verifying the class-total delta matches it after retirement
accounting is corrected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fdc7926b-d50f-4f5c-bbd9-ea690fb081d1
📒 Files selected for processing (6)
crates/rustyn64-core/Cargo.tomlcrates/rustyn64-cpu/Cargo.tomlcrates/rustyn64-cpu/src/lib.rscrates/rustyn64-cpu/src/pipeline.rscrates/rustyn64-cpu/src/pipeline/fastexec.rscrates/rustyn64-frontend/examples/work_bench.rs
Comment on lines
+428
to
+431
| /// Anything else, including aborts. | ||
| pub const OTHER: usize = 4; | ||
| /// Number of classes. | ||
| pub const COUNT: usize = 5; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the OTHER class contract.
count_commit has no abort state. Its current WriteBack variants classify only as SIMPLE or HILO. Therefore OTHER is unreachable, and it cannot include aborts in a retired-instruction census.
Remove the abort claim. Rename the test so it describes the currently reachable classes, or add a real retired OTHER case when such a class exists.
As per path instructions, flag a comment that asserts what the code does but disagrees with it.
Also applies to: 7492-7506
🤖 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 428 - 431, Correct the
`OTHER` documentation and related test expectations around the `count_commit`
classification: remove the claim that `OTHER` includes aborts, and rename the
test to describe only the currently reachable `SIMPLE` and `HILO` classes. Do
not add an `OTHER` case unless `count_commit` gains a real retired variant for
it.
Source: Path instructions
Comment on lines
+443
to
+454
| /// Retired-instruction census by commit class, indexed by [`commit_class`]. | ||
| /// | ||
| /// A **counter**, not a schedule input (ADR 0006). It exists to answer one | ||
| /// question before any code is written against it: what fraction of executed | ||
| /// instructions could a direct commit path actually serve? Sizing a change on | ||
| /// an assumed fraction is how the last program produced four reverts. | ||
| /// | ||
| /// `#[serde(skip)]` because it is diagnostic and must not enter the | ||
| /// save-state layout (ADR 0011 §4). | ||
| #[cfg(feature = "work-counters")] | ||
| #[serde(skip)] | ||
| commit_census: [u64; commit_class::COUNT], |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the CPU instrumentation contract.
Add the work-counters contract to docs/cpu.md. Define its feature gate, classification scope, cumulative lifetime, and reset-on-save-state-restore behaviour caused by #[serde(skip)].
Based on learnings, “When adding or modifying RustyN64 chip instrumentation, document the feature contract in the corresponding docs/.md file.”
🤖 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 443 - 454, Add a
work-counters instrumentation contract section to docs/cpu.md covering the
feature gate, the commit-class classification scope, cumulative lifetime during
execution, and reset-on-save-state-restore behavior resulting from
#[serde(skip)].
Source: Learnings
Comment on lines
+789
to
+794
| /// The retired-instruction census by commit class ([`commit_class`]). | ||
| #[cfg(feature = "work-counters")] | ||
| #[must_use] | ||
| pub const fn commit_census(&self) -> &[u64; commit_class::COUNT] { | ||
| &self.commit_census | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Restore the wb_stage rustdoc association.
The WB rustdoc block immediately before this accessor now documents Pipeline::commit_census, although the accessor does not commit or retire an instruction. Move that block to immediately precede fn wb_stage, and keep only accessor documentation here.
Based on learnings, consecutive /// blocks attach to the next item unless an item separates them.
🤖 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 789 - 794, Move the WB
rustdoc block from the `commit_census` accessor to immediately before `fn
wb_stage`, restoring its association with that method. Keep only the
retired-instruction census documentation on `commit_census`, preserving its
existing signature and behavior.
Source: Learnings
Comment on lines
+273
to
+277
| // Census BEFORE the latch is built: the question this answers is how | ||
| // often building it is avoidable at all. | ||
| #[cfg(feature = "work-counters")] | ||
| self.count_commit(e.cop0, e.mem, e.write_back); | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Count only instructions that retire.
This increment occurs before access and wb_stage. A failing MemOp returns at Lines 321-324 without retirement. A CTC1 or FP trap returns at Lines 344-346 before retired increments. Both cases already increment this census.
Classify early if needed, but increment the counter only after a successful wb_stage retirement. Prefer placing the increment beside self.retired so the accurate and fast paths share the retirement boundary. Add a test that a faulting memory operation and a trapping COP1 operation do not change the census.
🤖 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/fastexec.rs` around lines 273 - 277, Move
the work-counters increment from the pre-latch location to the successful
retirement boundary beside self.retired, after wb_stage completes, so faulting
MemOp and trapping COP1/CTC1 instructions are excluded while both accurate and
fast paths remain counted consistently. Add tests verifying neither a faulting
memory operation nor a trapping COP1 operation increments the census.
Comment on lines
+169
to
+218
| report_commit_census(&core); | ||
| } | ||
|
|
||
| /// **How much of the instruction stream a direct commit path could actually | ||
| /// serve.** | ||
| /// | ||
| /// `fast-exec` builds a 120-byte `Latch` for every instruction and runs it | ||
| /// through the accurate path's `wb_stage` — deliberately, because that is what | ||
| /// keeps COP0, COP1, TLB and retirement semantics identical in both modes | ||
| /// without a second implementation. A direct commit would skip that, but only | ||
| /// safely for instructions that touch none of it. | ||
| /// | ||
| /// This is the fraction that decides whether such a path is worth writing. | ||
| /// Sizing it on an assumed "most instructions are simple ALU ops" is exactly the | ||
| /// move that produced four reverted changes in the previous program. | ||
| fn report_commit_census(core: &EmuCore) { | ||
| use rustyn64_core::cpu::commit_class as cc; | ||
| let census = core.system().cpu.pipeline.commit_census(); | ||
| let total: u64 = census.iter().sum(); | ||
| assert!( | ||
| total > 0, | ||
| "no instructions were classified — the census is not wired into the \ | ||
| executing path, and a table of zeros reads as a result" | ||
| ); | ||
| #[allow( | ||
| clippy::cast_precision_loss, | ||
| reason = "counts over a bench run are far below 2^53" | ||
| )] | ||
| let pct = |n: u64| n as f64 / total as f64 * 100.0; | ||
| let names = [ | ||
| (cc::SIMPLE, "SIMPLE (GPR or nothing)"), | ||
| (cc::MEM, "MEM (has a DC access)"), | ||
| (cc::COP, "COP (COP0/COP1)"), | ||
| (cc::HILO, "HILO (mul/div)"), | ||
| (cc::OTHER, "OTHER"), | ||
| ]; | ||
| println!("\ncommit classes over {total} retired instructions:"); | ||
| for (idx, label) in names { | ||
| println!( | ||
| " {label:<28} {:>12} {:>6.2}%", | ||
| census[idx], | ||
| pct(census[idx]) | ||
| ); | ||
| } | ||
| println!( | ||
| "\nA direct commit path could serve the SIMPLE class only: {:.2}%.\n\ | ||
| That share bounds what skipping the Latch can win — the rest must keep \ | ||
| going through wb_stage.", | ||
| pct(census[cc::SIMPLE]) | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Report the timed-window census delta.
commit_census is cumulative from power-on, but this function reports its raw value after warm-up. The table therefore includes boot and warm-up instructions while the CPU, RSP, bus, and VU metrics report only the FRAMES window.
Snapshot the census with the other *_before values. Subtract it per class before calculating total and percentages. Assert that the class-total delta equals the CPU retired delta after the retirement accounting is 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 `@crates/rustyn64-frontend/examples/work_bench.rs` around lines 169 - 218,
Update report_commit_census to snapshot commit_census before the timed FRAMES
window, alongside the existing *_before metrics, then subtract each class’s
baseline from the post-window census before computing totals and percentages.
Use the timed CPU retired delta for the consistency assertion, verifying the
class-total delta matches it after retirement accounting is corrected.
fast-exec built a 120-byte Latch for EVERY instruction and ran it through the accurate path's wb_stage. That was deliberate and the comment said so: sharing one commit implementation is what keeps COP0, COP1, TLB and retirement semantics identical in both modes. It was also the largest per-instruction cost in the profile. The census (previous commit) measured how much of the stream needs any of that machinery: 3.11% has a DC access, 0.07% touches COP0/COP1, 0.01% writes HI/LO. The other 96.81% commits a GPR or nothing, and for those everything wb_stage does reduces to one register write plus two retirement counters. So those commit directly and EVERYTHING ELSE falls through unchanged. The duplicated logic is regs.write(dest, value) plus the retirement tail — small enough that it cannot silently disagree with wb_stage about semantics it never implements, which is the property the shared path was protecting. A1 63.522 B1 51.972 A2 63.691 B2 52.039 ms A legs agree to 0.27%, B legs to 0.13%. Conservative pairing (worst B vs best A) is 1.2207x: 15.74 -> 19.22 FPS, 220 -> 180 cycles/instruction. The gap to 60 FPS goes 3.81x -> 3.12x and the gap to cen64 2.39x -> 1.96x. The retirement tail is the part that is easy to miss and is called out in the code: `retired` feeds the golden-log comparison, and tick_random advances COP0 Random, which "decrements as each instruction executes" (UM 5.4.2). A fast path skipping it would leave Random stuck, and a stuck Random makes every TLBWR overwrite the same entry — a bug this project has already shipped once from the other direction, when tick_random was implemented and never called. Accuracy: n64-systemtest Failed: 0 on the Phase-1 categories through BOTH paths (default, and rustyn64-core/fast-exec + fast-scheduler), plus fast_exec_differential and fast_exec_scheduler green.
… plane A performance investigation into the VI's per-pixel cost turned up an accuracy defect instead, and the optimization it looked like was NOT taken. docs/performance.md had an outstanding measurement recorded against the vi_divot entry: "settling it needs a coverage histogram over a real frame". Taken on Super Mario 64 over 27,150,246 filtered pixels: cvg0 22.45% cvg4 77.55% every other value 0.00% Two values, both with the low two bits clear. That is not a distribution, it is a bit that is never set. pixel_coverage computes a full 0..=7 and its top bit reaches the framebuffer as the RGBA5551 alpha LSB. The VI reconstructs coverage as ((px & 1) << 2) | rdram_read_hidden(byte) -- and nothing ever writes the color buffer's hidden plane. The RDP's only rdram_write_hidden is in zbuffer_write, storing delta-Z to the Z buffer. The N64brew Wiki RDRAM page is explicit that the 9th bit is where anti-aliasing coverage lives in the color buffer. Consequence: cvg == 7 can never hold, so the de-dither filter is unreachable on every workload and the AA-edge filter runs on every pixel with all six neighbor taps discarded, because each is kept only if nb_cvg == 7. VI anti-aliasing is effectively disabled. Why this is a ledger entry and not a patch: the same measurement reads as a large VI win -- six of seven RDRAM reads per pixel are provably dead, ~163 M wasted reads across the benchmark. Taking it would have made the emulator faster at producing a picture with its anti-aliasing broken, and cemented the defect behind a performance argument. The reads are dead BECAUSE of the bug, so the bug is what gets fixed. Not oracle-pinned yet: the wiki establishes where coverage belongs and the histogram establishes that we do not put it there, but no committed Angrylion vector asserts the hidden plane after a partial-coverage draw. That vector is what closes this, and it has to be a rendered comparison -- angrylion-rdp-plus is study-only, outputs never source.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/rustyn64-cpu/src/pipeline/fastexec.rs (1)
273-276: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix the census counting point: the "opportunity" framing contradicts the "retired instructions" claim.
The comment here says the census is deliberately taken "BEFORE the latch is built," answering "how often building it is avoidable at all." But the comment at Lines 302-304 describes the same counter as "96.81% of retired instructions commit a GPR or nothing." These are two different metrics: one counts every instruction that reaches this point regardless of outcome, the other implies composition of instructions that actually retired.
Because
count_commitruns beforeaccess(Line 375) and beforewb_stage(Line 396), aMemOpthat later faults, or a COP1 operation that later traps throughself.pending(Lines 397-403), is still counted here even though it never retires. If the 96.81% figure is meant to describe retired-instruction composition, it is measuring the wrong population.Move the increment to the shared retirement boundary — beside
self.retired = self.retired.wrapping_add(1)in the fast path (Line 340) and beside the accurate path's own retirement point — so both paths agree on what "retired" means. If the counter is intentionally an opportunity metric instead, rewrite the Line 302-304 comment so it no longer claims "% of retired instructions."This was raised on an earlier revision of this same call site ("Count only instructions that retire," recommending the counter move beside
self.retiredand a test asserting a faultingMemOpand a trapping COP1 op leave the census unchanged). That test still appears absent from what is shown.🐛 Proposed fix: share the retirement boundary
- // Census BEFORE the latch is built: the question this answers is how - // often building it is avoidable at all. - #[cfg(feature = "work-counters")] - self.count_commit(e.cop0, e.mem, e.write_back); - // COP2 is one 64-bit latch rather than a register file; ... @@ self.retired = self.retired.wrapping_add(1); self.cop0.tick_random(); + #[cfg(feature = "work-counters")] + self.count_commit(e.cop0, e.mem, wb);(and analogously beside the accurate path's own retirement point in the fallback branch)
Also applies to: 302-306
🤖 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/fastexec.rs` around lines 273 - 276, Move the work-counters increment from the pre-latch call site to the retirement boundaries: beside self.retired.wrapping_add(1) in the fast path and the equivalent retirement point in the accurate path. Ensure faulting MemOp and trapping COP1 operations are not counted, while preserving the existing count_commit arguments and retired-instruction interpretation.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 `@docs/performance.md`:
- Around line 2824-2828: Update docs/performance.md lines 2824-2828 in the VI
coverage histogram section with the harness, command, commit, build features,
complete ROM hash, measurement window, and precise filtered-pixel definition.
Update docs/accuracy-ledger.md line 482 to limit the 0/4 conclusion to the
measured RGBA5551 coverage path unless adding measured 32-bit-path evidence;
cite the exact N64brew RDRAM section supporting the hidden-bit claim.
---
Duplicate comments:
In `@crates/rustyn64-cpu/src/pipeline/fastexec.rs`:
- Around line 273-276: Move the work-counters increment from the pre-latch call
site to the retirement boundaries: beside self.retired.wrapping_add(1) in the
fast path and the equivalent retirement point in the accurate path. Ensure
faulting MemOp and trapping COP1 operations are not counted, while preserving
the existing count_commit arguments and retired-instruction interpretation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e0e1bd2f-30c3-47c0-9b37-ed99ffcb9222
📒 Files selected for processing (3)
crates/rustyn64-cpu/src/pipeline/fastexec.rsdocs/accuracy-ledger.mddocs/performance.md
The RDP computed a full 0..=7 coverage and stored only its top bit, as the RGBA5551 alpha LSB. The VI reassembles coverage as ((px & 1) << 2) | rdram_read_hidden(byte), and nothing ever wrote the color buffer's hidden 9th-bit plane -- the only rdram_write_hidden was zbuffer_write, storing delta-Z to the Z buffer. So the low two bits were dropped on every write, cvg == 7 could never hold, and every coverage-gated VI filter silently degenerated: the de-dither path was unreachable and the AA-edge filter discarded all six of its neighbor taps on every pixel. write_coverage stores the low two bits for 16-bit color images only. A 32-bit image keeps all three in the alpha byte and the VI reads them as (px >> 5) & 7, so writing the plane there would store a second copy the hardware does not keep. IT HAD TO BE DONE TWICE, and the first half looked like success. Fixing only the no-Z span moved cvg7 from 0.00% to 2.59% and made every value reachable -- but 74.91% of pixels still sat at exactly cvg4, because depth_span had the identical gap and most of the scene goes through it. Super Mario 64, 27,150,246 filtered pixels: before cvg0 22.45% cvg4 77.55% everything else 0.00% no-Z only cvg0 22.28% cvg4 74.91% cvg7 2.59% both paths cvg0 17.43% cvg7 70.95% cvg1-6 2.21/0.92/1.89/4.30/1.33/0.97% The final shape -- a large fully-covered majority, a thin spread of partial edges, some background -- is what a rendered frame should look like, and no earlier state of the code could produce it. THE UNIT TEST DID NOT ESTABLISH THIS AND IS RECORDED AS INSUFFICIENT. coverage_survives_the_round_trip_for_every_value calls write_coverage directly, so deleting the call site left it green -- the wiring trap this project has hit before. It is kept because it pins the bit layout and the 32-bit exemption, but the witness is the histogram through the real render path. All 164 Angrylion RDP vectors and all 13 VI vectors still pass, which is the expected result rather than a surprise: this writes the hidden plane and changes no color output.
R-24's fix rested on the N64brew wiki plus a measured distribution that
looked right -- inference, not an oracle. Closing it needs a vector that
asserts the hidden plane, and no format carried one: .vivec v2 takes a
plane as INPUT for VI vectors, while .rvec v2's spare header words are
already spent on a preload region. Hence v3, which keeps v1's header and
appends a width*height plane after the golden framebuffer.
The generator reaches Angrylion's rdram_hidden by extern declaration --
a link-time reference to the oracle's ABI, not a transcription of its
source, which is what the non-commercial license forbids.
Two Makefile bugs fixed on the way, both of which failed in ways that
pointed at the wrong thing. ANGRYLION_CORE defaulted to ../../../ref-proj
which climbs OUT of the repository, failing with a bare "No rule to make
target" that reads as a broken Makefile. And the two Angrylion checkouts
in ref-proj lay out headers differently -- parallel-rdp's submodule keeps
vdac.h beside n64video.h, the standalone one splits it into a sibling
output/ and redeclares vdac_write -- so pointing at the wrong one fails
on a header conflict rather than on the path. Both are now named in the
Makefile.
All 164 existing vectors are byte-identical: they stay at version 1 and
the plane is emitted only when a vector asks for it.
THE NEW TEST IS #[ignore]d, and that is the honest state rather than a
flake. Its first run surfaced two findings, each worth its own change:
1. We render NOTHING for an AA 1-cycle triangle -- all-zero framebuffer
against a non-zero oracle. Every other vector renders in FILL mode,
so this is the first to exercise that path.
2. Angrylion clears the hidden plane to 3 (HB_CLEAN), we power on at 0,
so untouched pixels cannot match whatever the renderer does. Whether
3 is the hardware reset state or an Angrylion convention is NOT
established here and must not be assumed -- ADR 0004 makes power-on
state a documented value, not a copied one.
The infrastructure is what took the work and it is committed; the two
gaps are recorded in the test's own doc comment so the next person reads
them before re-running it.
…e block Ledger R-21 recorded this half as open and unexercised: a FLAT Fill Triangle (0x08) with no shade or texture block took the SET_FILL_COLOR register whatever the cycle type, because has_color keyed off the presence of a shade/texture block rather than the cycle type. R-21 resolved the identical defect for Fill Rectangle against the oracle (fill_rect_1cycle_16: a 1-cycle rectangle renders the PRIM color, never the fill register) and noted that no vector reached the triangle. aa_tri_coverage_16 is that vector -- the first committed one to render a triangle in 1-cycle mode; every earlier one is FILL. Its framebuffer now matches Angrylion where it previously did not. The rule is the same for both primitives because it is a property of the cycle type, not of the primitive: FILL and COPY write the fill register, 1-/2-cycle run the combiner, which for a flat primitive sees only its register inputs. THE FIRST VERSION OF THAT VECTOR COULD NOT HAVE FOUND THIS, and the reason is worth keeping: its combine emitted black, so "drawn black" and "not drawn at all" produced the same framebuffer and the comparison was VACUOUS. It now selects the prim color (adder input 3, where the shaded vectors use 4 = shade) and renders white. FALLOUT, and it is R-21's own fallout repeating: two unit tests named fill_triangle_flat_fills_a_right_triangle and fill_triangle_is_clipped_to_the_scissor assert the fill register reaches the framebuffer while never selecting FILL mode -- they were passing on this bug, exactly as the five fill_rectangle_* tests were. Both now set cycle_type explicitly and test what their names claim. All 41 RDP conformance vectors and all 13 VI vectors still pass. Still open, recorded in the ignored test rather than papered over: coverage reaches only 28 of 64 pixels we demonstrably DRAW (the framebuffer matches while the plane reads 0 against the oracle's 3), and the plane's power-on state differs. Two further defects, each needing its own change.
I reported "coverage reaches only 28 of 64 pixels we demonstrably draw" and that was WRONG. A per-pixel dump settles it: the framebuffers agree on every pixel with no exceptions, and our coverage matches the oracle EXACTLY on every pixel the triangle drew -- 3 in the interior, 1 along the anti-aliased edge. We draw ~28 pixels; the other 36 are undrawn, and the oracle's 3 there is its plane's initial state. The misdiagnosis was reading "the framebuffer matches" as "we draw all 64". It means both agree those pixels are EMPTY. Recorded because a wrong defect report costs the next reader the same detour. So R-24's fix is confirmed against the oracle, and the test is un-ignored and scoped to the drawn set. Undrawn pixels are excluded for a real unknown rather than convenience: nothing this project mirrors documents the hidden plane's power-on state, and Angrylion's HB_CLEAN = 4 has its bit 2 as an internal dirty marker rather than coverage, so that constant cannot just be adopted (ADR 0004: power-on state is documented, not copied). Still ledgered under R-24. The exclusion cannot swallow the test: DRAWN = 28 is asserted, so a change that stopped drawing fails here instead of quietly comparing nothing. That guard is not hypothetical -- an earlier revision of this vector rendered BLACK, which made "drawn" and "not drawn" the same framebuffer and the whole comparison vacuous. MUTATION CHECK COVERS ONE OF THE TWO WRITE SITES, and the doc comment now says so rather than overclaiming. Removing the no-Z span's write_coverage turns this red (verified). It CANNOT check depth_span's, because this vector's triangle carries no Z block and never reaches that path. That half stays witnessed by the SM64 histogram: no-Z alone left 74.91% of pixels at cvg4; both together put cvg7 at 70.95%. A Z-buffered companion vector would close the gap and is noted as worth adding.
…g 1.096x Every chip is stepped on every RCP step, so an event-driven scheduler's ceiling IS the idle fraction. That is a different quantity from a profile share and the difference is why this was measured rather than estimated: a share says what the RSP costs WHEN IT RUNS, not how often it is stepped while halted. Sizing the phase from the share would have repeated this program's most expensive mistake. Super Mario 64, 162,500,080 RCP steps: RSP halted 78.11% RDP idle 99.97% (frozen, XBUS, stalling, or empty FIFO) The RSP's profile share is vu.rs 5.94% + su.rs 5.31% = 11.25%, of which 78.11% is halted, so at most 8.79% of a frame is recoverable -> 1.096x. The RDP's larger idle share is worth much less: the split-borrow it guards was already made conditional, so what remains per step is the predicate itself. A METHODOLOGICAL TRAP THE FIRST RUN FELL INTO, recorded rather than quietly corrected. Sampling the RDP's idle state before rsp_tick reported 100.00% -- an artifact, not a result: the RSP's dp_write submits a command list during rsp_tick and rdp_tick consumes it in the same step, so the FIFO is always empty at that instant. The census now samples where rdp_tick itself decides, which gives 99.97%. A census must sample at the point the decision it models would actually be made. The predicate is pinned to tick_without_bus's real early-outs by census_predicate_agrees_with_the_real_early_outs, including that it is READ-ONLY -- the real step decrements the stall it tests, and a census doing the same would change what it measures. ADR 0006 throughout: counters only, nothing schedules against them, #[serde(skip)] so the save-state layout is untouched.
Guarding rsp_tick on !halted() is the RSP half of an event-driven scheduler, which the occupancy census sized at 1.096x. Built, A-B-A-B: 56.739 / 56.589 / 56.652 / 56.450 ms. The skip legs sit inside the baseline spread; 0.11% on the conservative pairing. Reverted. THE CEILING ARITHMETIC WAS WRONG AND THE ERROR GENERALIZES: idle-fraction x profile-share overstates a skip whenever the idle path is already cheap. su::su_step early-returns on its halt check before doing anything, so the 78.11% of halted steps already cost almost nothing -- the 11.25% profile share is spent almost entirely on the 21.89% of steps where the RSP runs. The census answered how OFTEN the RSP is idle; sizing a skip needs how much the idle steps COST, and that measurement was missing. Third ceiling this program has produced that did not survive being built, after fastmem's 7.2% and the block cache's decode share, and all three failed identically: a share multiplied by something it is not proportional to. ONE REAL DEFECT CAME OUT OF IT, and it is kept. rcp_steps was incremented at the tail of Bus::rsp_tick, so "RCP steps" meant "RSP ticks" -- invisible while the RSP was stepped unconditionally, and wrong the moment the skip landed, because the counter stopped with the chip and three_cpu_and_two_rcp_steps_per_six_ticks went red. The charge now lives in System::step_rcp where the step actually happens, pinned by an_rcp_step_is_charged_even_when_every_chip_is_idle.
….119x The scan-out's existing memo keys on the OUTPUT of the filter chain, which cannot help the divot filter: the divot runs inside one `cells` miss and calls `vi_fetch_cov` three times per output pixel (x-1, x, x+1), so advancing one column recomputes two of the three from scratch. Each is 6 (AA-edge) or 8 (de-dither) `vi_read_cov` calls plus its own center read. Sized by elision rather than by arithmetic, because the three ceilings this program produced that did not survive being built all failed by multiplying a share against something it is not proportional to: stubbing the taps out measured 16.7% of a frame (9.40 ms), so perfect elimination is 1.200x and a window memo was banded at 1.09-1.12x before building. Adds `ViSampler::cov_cells`, a second memo layer keyed on x - (x_lo - 1) — two columns wider than `cells` on each side so the divot's outer taps land inside the window instead of missing on every row's first and last column. Same rows, same `row_y`, same eviction; `row_slot` now clears both layers. Mutation-checked: deleting the `cov_cells` clear turns `memoized_scanout_matches_uncached_recomputation` red. A-B-A-B on Super Mario 64 (examples/frame_bench, fast-exec + fast-scheduler): A 56.832 / B 50.749 / A 56.805 / B 50.338 ms. 1.119x on the conservative pairing, 17.60 -> 19.79 FPS, 197 -> 176 host cycles per emulated instruction. All 13 Angrylion VI conformance vectors still pass byte-for-byte. Also corrects the differential test's doc: it described `vi_sample_direct` as bypassing the memo, which stopped being true once the second layer was consulted from inside it. The test remains valid because its reference sampler has span == 0 and `cov_span()` keeps zero at zero — but the property now rests on the `bypass.span == 0` assertion rather than on which function is called, and without that assertion it would have become a comparison of the memo against itself. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ADR 0016 declined the RSP VU SIMD exception at a 5.3% / 1.056x ceiling taken from the VU census, where it was computed as 62% x 8.5% — an operation-count share multiplied by a time share. That is the same shape as the three ceilings this program produced that did not survive being built, and the census itself ended by instructing that any SIMD work be measured against it. It was. The obvious probe is invalid and the workload counters are what caught it: replacing multiply_lane's dispatch and arithmetic with an XOR measured 1.255x, but `work-counters` shows RSP instructions per frame falling 294,983 -> 121,715. The garbage results steered the microcode, so most of that delta was avoided RSP work. `retired` moved by 80 out of 173 M, so checking only the CPU counter would have passed it through. Measured instead by doubling — multiply_lane split into an inline(never) body called once or twice, with vu_acc[lane] saved and restored around the discarded pass, so final state is bit-identical and `retired` is identical in all four legs. One pass is 1.33-1.39 ms of a ~50.8 ms frame: 2.6-2.7%, a ~1.028x ceiling. The multiply/accumulate family is among the cheapest work the VU does, so 61.6% of the operations is well under 61.6% of the time. B2 stays declined; every reason for it is stronger and the cost is unchanged. Marks the census's 5.3% superseded in place so it cannot be re-cited, and adds a dated addendum to ADR 0016 rather than restating its figures — the decision is unchanged, and one that survives its headline number being halved is worth reading in the original. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sizing the block cache found something far larger. Two independent methods put the average sequential run at 2.14 instructions, and a PC histogram says why: `beq $0,$0,-1` with a `nop` delay slot accounts for ~95% of every instruction Super Mario 64 retires and ~90% of Mario Kart 64's. It is the N64 idle thread, and the emulator was faithfully burning host cycles on it. Skipping it is not an approximation. After the pair the PC is back where it started and the only state either instruction touches is `Count` — derived from the scheduler's tick — and `Random`, which the skip reproduces by hand. A-B-A-B, conservative pairing: OoT 2.046x (52.5 FPS), Mario Kart 64 1.836x (41.3 FPS), Super Mario 64 1.589x (31.6 FPS), Banjo-Kazooie neutral at 1.003x — the control, whose run length is 5.93 because it does not use this loop. `retired` is identical in every leg of every title. Caching the recognition is as correct as the hardware: the N64 does not snoop DMA against the I-cache, so software that overwrites code must issue CACHE itself and until it does the CPU executes the stale line. `cache_op` clears the recognition at the single entry point for every CACHE variant, and so does taking any exception. Both hooks are mutation-checked — deleting the cache hook or the `Random` ticks each turns a specific test red, and the second names COP0 register 1 in its message. Gates: n64-systemtest unchanged under fast-exec (Phase 1 and RSP categories Failed: 0, 90 suite-wide), clippy clean under both feature sets, no_std builds. Adds nothing to ledger C-16 and needs no ADR — it lives inside the execution mode ADR 0013 already authorizes, and unlike that mode it introduces no divergence at all. Extracts `apply_cop2` and `recognize_idle_loop` from `execute_one`, which the added line pushed over the too_many_lines gate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The backlog was priced at ~1.12x / +1.8 FPS and each item declined on that basis. The arithmetic was right and is now stale: every figure in it was a share of a 65.3 ms frame, and the idle-loop skip, the fast commit and the VI memo removed CPU and VI cost rather than RDP, GPU or RSP cost. The backlog's absolute cost is unchanged while the frame halved, so its share doubled. The multiplier barely moves (1.115x) but the FPS does, because FPS is not linear in frame time: +3.6 FPS on Super Mario 64 rather than +1.8, and on Ocarina of Time 52.5 -> 58.6, which puts a real title within a percent of the target. None of the three is free — A3 needs an ADR and double-buffered RDRAM, A4 reopens ADR 0015's determinism argument, B2 costs ADR 0016's unsafe exception at a ceiling that was just re-measured downward. Also marks §The honest position on 60 FPS superseded at its head. It concluded 60 FPS was unreachable by reasoning from profile shares of the existing execution path; what closed the gap was noticing that ~90% of the instructions did not need to run. Kept as the record of the reasoning, not as a claim. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three ADRs for the three items in the declined backlog that measure positive. Each is written on the re-derived numbers rather than the ones they were declined on, since those were shares of a frame that has since halved. 0018 (async GPU RDP) accepts shape (b) — present one frame late — as an opt-in that is off by default, because its cost is a frame of presentation latency and ~3.5% is not enough to spend a user's latency budget for them. Records that shape (a) is unavailable: `present` stages RDRAM before enqueueing, so submitting mid-frame would change which memory each command reads. Also names a GPU-to-GPU ordering hazard the plan missed, which is NOT the CPU-side tracker ADR 0014 §6 describes. 0019 (GPU as the machine's rasterizer) accepts A4 as an opt-in but BLOCKS building it until the GPU/Angrylion census reaches 43/43. This is the finding that changed the shape of the decision: the GPU is currently LESS complete than the software rasterizer it would replace (`key_en`, #160), so shipping it for ~2.5% would trade correctness for frame time. The ADR inverts the justification — A4 is worth building as an ACCURACY change, with the frame time as a side effect, and that reframing is what makes the gate load-bearing. 0020 amends 0016 to accept the SIMD exception for `multiply_lane` only, and says plainly that the technique got worse while the context got better: 0016 declined 1.056x, this accepts 1.045x. The four gates carry forward unrelaxed, and the ADR states in advance what would make it a mistake — that a hard-to-write equivalence test is a reason to stop, not to sample. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…r the queue Measured, not summed: charging 64 PCycles per idle skip instead of 2 drops the number of CPU-side idle iterations 32x while leaving the frame's total emulated ticks — and so the total RCP step count — unchanged, so the delta is 31/32 of the per-iteration overhead. 4.17 ms across four legs gives ~4.30 ms of a 31.76 ms frame: 13.5%, a 1.156x ceiling, 31.6 -> 36.3 FPS on Super Mario 64. That re-orders the queue and the reason is worth recording: the free item is larger than both ADR-gated ones combined (B2 at 4.3% costing forbid->deny, A3 at 3.5% costing a frame of latency). Both should be re-sized against the frame the idle work leaves behind rather than against this one — repeating the exact error the backlog re-derivation two sections up corrects. Also records the constraint any implementation must respect: an idle pair is 4 master ticks and the RCP steps every 3, so the RCP steps MORE often than the CPU idles. No batching removes an RCP step, which is why the ceiling is 1.156x and not the whole idle path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ocates the cost Built the obvious version — hoist the scheduler's scaffolding (boot_nmi_halt, the RCP edge setup, the report tally) out of the idle stretch, keeping the interrupt check at every instruction boundary. 32.137 ms against a 31.674/31.841 baseline with `retired` bit-identical: slower, outside the spread, reverted. The negative result is worth more than the change would have been, because it locates the 13.5%. The 64-PCycle probe cut `step_instruction_at` CALLS by 32x; the hoist kept those calls and moved only what surrounds them, and gained nothing. So the cost is inside the per-boundary work — set_now, sample_interrupt_lines, the NMI check, interrupt_pending, the call — which cannot be batched by the easy route, because reducing how often the interrupt check runs changes the cycle an interrupt is recognized on. Records the design that would actually recover it: poll_irq flips only at an RCP step, so the level can be tested right after each step_rcp and the batch left at the first boundary at or after the raising edge; timer_edge is computable from master_ticks so it bounds the batch rather than needing a per-boundary test; Random and the retired tally are pure arithmetic. Also warns that the 1.156x must be re-derived before the attempt rather than inherited — the same error the backlog re-derivation corrects. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…oval Deleting the per-boundary interrupt sampling to measure its cost is vacuous: the idle loop is exited BY the interrupt, so with the check gone the machine never leaves it and the VI never comes up. frame_bench's liveness assert caught it rather than reporting a fast frame over a dead machine. Doubling is not a free substitute either — sample_interrupt_lines calls Cop0::timer_edge, which is edge-detecting and carries internal state, so a second call consumes the edge instead of measuring it. A sound probe can only duplicate the pure reads, which covers part of what a batch would remove. So the 13.5% stays a total rather than a decomposition, and the recorded batch design should be graded against that total with its own A-B-A. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The idle skip returned to the scheduler every two idle instructions. The batch
runs further pairs without re-evaluating the boundary, entered only AFTER a
boundary has executed without vectoring — which is what establishes nothing is
pending. From there every input to that check is constant or bounded:
* Cause.IP2 follows poll_irq, which flips only when an RCP chip sets mi_intr,
and that happens only at an RCP step the batch performs itself and tests
after.
* Cause.IP7 follows timer_edge, a delta against last_count. COUNT_DIVIDER is
exactly twice CPU_DIVIDER, so one idle pair is exactly one Count tick and
count_ticks_until_timer_match bounds the batch in pairs, keeping the
crossing on the boundary the per-instruction walk would have found it on.
* Status's masks cannot move: no instruction executes.
* boot_nmi_halt is bus state, so it changes only across an RCP step.
Leaving the batch does not consume the boundary that ends it, which is what
keeps an interrupt on its original cycle. retire_idle_pairs ticks Random in a
loop rather than computing it: Random wraps to 31 at Wired, not at zero, so
random -= 2n is wrong exactly when a long batch crosses that wrap. The new test
sweeps Wired across 0/1/7/30/31 and pair counts across it.
Super Mario 64 1.064x (32.6 -> 34.8 FPS), Mario Kart 64 1.072x (43.2 -> 46.6),
every B leg beating every A leg, retired bit-identical in all six.
n64-systemtest unchanged under fast-exec.
THE HARNESS WAS ALSO WRONG, and this is the larger finding. Three attempts to
measure this came back contaminated because every leg ran `cargo build` and
then timed immediately after — a parallel release build is a multi-core job and
loadavg is a 1-minute average, so it was still decaying from the harness's own
build. Interleaving does not cancel it: A-B-A cancels monotonic session drift,
but here BOTH legs sit behind a build, so the bias lands on both. Leg spreads
went from 36% to under 1% once the binaries were pre-built and the measurement
window contained no compilation. scripts/bench_aba.sh is that harness.
Recorded deltas near the noise floor are downgraded to unverified rather than
overturned — nothing decided by `retired` is affected, and the large results
sit far outside the bias.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…028x/1.057x `retire_idle_pairs` ticked `Random` in a loop, putting a per-pair cost back into the batch that exists to remove per-pair costs. `Cop0::tick_random_by` is the closed form of the same walk. It is not `random - n`. `Random` walks down to `Wired` and then jumps to 31, so the sequence is a transient followed by a cycle of `31..=Wired`. Three cases look impossible and are reachable, because both fields are masked to 6 bits on write: `Wired > 31`, `Random < Wired`, and `Random > 31`. Each lengthens the walk rather than erroring, so both distances are taken modulo 64 rather than assumed in range — a form that assumes `Wired <= 31` is right for every value software sensibly writes and wrong for exactly the ones a test ROM probes. Tested exhaustively rather than by sampling: every (Wired, Random) pair against 71 tick counts, plus a million-tick case for the modulo. Mutation-checked — the naive form dies at `wired 0, random 0, 32 ticks`, the wrap on the first cycle. A-B-A-B-A-B on the no-build harness, every B leg beating every A leg, spreads 0.14-0.79%: Super Mario 64 1.028x (35.0 -> 36.2 FPS), Mario Kart 64 1.057x (46.9 -> 49.6). Batch plus closed form against the pre-batch baseline is 1.105x and 1.145x against the probe's 1.156x ceiling — the first measured implementation in this program to land on its predicted ceiling rather than well under it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`master {n} ticks` is a monotonically climbing 64-bit number that tells a user
nothing about whether the emulator is keeping up. The tick count stays in the
debugger panel, where a raw timebase belongs.
Counts PRODUCED frames against wall time, not egui's repaint rate — the latter
would read 60 while the machine crawled, which is the failure mode worth
avoiding. Sampled on a 0.5 s interval so the reading is steady rather than
flickering through a range, shows `-- FPS` before the first interval instead of
a confident 0.0, and clears rather than reporting a negative rate if the frame
counter goes backwards (reset, save-state load).
Uses egui's monotonic `input.time` rather than `std::time::Instant`, and the
doc says why the tempting justification does NOT apply: `Instant` panicking on
wasm32 is true of the crate and not of this module, which is
`cfg(not(target_arch = "wasm32"))` with a separate wasm shell. That reason
would have quietly stopped holding.
Two tests, both witnessed failing when broken.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… /tmp
A `/tmp` scan surfaced ~1970 candidates and 612 "strong" ones, but almost all
were strong by LOCATION — they sat in this project's agent scratch tree, so the
directory name matched rather than anything about the file. Hand-picked instead:
30 files, 140 KB, each hand-written and not reproducible from the repository.
DELIBERATELY EXCLUDED, because the default plan would have taken them:
* ~100 MB of commercial ROMs and a framebuffer dump derived from one.
scripts/check_no_roms.sh is the gate; this would have walked around it.
* a 128 MB directory of synthetic test ROMs, which the scan wanted as a
single directory unit
* PGO profile data (~37 MB) and rendered screenshots (~14 MB), the latter
also derived from commercial ROMs
* 284 benchmark .txt logs — every conclusion is already in docs/performance.md
* 214 .md PR bodies and review replies — that text lives on the pull requests
* copies of tracked source (audio_*.rs, sched_*.rs, pipeline_orig.rs) —
recoverable with `git show`
TOUCHES A QUALITY GATE, flagged for review: scripts/check_en_us.sh gains
`salvaged/` as an excluded tree. Two rescued files are en-US conversion tools
whose substitution TABLES are en-GB words as data (26 flagged lines in one), and
one is a diff that must stay byte-exact to be worth keeping — per-line markers
would litter a data table and corrupt a patch. This extends the documented
"not our prose to edit" rationale already covering ref-docs/, n64brew_wiki/,
ref-proj/ and third_party/, rather than relaxing what the gate checks over
project content. Verified by negative control: planting `colour behaviour` in
docs/glossary.md still fails the gate.
salvaged/README.md records what is there, what was left, and that several
bench/ shells contain the rebuild-between-legs measurement bug — kept as the
record of how the affected numbers were taken, not as a pattern to copy.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Antigravity review (Gemini via Ultra)This PR adds host-cycles-per-instruction reporting to benchmarks, implements a CPU idle-loop recognition skip, a GPR commit fast path in Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Phase 0 of the new performance plan. Every performance conclusion this project has reached was expressed as FPS or a profile share, and both hide the quantity that decides whether an emulator is fast.
That second blind spot is how "no bucket is 84% of a frame, therefore 60 FPS is unreachable for this execution model" got written into
docs/performance.mdand two ADRs. The question it never asked:What this adds
frame_benchnow prints, alongside the existing line:Directly comparable to what other emulators and the literature quote, which neither previous metric was.
Honesty about
HOST_GHZIt is an assumed clock, not a measured one, and the doc comment says so rather than letting a derived figure look sourced. Reading the real TSC frequency, or taking the cycle count from
perf stat, is the correct fix; this is the interim.The approximation errs optimistic: a core running below the assumed 5 GHz has a lower true cycles/instruction than reported, so this cannot flatter the emulator.
A note on measurement conditions
While validating this I recorded
mean=189.258msfor a configuration that measures99.955mson a quiet machine — a 1.9x inflation, because a 19-core job was running concurrently. Those readings were discarded. Worth flagging in review: this repo's A-B-A discipline is designed for ~1-2% session drift, not for a competing multi-core load, and nothing in the harness currently detects the latter. A load guard inframe_benchmay be worth a follow-up.Gates
cargo fmt --check,clippy --workspace --all-targets -D warnings,cargo test --workspace,rustdoc -D warnings— one conditional,ALL-GATES-OK. No production code touched; this is an example binary only.