interp_stats: divide CPU cycles by CPU cycles, not by the master clock - #66
Merged
Conversation
The interpreted/recompiled split was computed as interp816_cycles_total() / g_cpu.master_cycles, and its comment invited the reader to take 100 minus that as "the fraction of execution that ran as statically-recompiled C". Those are different units. interp816 charges CPU (bus) cycles -- interp816.c sets cyclesUsed = 7 for an interrupt -- while master_cycles is the 21.477 MHz master clock, where one CPU cycle is 6, 8 or 12. Master cycles also advance for DMA, HDMA and time the CPU is not executing, so it was never a measure of execution to begin with. The error flattered the AOT tier by roughly 6-8x, and in the direction that stops anyone investigating. Measured on Gundam Wing, whose AOT graph is 908 instructions -- the reset and interrupt vectors and nothing else: before: interp_cycle_pct 8.88 => "91.11% recompiled C" after: interp_cycle_pct 97.91 => 2.10% recompiled C The corrected figure agrees with everything else: 6.5M interpreter instructions over 718 frames is ~9,100 per frame against a frame's ~29,830 CPU cycles, i.e. the interpreter is running the entire guest instruction stream, and the 2% is those vector trampolines firing 60 times a second. g_cpu.cycles is the right denominator because both tiers charge it in the same unit: the generated C emits cpu->cycles += <const> per block, and the interpreter adds its bus-cycle count in interp_bridge.c. master_cycles is still reported -- pacing and APU work want it -- it is just not what the share is computed from. aot_cycle_pct is now sent explicitly rather than left as a subtraction for the reader to get wrong. Clamped at 100% for reporting only: the interpreter's own bus-cycle rounding can put it a hair over, and a headline of 100.4% invites a bug hunt into a sub-percent accounting artifact. This affects every port that has used interp_stats to decide where to optimise. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013VUH3SvejkGbYN7DdqeBfc
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.
cmd_interp_statscomputed the interpreted/recompiled split asand its comment invited the reader to take
100 - interp_cycle_pctas "the fraction of execution that ran as statically-recompiled C".Those are different units:
interp816charges CPU (bus) cycles —interp816.csetscyclesUsed = 7for an interrupt.master_cyclesis the 21.477 MHz master clock, where one CPU cycle is 6, 8 or 12.master_cyclesalso advances for DMA, HDMA and time the CPU is not executing, so it was never a measure of execution in the first place.Effect
The error flattered the AOT tier by roughly 6–8×, in the direction that stops anyone investigating. Measured on a Gundam Wing build whose AOT graph is 908 instructions — the reset and interrupt vectors and nothing else:
interp_cycle_pctThe corrected figure agrees with everything else observable on that build: 6.5M interpreter instructions over 718 frames is ~9,100 per frame against a frame's ~29,830 CPU cycles — the interpreter is running the entire guest instruction stream, and the remaining ~2% is those vector trampolines firing 60 times a second.
Fix
g_cpu.cyclesis the right denominator because both tiers charge it in the same unit: the generated C emitscpu->cycles += <const>per block, and the interpreter adds its bus-cycle count ininterp_bridge.c.master_cyclesis still reported — pacing and APU work want it — it is just not what the share is computed from.cpu_cyclesis now reported too.aot_cycle_pctis sent explicitly rather than left as a subtraction for the reader to perform (and, as the old comment shows, get wrong).Scope
debug_server.cis shared, so any port that has usedinterp_statsto decide where to optimise has been reading the same inverted picture. Worth a look if that number has informed decisions elsewhere.Single file, no behaviour change outside the reported statistics. Verified compiling against
main, and the before/after figures above were taken live over the debug protocol.