perf(l1): track trie path position with a cursor instead of a second vector - #7162
perf(l1): track trie path position with a cursor instead of a second vector#7162diegokingston wants to merge 1 commit into
Conversation
…vector
`Nibbles` kept the traversal position by moving nibbles between two vectors:
`data` held what was left to match and `already_consumed` what had been walked
past. Their concatenation is invariant, so every advance paid to shift bytes
between them:
next() -> data.remove(0) memmove of the whole remaining path
skip_prefix() -> data.drain(..n) memmove, plus extend of already_consumed
offset(n) -> two fresh vectors and two copies
`next()` runs once per branch descended, so walking a 64-nibble path did 64
memmoves of up to 64 bytes to consume 64 nibbles — quadratic work for what is
logically an index bump.
Keep one buffer and a cursor instead: `data[..cursor]` is consumed (what
`current()` returns) and `data[cursor..]` remains. Advancing is now an integer
add, and `offset` allocates once rather than twice.
The public API and its semantics are unchanged. Every method that reads "the
nibbles" still means the remaining ones, including `len`, `at`, `is_leaf`,
`AsRef<[u8]>`, the comparison impls and the compact encoding.
Correctness: the existing suite passes, and a differential harness comparing
this against a transcription of the previous implementation ran 115,975
randomized operations over 3,161,843 assertions with no divergence, covering
`next`, `skip_prefix`, `offset`, `slice`, `concat`, `append_new`, `current`,
`take`, `prepend`, `extend`, the ordering/equality/hash impls and the compact
encoding.
Measured on zkVM guests, where allocation and memmove are executed work:
- ZisK, trie-heavy workload (1200-leaf trie, 1200 lookups, full hash):
6,956,595 -> 6,751,333 steps (-2.95%), `dma_memcpy` -22.8%, RAM use -7.4%,
with an identical keccak count, so no work is skipped.
- LambdaVM, transfer blocks: -0.26% to -0.74%. Small, as expected — those
blocks barely touch the trie and are dominated by signature recovery.
- Host, 1200-account trie: 12.5% fewer allocations.
Note for integrators: `Nibbles` derives `rkyv::Archive`, so replacing
`already_consumed: Vec<u8>` with `cursor: usize` changes the archived layout.
Anything holding pre-serialized `ExecutionWitness` bytes has to regenerate
them; host and guest built together are unaffected.
|
github-actions
Bot
added
L1
🤖 Kimi Code ReviewThis is a well-structured optimization PR that replaces the Issues Found1.
|
🤖 Codex Code Review
The rest of the path-tracking rewrite looks internally consistent: I couldn’t run the Rust tests in this environment because Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
MegaRedHand
left a comment
MegaRedHand
left a comment
There was a problem hiding this comment.
LGTM. There's probably a better way to address this issue, though
| Encoder::new(buf).encode_field(&self.data).finish(); | ||
| } else { | ||
| Encoder::new(buf) | ||
| .encode_field(&self.as_ref().to_vec()) |
There was a problem hiding this comment.
Does this need the to_vec?
| } | ||
| // ───────────────────────────────────────────────────────────────────────────── | ||
|
|
||
| // TODO: move path-tracking logic somewhere else |
🤖 Claude Code ReviewReview: perf(l1): track trie path position with a cursor instead of a second vectorCorrectnessI traced every mutator of the new A few spots worth double-checking (I verified all are correct):
No functional bugs found in the refactor itself. Test coverage gapThe PR description cites an extensive differential-testing harness (115,975 randomized operations, 3.16M assertions) validating this against a transcription of the old implementation — but that harness isn't part of the diff. The Stale docs
Minor
Overall: a clean, well-reasoned allocation-elimination refactor with matching semantics and a real complexity win (O(1) Automated review by Claude (Anthropic) · sonnet · custom prompt |
2 participants
Motivation
Nibbleskeeps the traversal position by moving nibbles between two vectors:dataholds what is left to match,already_consumedwhat has been walked past. Their concatenation is invariant, so every advance pays to shift bytes between them:next()runs once per branch descended, so walking a 64-nibble path does 64 memmoves of up to 64 bytes to consume 64 nibbles — quadratic work for what is logically an index bump.Description
Keep one buffer and a cursor:
data[..cursor]is consumed (whatcurrent()returns) anddata[cursor..]remains. Advancing becomes an integer add, andoffsetallocates once instead of twice.The public API and its semantics are unchanged. Every method that reads "the nibbles" still means the remaining ones —
len,at,is_leaf,AsRef<[u8]>, the comparison impls and the compact encoding.Measurements
On zkVM guests, where allocation and memmove are executed work:
dma_memcpy−22.8%, RAM use −7.4%. The keccak count is identical before and after, so no work is skipped, and the guest's committed output is byte-identical.The argument for the change is mostly the algorithmic one — it removes a quadratic memmove from every traversal — rather than the size of the win on transfer-heavy blocks.
Validation
The existing suite passes (61 trie tests, plus
ethrex-storageandethrex-common),clippyclean.Beyond that, a differential harness compared this against a verbatim transcription of the previous implementation: 115,975 randomized operations over 3,161,843 assertions, no divergence. It covers
next,skip_prefix,offset,slice,concat,append_new,current,take,prepend,extend, the ordering/equality/hash impls and the compact encoding.Note for integrators
Nibblesderivesrkyv::Archive, so replacingalready_consumed: Vec<u8>withcursor: usizechanges the archived layout. Anything holding pre-serializedExecutionWitnessbytes has to regenerate them; host and guest built together are unaffected. As a side effect the serialized witness gets slightly smaller.This touches the same file as #7155, so whichever lands second will need a trivial rebase.