Skip to content

Decode the opcode header once per instruction via RuntimeOp.seek - #21602

Open
NullVoxPopuli wants to merge 1 commit into
emberjs:mainfrom
NullVoxPopuli:nvp/perf/runtime-op-seek
Open

NullVoxPopuli wants to merge 1 commit into
emberjs:mainfrom
NullVoxPopuli:nvp/perf/runtime-op-seek

Conversation

@NullVoxPopuli

@NullVoxPopuli NullVoxPopuli commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

RuntimeOpImpl now decodes type, size, and isMachine in a single seek(offset) call instead of re-reading the heap in three getters.

The RuntimeOp interface fields become readonly and seek is the only way to move the cursor.

Split out of nvp/simplify-some-vm-hot-paths.

Co-Authored-By: Claude Fable 5.1 <[email protected]>
@NullVoxPopuli NullVoxPopuli changed the title [PERF] Decode the opcode header once per instruction via RuntimeOp.seek Decode the opcode header once per instruction via RuntimeOp.seek Sep 9, 2026
@NullVoxPopuli

Copy link
Copy Markdown
Contributor Author

Benchmark: main (bdb2580) vs nvp/perf/runtime-op-seek (a922570)

Ran via pnpm bench (the krausest js-framework-benchmark scenario in smoke-tests/benchmark-app, tracerbench, 20 samples each, 8x CPU throttle). Negative delta means the PR is faster. Only rows tracerbench marks significant (p < 0.05) are meaningful.

Phase main median PR median Δ (estimator) 95% CI Significant
Total duration 9,497ms 9,519ms 8ms (+0.1%) -444ms .. 410ms
render 143ms 138ms -3ms (-2.3%) -9ms .. 3ms
render1000Items1 348ms 329ms -16ms (-4.7%) -34ms .. -2ms
clearItems1 37ms 38ms 0ms (-1.2%) -2ms .. 2ms
render1000Items2 292ms 294ms 1ms (+0.5%) -16ms .. 16ms
clearItems2 135ms 135ms 0ms (-0.3%) -2ms .. 2ms
render10000Items1 2,742ms 2,718ms -40ms (-1.5%) -183ms .. 43ms
clearManyItems1 369ms 370ms 1ms (+0.3%) -6ms .. 8ms
render10000Items2 2,732ms 2,730ms 1ms (+0.1%) -126ms .. 129ms
clearManyItems2 264ms 261ms -3ms (-1.1%) -11ms .. 3ms
render1000Items3 260ms 258ms -7ms (-2.7%) -25ms .. 10ms
append1000Items1 346ms 364ms 14ms (+4.0%) -23ms .. 53ms
append1000Items2 320ms 325ms 4ms (+1.4%) -15ms .. 33ms
updateEvery10thItem1 286ms 290ms 4ms (+1.5%) -16ms .. 23ms
updateEvery10thItem2 305ms 307ms -2ms (-0.6%) -30ms .. 19ms
selectFirstRow1 75ms 79ms 2ms (+2.5%) -6ms .. 10ms
selectSecondRow1 59ms 60ms 0ms (+0.2%) -9ms .. 8ms
removeFirstRow1 194ms 193ms 5ms (+2.4%) -7ms .. 15ms
removeSecondRow1 183ms 191ms 12ms (+6.3%) -1ms .. 28ms
swapRows1 131ms 127ms -1ms (-0.9%) -12ms .. 13ms
swapRows2 124ms 127ms 3ms (+2.2%) -8ms .. 15ms
clearItems4 104ms 106ms 2ms (+2.0%) -1ms .. 5ms
paint 9ms 11ms 1ms (+7.4%) -3ms .. 4ms

@johanrd

johanrd commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

@NullVoxPopuli FWIW: Claude found flaws (but for VM_LOCAL_DEV only) Recorded as two failing test here: johanrd#41

@NullVoxPopuli

NullVoxPopuli commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

@johanrd can we trust that those very private tests are valid?

afaik, the test setup could be wrong?

@johanrd

johanrd commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

@NullVoxPopuli true, though, the tests were created to prove this failure:

  1. debug.ts ends the walk with opcode.seek(-_size)
  2. _size comes from size = ((header & OPERAND_LEN_MASK) >> ARG_SHIFT) + 1, so it is never 0 → the argument is always <= -1
  3. console.log(new Int32Array(8)[-1]) // undefined

@NullVoxPopuli

Copy link
Copy Markdown
Contributor Author

right, but would an app ever do run in to that?

@johanrd

johanrd commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

no, it wont affect real apps, it only affectsVM_LOCAL_DEV (trace logging is perhaps already not working?)

Maybe just delete the line, not port it? (the op seems to be discarded immediately after, nothing reads it? – not very familiar with opcodes, sorry.)

@johanrd

johanrd commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

@NullVoxPopuli else, very cool that you have found these improvements to the vm (also the other prs!). Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants