perf(l1): move trie traversal state out of Nibbles into a cursor - #7173
perf(l1): move trie traversal state out of Nibbles into a cursor#7173MegaRedHand wants to merge 10 commits into
Nibbles into a cursor#7173Conversation
`get_node_inner`'s extension arm built the child's database key from `partial_path`, which `skip_prefix` had already advanced past the prefix. The resulting key was `remaining ++ prefix` rather than the root-relative path the node is stored under, so any lookup crossing an extension node with a hashed child failed with `InconsistentTree` instead of returning the node. The branch arm alongside it already derives the key from `current_path`; do the same here. Reachable from the snap-sync `GetTrieNodes` server path, which had no test coverage. Adds tests for a compact partial path crossing an extension node, the full-32-byte path shape, and the paths that should return an empty vec.
The trie had no microbenchmark, so allocation work on the descent path had no number to move. Covers lookups that hit and miss, insert, remove, and root hashing over a 50k-leaf trie with deterministic 32-byte keys, which puts leaves at depth 6-7 like the real state trie. Reads go through a re-opened trie so they take the node-traversal path rather than the in-memory node cache, and a sanity check at fixture build time asserts hits hit and misses miss: a `get_hit` that silently always missed would measure a truncated descent and read as a speedup.
`Nibbles` played two unrelated roles. It was an owned path, stored in `LeafNode::partial` and `ExtensionNode::prefix` and used as a database key, and it was also a mutable traversal cursor: `next_choice` and `skip_prefix` popped nibbles off the front of `data` and pushed them onto an `already_consumed` field, which `current()` then cloned to reconstruct the visited node's database key. That cost an allocation per node visited on the hottest path in the client, forced five hand-written comparison impls whose only job was to hide `already_consumed` from equality, and left node prefixes built via `offset`/`concat` carrying a stale copy of somebody else's traversal state. Split the roles. `Nibbles` is now an immutable single-vector path with derived comparison impls. A new `PathCursor<'a>` owns traversal: it is `Copy` and allocation-free, `consumed()` yields the visited node's root-relative path as a slice and `remaining()` what is left to match. `TrieDB::get` and `flatkeyvalue_computed` take `&[u8]` so `consumed()` never has to be materialized, and `already_consumed` is gone. Measured with a counting allocator over a 50k-leaf trie, averaged across 64 keys: a lookup that hits drops from 25.6 allocations to 18.9, one that misses from 19.6 to 14.0, an insert from 30.0 to 22.0, and a remove from 68.8 to 36.9. The saving grows with depth, since it is the per-level key that is gone: 19.8 to 14.6 at 1k leaves, 31 to 23 at 1M. Key construction for the account trie is now allocation-free end to end; the descent still allocates about one vector per level because `TrieDB::get` returns `Option<Vec<u8>>`, which a borrowed read API would be needed to remove. On the new microbenchmark, interleaved against the pre-refactor binary and taking the minimum of six rounds, lookups are ~8% faster, insert ~11% and remove ~23%, the last because `BranchNode::remove` was cloning both vectors on every call. Root hashing is unchanged, correctly: `commit` builds its paths from a root whose `already_consumed` was empty, so the old clone was free there. Closes #5825
Review follow-ups on the cursor introduced in the previous commit. `advanced` computed `self.idx + n` and only then asserted the result was in bounds. Release builds have no overflow checks, so a wrapped index would sail past that assert and hand out silently wrong `consumed()` and `remaining()` slices, which is the exact failure the assert exists to prevent. It now checks the addition itself. Dropped the derived `PartialEq`/`Eq`: they compared `(nibbles, idx)`, so two cursors denoting the same position over different backing paths compared unequal and nothing in the tree wanted either meaning. Added `Nibbles::cursor()` and `From<&Nibbles>`, so call sites read `path.cursor()` rather than `PathCursor::new(path.as_ref())`. `trie_sorted` relied on `Nibbles::next()` returning `None` on an empty path; `offset(1)` panics instead. The invariant that the path is exactly one nibble does hold, so this makes it explicit with a `debug_assert` and says in the comment that a violation now panics. Also switches an eager `ok_or(TrieError::Verify(format!(..)))` in `verify_range` to `ok_or_else`, and updates the five blocks of `docs/perf/architecture.md` that still documented `already_consumed` as a live field, one of which proposed this design as a recommended fix.
`NodeRef::commit` built `path.append_new(choice)` for all sixteen branch choices, but `commit` on a `NodeRef::Hash` hands the hash straight back without touching `acc`, and the loop discards the return value. For absent and already-hashed choices that allocation was pure waste, up to sixteen per branch node. The shape this matters for is the one a real block commit has: a trie reopened from its root hash and mutated, where the touched path is in memory and every sibling is a hash. There, commit allocations fall by about 55%: 299 to 123 over a 1000-leaf trie with five touched keys, 51 to 21 over a 16-leaf trie with one. A full fresh build, where most children are in memory anyway, falls by about 28%. A branch with all sixteen children resident is unchanged, as expected. Verified equivalent over fourteen trie shapes, including five reopen-and-mutate cases: identical root hashes and identical `acc` contents in push order.
Every trie proptest used `Trie::new_temp()` and inserted into it, which leaves every node an in-memory `NodeRef::Node`; `commit` does not evict them, so the descent walked the node graph and `TrieDB::get` was never called with a path key at all. Instrumenting the old shape over eight keys counts zero database reads against forty-two for the shape added here. That is why the `Trie::get_node` key bug earlier in this branch shipped: nothing exercised the code it broke. Adds a proptest that builds a trie, commits it, reopens it from the root hash so the root is a `NodeRef::Hash` and every child must be fetched, then reads every key, some absent keys, proofs and a full iteration against the still-in-memory trie as oracle. It then mutates the reopened trie and compares the root against a from-scratch build, which is what catches a wrong key during `insert`/`remove`: a descent that reads the wrong node takes a different structural branch and lands on a different root. Finally it reopens from the mutated root, which covers write keys. Uniformly random 32-byte keys diverge in the first nibble or two and barely produce extension nodes, which is the node type the shipped bug lived under. So the generator tags three key families by first nibble, making them provably disjoint, and uses `btree_set` to guarantee at least two distinct keys in each of the 20- and 31-byte-shared-prefix families. Extension nodes spanning 39 and 61 nibbles are therefore a property of the generator rather than a lucky draw, and the test asserts their presence with an oracle that computes keys from `Nibbles` independently of the cursor under test. Confirmed the test fails against a deliberately wrong read key, both on its read assertions and, with those neutralised, on the mutated-root comparison alone.
`apply_prefix_bytes` expanded the account hash into a temporary `Nibbles` only to copy it into the output vector and drop it. It runs once per level for every storage-trie node read, so expand the 32 bytes straight into the output instead. The layout is unchanged, which is the whole risk here: `Nibbles::from_bytes` appends a leaf-flag nibble, so a prefixed key is 64 nibbles, then 16, then the 17 separator, then the path. The accompanying test keeps the previous expression verbatim as an oracle over five hash/path pairs and pins the layout offsets, and was checked to fail if the nibble order is swapped.
MegaRedHand
added
the
performance
|
Lines of code reportTotal lines added: Detailed view |
This branch added its unit tests inline, as `#[cfg(test)]` modules beside the code. The repository keeps tests in the `ethrex-test` crate instead, one file per area under `test/tests/<subsystem>/`, aggregated through a `mod.rs`, so put them where they belong: crates/common/trie/path_cursor.rs -> test/tests/trie/path_cursor_tests.rs crates/storage/layering.rs -> test/tests/storage/layering_tests.rs Test counts move accordingly and nothing is lost: `ethrex-trie` goes from 70 back to its pre-branch 61, `ethrex-storage` from 92 to 91, and the integration binary picks up the same ten (trie 67 to 76, storage 24 to 25). The bodies are unchanged apart from unwrapping the `mod tests` indentation and importing through the public crate root. `apply_prefix_bytes` is now re-exported next to `apply_prefix`, which it is the borrowing counterpart of, so its test can reach it from outside the crate. The pre-existing inline tests in `layering.rs` stay put: they cover private items and predate this branch.
Benchmark Block Execution Results Comparison Against Main
|
# Conflicts: # crates/common/trie/db.rs # test/tests/storage/mod.rs
The merge adaptation left rustfmt-nonconforming wrapping in trie.rs, the main-side multi_get order test still called TrieDB::get with an owned Nibbles, and the changelog enforcer wants an entry for perf PRs.
2 participants
Motivation
Nibblesplayed two unrelated roles. It was an owned path, stored inLeafNode::partialandExtensionNode::prefixand used as a database key, and it was also a mutable traversal cursor:next_choice/skip_prefixpopped nibbles off the front ofdataand pushed them onto analready_consumedfield, whichcurrent()then cloned to reconstruct the visited node's database key.That cost an allocation per node visited on the hottest path in the client, forced five hand-written comparison impls whose only job was to hide
already_consumedfrom equality, and left node prefixes built viaoffset/concatcarrying a stale copy of somebody else's traversal state.Description
Splits the two roles:
Nibblesis now an immutable single-vector path with derivedPartialEq/Eq/PartialOrd/Ord/Hash.current,next,next_choiceandskip_prefixare gone.PathCursor<'a>owns traversal. It isCopyand allocation-free;consumed()yields the visited node's root-relative path as a slice (which is its database key) andremaining()what is left to match.TrieDB::getandflatkeyvalue_computed, plusNodeRef::get_node{,_checked,_mut}, take&[u8], soconsumed()never has to be materialized into an owned path.Along the way it fixes a latent bug and adds the microbenchmark the trie was missing.
Seven commits, each standalone:
fix(l1)Trie::get_node's extension arm built the child's DB key frompartial_path, whichskip_prefixhad already advanced past the prefix, givingremaining ++ prefixinstead of the root-relative path the node is stored under. Any lookup crossing an extension node with a hashed child failed withInconsistentTree. Reachable from the snap-syncGetTrieNodesserver path, which had no test coverage at all.bench(l1)perf(l1)refactor(l1)advanced, dropping an ambiguous derivedPartialEq, aNibbles::cursor()bridge, an explicit invariant intrie_sorted, and the stalealready_consumeddocumentation indocs/perf/architecture.md.test(l1)perf(l1)NodeRef::commitbuilt a child pathVecfor all 16 branch choices, including absent and already-hashed ones, butcommiton aNodeRef::Hashis a no-op whose return value the loop discards.perf(l1)apply_prefix_bytesbuilt a temporaryNibblesonly to copy it into the output.The last two are independent of the refactor and can be dropped without touching the rest.
Numbers
Allocations per operation, counting
GlobalAlloc, 50k-leaf trie, mean over 64 keys:gethitgetmissinsertremoveThe saving on lookups grows with depth, since it is the per-level key that is gone: 19.8 → 14.6 at 1k leaves, 25.6 → 18.9 at 50k, 31 → 23 at 1M.
Commit-path allocations, from the
NodeRef::commitchange, measured over the shape a real block commit has (a trie reopened from its root hash and mutated, so the touched path is in memory and its siblings are hashes):The last row is the control: nothing is skipped when every child is in memory.
Wall clock, interleaved A/B against the pre-refactor binary, min of 6 rounds:
get_hitget_missinsert(64 keys)remove(64 keys)root_hash(64 dirty)Two caveats worth stating plainly:
Vecper level, becauseTrieDB::getreturnsOption<Vec<u8>>. What this PR removes is the key allocation per level; for the account trie (no address prefix) key construction is now allocation-free end to end. Removing the value allocation needs a borrowed read API, which is out of scope here.Trie::getshort-circuits to a singledb.getand never descends, so the ~7-8% lands on pre-FKV storage tries, proofs and witnesses,get_node, and healing.insert,removeand merkleization always descend, so those carry over directly.root_hashis unchanged by the refactor itself, correctly:commitbuilds paths from a root whosealready_consumedwas empty, so the old clone was free there. The commit-path allocation win above is a separate change and is not reflected in this benchmark, which callshash_no_commit.Correctness
The traversal rewrite was checked by building the same differential harness against
bf94de769and against this branch: 420 randomized cases, each one building a trie, committing, re-opening from the root hash so every read goes throughTrieDB::get, then comparing root hashes, mutated-root hashes, proof digests and iteration digests. Key shapes included 20- and 31-byte shared prefixes so deep extension nodes actually form. Output is byte-identical apart fromget_node, where base fails and this branch succeeds — the bug fixed in the first commit.Database key layout is unchanged, byte for byte, in both directions: every one of the 23 old
current()sites maps ontoconsumed(), and read keys still equal whatNodeRef::commitwrites at every level. Length-based column-family dispatch (classify_trie_key) sees the same lengths. A node with a database written by the previous binary reads correctly.ethrex-prover,ethrex-guest-program, and the separate-workspacestateless-validatoracross all four of its feature combinations were built explicitly, sincemake lint-l2excludes the first two and--workspacenever sees the third. Theno_stdriscv64 zkVM target builds clean.Test coverage gap this exposed
Every pre-existing trie proptest used
Trie::new_temp(), which leaves all nodes as in-memoryNodeRef::Node;commitdoes not evict them, so the descent never calledTrieDB::getwith a path key. That is why theget_nodebug shipped. This PR adds a proptest that commits, re-opens from the root hash, mutates, and compares the root against a from-scratch build, so the path-based read surface is actually exercised.Breaking changes
TrieDB::getandTrieDB::flatkeyvalue_computedchange signature, so out-of-tree implementors need updating (ethrex-replayis named indb.rs's comments).NodeRef::get_node/get_node_checked,Trie::get_root_node, and theNode/BranchNode/ExtensionNode/LeafNodetraversal methods likewise. Removingalready_consumedchanges the rkyv and serde shape ofNibbles; the only durable consumer is the L2batch_prover_inputblob, which is keyed by git commit hash, so a binary built from this commit never reads an old-layout row and the miss path regenerates the witness. RLP is unaffected: it only ever encodeddata.Follow-ups found while reviewing, deliberately not fixed here
ExtensionNode::insert,match_index == 0arm withprefix == [16], reads its child at this node's own path instead ofown_path ++ prefix— the same bug family as the one fixed here, but pre-existing and needing its own test.Trie::get_nodenever returns a node for the full-32-byte-path shape it advertises:from_bytesappends the leaf flag, sopartial_pathis never empty on arrival at a leaf. The snap-sync server answers a full account hash with an empty node.< 16, so a path terminating at a branch's own value returns empty;branch.valueis never consulted.Trie::get_root_node's path parameter is vestigial: all callers pass an empty key.Closes #5825