perf: eliminate redundant node clones during Trie::recalculate_root_hash()#6935
perf: eliminate redundant node clones during Trie::recalculate_root_hash()#6935cylewitruk-stacks wants to merge 5 commits intostacks-network:developfrom
Trie::recalculate_root_hash()#6935Conversation
There was a problem hiding this comment.
Pull request overview
This PR optimizes MARF trie insert performance by reducing redundant node cloning/writes during Trie::recalculate_root_hash(), especially when computing the root hash with skiplist updates enabled.
Changes:
- Gate the “pre-flush then final write” behavior to only the root node when
update_skiplist == true. - Add
TrieStorageConnection::write_node_hash()to update only a node’s stored hash in the uncommitted trie state (avoiding a full node clone/write). - Minor logging/message and comment hygiene updates in trie operations.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
stackslib/src/chainstate/stacks/index/trie.rs |
Avoids redundant pre-flush writes for non-root ancestors; uses hash-only update for the root’s final write when skiplist updating. |
stackslib/src/chainstate/stacks/index/storage.rs |
Introduces TrieStorageConnection::write_node_hash() to support hash-only updates in uncommitted state. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cylewitruk-stacks
requested review from
aaronb-stacks,
federico-stacks and
jcnelson
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6935 +/- ##
============================================
+ Coverage 59.40% 70.02% +10.61%
============================================
Files 412 412
Lines 218187 218190 +3
Branches 338 338
============================================
+ Hits 129610 152778 +23168
+ Misses 88577 65412 -23165
... and 317 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
federico-stacks
left a comment
federico-stacks
left a comment
There was a problem hiding this comment.
the change makes sense to me. Just added a nit
jcnelson
left a comment
jcnelson
left a comment
There was a problem hiding this comment.
Couple nits but LGTM. Will approve once resolved. Thanks @cylewitruk-stacks!
cylewitruk-stacks
force-pushed
the
perf/marf-put-write-elision
branch
from
f6f9f82 to
107308e
Compare
| ); | ||
| debug!("Next root hash is {h} (update_skiplist={update_skiplist})"); | ||
|
|
||
| storage.write_nodetype(child_ptr.ptr(), &node, h)?; |
There was a problem hiding this comment.
sanity-check: could write_node_hash be used here?
There was a problem hiding this comment.
yeah I think we might since we're not modifying the node in this branch (like we do below) 👍 Will try it and make sure things are green.
benjamin-stacks
left a comment
benjamin-stacks
left a comment
There was a problem hiding this comment.
I'm an absolute noob in our MARF implementation code, but this change makes a lot of sense to me. Maybe take me as half an approval, given that there are probably a lot of edge cases I may not be aware of 😅
One thing I'm wondering: How good is the code coverage of the surface area of this? I assume that any regression would (at the latest) be caught during a full replay test, but I don't have a good intuition on the validity of that assumption.
|
@cylewitruk-stacks please merge then when able. |
Description
Background
Trie::recalculate_root_hash()is called on every MARF insert (regardless of trie hashing mode).Currently, all ancestor nodes are pre-flushed with a zero-hash prior to being written with their final hash, which results in two
write_nodetype()calls (and two full-node clones) per node.The pre-flush exists to materialize a node's child pointers before
get_trie_root_hash()performs Merkle skiplist ancestor lookups, but that lookup only happens for root nodes, and only whenupdate_skiplist == true(i.e., non-Deferredhashing modes). Non-root ancestors never trigger ancestor lookups, and neither does the root inDeferredmode.The Change
This PR gates the two-write pattern on
is_root_node && update_skiplistand thus eliminates one full-node clone per-ancestor-per-insert.Applicable issues
Additional info (benefits, drawbacks, caveats)
Core Optimization:
TrieStorageConnection::write_node_hash()function which updates only a node's stored hash without cloning and overwriting the full node body.write_nodetype()— one clone instead of two (both hash modes).Deferredmode: write final hash directly viawrite_nodetype()— one clone instead of two (no skiplist lookup, no pre-flush needed).Immediate(andAll) modes: child pointers are still pre-flushed viawrite_nodetype()before skiplist hash derivation, but the final write is now a hash-only update viawrite_node_hash()— one clone instead of two.Minor Hygiene:
trace!& comments cleanup.Benchmark Diff:
Using
marf-benchin PR #6932Write path allocation counts down between 3-8%, allocated bytes down 3-18% (with synthetic benchmark data) and some minor improvements in timings.
Checklist