Skip to content

perf(l1): keep the memoize_hashes guard out of the recursive call #7101

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
diegokingston wants to merge 1 commit into
base: main
Choose a base branch
from
Open

perf(l1): keep the memoize_hashes guard out of the recursive call #7101

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 23 additions & 2 deletions crates/common/trie/node.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,15 +323,36 @@ impl NodeRef {
}
}

/// Memoizes this reference's subtrie hash, if there is anything to do.
///
/// The guard is deliberately split from the recursive body. A branch node
/// visits all 16 of its children, but the vast majority of child references
/// are `NodeRef::Hash` or empty slots with nothing to memoize (measured on a
/// mainnet block witness: ~93%). Because the body is recursive LLVM will not
/// inline it, so keeping the guard in a separate `#[inline]` shell makes the
/// overwhelmingly common no-op case a load and a branch at the call site
/// instead of a full call.
#[inline]
pub fn memoize_hashes(&self, buf: &mut Vec<u8>, crypto: &dyn Crypto) {
if let NodeRef::Node(node, hash) = &self
&& hash.get().is_none()
{
node.memoize_hashes(buf, crypto);
let _ = hash.set(node.compute_hash_no_alloc(buf, crypto));
Self::memoize_hashes_uncached(node, hash, buf, crypto);
}
}

/// 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(
Comment on lines +345 to +346

@iovoid iovoid Aug 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth having an inline(never) to ensure it behaves as expected.

@ElFantasma ElFantasma Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

node: &Arc<Node>,
hash: &OnceLock<NodeHash>,
buf: &mut Vec<u8>,
crypto: &dyn Crypto,
) {
node.memoize_hashes(buf, crypto);
let _ = hash.set(node.compute_hash_no_alloc(buf, crypto));
}

/// Resets the memoized hash of this Node
///
/// This is used when mutating a node in place, in which case the memoized hash
Expand Down
Loading