perf(l1): keep the memoize_hashes guard out of the recursive call - #7101
perf(l1): keep the memoize_hashes guard out of the recursive call#7101diegokingston wants to merge 1 commit into
Conversation
`NodeRef::memoize_hashes` already skips subtries whose hash is memoized, and `NodeRef::Hash` references have nothing to memoize at all. But the check sits behind a call: `Node::memoize_hashes` visits all 16 children of a branch, and because the body is recursive LLVM will not inline it, so every child costs a real call even when the answer is immediately 'nothing to do'. Measured on a mainnet block execution witness, 93% of child references are `NodeRef::Hash` or empty slots, and only 7.3% are embedded nodes. Counting calls in a stateless run of block 25368371: `memoize_hashes` was entered 54,810 times while a hash was actually computed 2,094 times — 96.2% of the calls did nothing but test the guard and return. Splitting the guard into an `#[inline]` shell and moving the recursive body to a separate function makes the common case a load and a branch at the call site. Behaviour is unchanged; this is purely where the code lives. Effect is largest where call overhead is not amortised by a big out-of-order core: in the LambdaVM zkVM guest, where the measurement above comes from, this removed 1,480,099 of 26,794,880 cycles (-5.52% of total execution) and `memoize_hashes` disappeared from the profile entirely. Native gains should be smaller, but the change strictly removes work.
github-actions
Bot
added
the
performance
|
🤖 Kimi Code ReviewThe changes represent a correct performance optimization using the "outline cold path" pattern. Analysis:
Minor suggestion (optional): #[cold]
fn memoize_hashes_uncached(...)This further hints to LLVM that the function is unlikely to be called, though the compiler likely infers this from the Verdict: LGTM. Clean optimization with clear documentation. Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Claude Code ReviewReview:
|
Greptile SummaryThe PR preserves trie hash memoization behavior while moving the recursive cold path behind an inline guard to avoid function-call overhead for cached and hash-only references.
Confidence Score: 5/5The PR appears safe to merge because the refactor preserves the existing memoization behavior while optimizing the no-op path. The same uncached-node guard, recursive traversal, hash computation, and
|
| Filename | Overview |
|---|---|
| crates/common/trie/node.rs | Refactors the existing guarded memoization body into a private helper without changing traversal, hashing, buffer use, or cache-update order. |
Reviews (1): Last reviewed commit: "perf(trie): keep the memoize_hashes guar..." | Re-trigger Greptile
🤖 Codex Code ReviewNo findings. The change in crates/common/trie/node.rs looks behavior-preserving: Testing gap: I could not run a local Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
Lines of code reportTotal lines added: Detailed view |
| /// whose hash is not memoized yet, so descend into it and compute it. | ||
| fn memoize_hashes_uncached( |
There was a problem hiding this comment.
Might be worth having an inline(never) to ensure it behaves as expected.
iovoid
changed the title
…lass#7104) **Motivation** `RLPEncode::encode` takes `&mut dyn bytes::BufMut`. Encoding a branch node writes ~41 items through that trait object (16 children — each an RLP length byte plus a 32-byte hash — plus the header and value), so each one is a vtable dispatch. That lands on the hashing path: `BranchNode::compute_hash_no_alloc` encodes into a `Vec<u8>` before keccak, and every `memoize_hashes` walk re-encodes each dirty branch. Note `encode_to_vec` directly below it is *already* monomorphic (`buf.push` / `extend_from_slice`) — it just allocates a fresh buffer each call, so the hashing path can't use it. **Description** Add `BranchNode::encode_into_vec(&self, buf: &mut Vec<u8>)`, a concrete-typed sibling that appends into a caller-owned `Vec<u8>`, and point `compute_hash_no_alloc` at it. `RLPEncode::encode` is untouched for every other caller. Also resolve each child's hash once rather than twice — the payload-length pass and the encode pass each called `compute_hash_ref`, so a 16-choice branch performed 32 resolutions. `cargo test -p ethrex-trie` (61 tests), `fmt` and `clippy` are clean. **Effect** The win scales with how poorly indirect calls are amortised. These numbers come from the [LambdaVM](https://github.com/yetanotherco/lambda_vm) zkVM guest executing mainnet block 25368371, where every RISC-V instruction is proven and there is no out-of-order core to hide a vtable dispatch: | | cycles | |---|---| | `BranchNode::encode` + `put_slice` + `put_u8` | 2,122,427 → 1,312,210 (**−38%**) | | `memcpy` | 908,885 → 586,969 | | whole-block execution | 20,544,222 → 19,408,617 (**−5.53%**) | `memcpy` drops as well because `extend_from_slice` into a `Vec` with known capacity beats the trait object's `put_slice`. Native gains will be considerably smaller — a modern core predicts and pipelines these calls well — but the change strictly removes indirection, so it should not regress. Happy to add a native benchmark before merging if you'd like a number for that path. Companion to lambdaclass#7101 (same guest, same profiling run).
ElFantasma
left a comment
ElFantasma
left a comment
There was a problem hiding this comment.
Correct and behaviour-identical; one durability nit inline, and I don't think a native benchmark is needed since the change strictly removes work.
|
|
||
| /// Cold path of [`Self::memoize_hashes`]: this reference is an embedded node | ||
| /// whose hash is not memoized yet, so descend into it and compute it. | ||
| fn memoize_hashes_uncached( |
There was a problem hiding this comment.
nit: worth pinning this with #[inline(never)], since the whole benefit depends on the two functions staying apart and nothing in the code says so.
The split works because the recursion cycle is cut here: the shell can be inlined into Node::memoize_hashes's 16-child loop precisely because the call to this function is opaque to it. If LLVM ever decides to inline this body back into the #[inline] shell — it's only two calls, so it's not an absurd choice — the shell becomes mutually recursive again (Node::memoize_hashes → shell → here → Node::memoize_hashes), stops being inlinable at the hot call site, and the win silently evaporates. Nothing would fail; the cycle count would just quietly go back up on some future toolchain bump.
#[inline(never)]
fn memoize_hashes_uncached(The doc comment already explains the intent — this just makes the compiler honour it.
One thing I'd avoid: #[cold]. It reads like the natural fit given the 3.8% hit rate, but it also marks the callee optsize, and this is the path that does all the real work (recursing the subtrie and hashing it). Optimising it for size would pessimise exactly the calls that matter.
4 participants
Motivation
NodeRef::memoize_hashesalready skips subtries whose hash is memoized, andNodeRef::Hashreferences have nothing to memoize at all. The problem is where that check lives:Node::memoize_hashesvisits all 16 children of a branch node, and because the body is recursive LLVM will not inline it — so every child costs a real function call even when the answer is immediately "nothing to do".How lopsided that is, measured on a real mainnet block execution witness (block 25368371):
NodeRef::Node(embedded, must be hashed)NodeRef::Hash(hash only, nothing to do)So ~93% of the references a branch iterates over have no work behind them. Counting actual calls during a stateless execution of that block:
Description
Split the guard into an
#[inline]shell and move the recursive body into a separate function, so the overwhelmingly common no-op case becomes a load and a branch at the call site instead of a call.Behaviour is unchanged — this is purely about where the code lives.
cargo test -p ethrex-triepasses (61 tests),fmtandclippyare clean.Effect
The win scales with how poorly the call overhead is amortised. The measurement above comes from the LambdaVM zkVM guest, where every executed RISC-V instruction is proven and there is no out-of-order core to hide the call:
memoize_hashesdisappeared from the guest profile entirely (1,786,030 → 0 self cycles), and the trie went from 20.5% to 15.9% of the block.Native gains will be considerably smaller — a modern core predicts and pipelines these calls well — but the change strictly removes work, so it should not regress anywhere. Happy to add a native benchmark if you'd like a number for that path before merging.