fix: stop the WASM engine counting bus accesses on top of per-instruction cycles - #218
Merged
Conversation
|
Warning Review limit reachedNext included review available in 12 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
… engines (#215) compareEngines now compares the cycle count each engine returns from performSingleStep, and a new battery holds both engines to the datasheet (NOP, LDA modes incl. page-cross, STA abs,X, INC abs, JSR/RTS, branches, PHA/PLA). Against real WASM this is RED: the engine reports NOP=3, LDA #=4, LDA zp=6 because the bus helpers and the opcode arms both count. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VDq6at6VpsyBxLqB8wBnDT
…tion cycles read_byte_from_bus / write_byte_to_bus each added a cycle while every opcode arm also added the instruction's full documented count, so step_with_bus reported roughly twice the real figure (NOP=3, LDA #=4, LDA (zp),Y=10). Clock budgets against that value, so the WASM engine ran about half the instructions per emulated second that the JS engine did. Drop the per-access increments; the arms alone now match the datasheet and the JS engine's model (IRQ/NMI stay at a flat 7 in both). Verified against the real WASM build: parity 27/27 incl. the new documented-cycle battery, all engine suites 75/75. Re-ran the headless benchmark with honest counting: WASM is ~7.8x JS on cycles/s (instr/s ~8.5x), not the ~14x the docs quoted, which was measured with the inflated counter. Docs and CLAUDE.md updated. Closes #215 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VDq6at6VpsyBxLqB8wBnDT
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VDq6at6VpsyBxLqB8wBnDT
stid
force-pushed
the
fix/wasm-cycle-double-count
branch
from
September 4, 2026 06:12
53f8f24 to
adb4fa9
Compare
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.
Closes #215
Summary
read_byte_from_bus/write_byte_to_businwasm-cpu/src/cpu.rseach added a cycle while every opcode arm also added the instruction's full documented count, sostep_with_busreported ~2× (NOP=3, LDA #=4, LDA zp=6, LDA (zp),Y=10).Clockbudgets against that value, so the WASM engine executed about half the instructions per emulated second that the JS engine did.compareEnginesnow asserts the cycle count returned by eachperformSingleStepmatches, and a new documented-cycle battery holds both engines to the datasheet (NOP, LDA imm/zp/abs, abs,X and (zp),Y with and without page cross, STA abs,X, INC abs, JSR/RTS, branch not-taken/taken/page-cross, PHA/PLA). Before the fix: 23/27 red on cycles only.wasm-performance.md"~14×" headline was measured with the inflated counter. Re-ranBENCH=1with honest counting → WASM 443 M vs JS 56 M cycles/s = ~7.8× (instr/s, never affected, says ~8.5×). Table + CLAUDE.md updated.src/version.ts).Verification against real WASM (not just
cargo check)Run with
yarn dev:viteserving the rebuilt release build, percpu_test_guidelines.md:src/core/cpu-engines/__tests__suites: 75 passed, 1 skipped (benchmark gate)yarn test:ciwith the server up: 818 passed, 1 skipped — the parity suites ran for real inside the gatecargo checkclean;yarn wasm:testshows only the documented wasm-bindgen native-target panics (pre-existing)LCD: Standard lane, JOURNAL at
docs/lcd/work/wasm-cycle-double-count/.🤖 Generated with Claude Code
https://claude.ai/code/session_01VDq6at6VpsyBxLqB8wBnDT