Skip to content

starknet_patricia: actually pre-allocate filled-tree output map capacity#14822

Open
gkaempfer wants to merge 2 commits into
mainfrom
claude/wizardly-archimedes-4bx9fr
Open

starknet_patricia: actually pre-allocate filled-tree output map capacity#14822
gkaempfer wants to merge 2 commits into
mainfrom
claude/wizardly-archimedes-4bx9fr

Conversation

@gkaempfer

Copy link
Copy Markdown
Contributor

Summary

Follow-up fix on top of #14638, which merged with the stated goal of pre-allocating initialize_filled_tree_output_map_with_placeholders's output HashMap to avoid O(log N) rehashing during tree fill — a hot path called once per block per Merkle tree (storage + class tries).

That PR's implementation doesn't actually achieve the goal: it collects through a .filter_map(...).collect() chain, but Iterator::filter_map's size_hint() always reports a lower bound of 0 (verified empirically — a HashMap::iter() filtered through filter_map degrades from an exact size_hint to (0, Some(n))). HashMap's Extend impl (which backs .collect()) reserves capacity based on exactly that lower bound, so the merged code reserves zero extra capacity up front and still rehashes while filling — the same cost the PR claimed to remove.

Fix

  • Capture size_hint() from the raw get_nodes() iterator (exact, since it comes from a HashMap::iter()) before applying .filter_map, and explicitly HashMap::with_capacity(...) + .extend(...) instead of relying on .collect()'s internal reservation.
  • Document on the get_nodes trait method that callers rely on this exact size_hint, so a future implementation wrapping it in a filtering adapter doesn't silently regress the optimization again.
  • Note the deliberate (and bounded) over-allocation for UnmodifiedSubTree nodes that get filtered out, as a documented tradeoff against a separate counting pass.

Verification

  • cargo build -p starknet_patricia — clean.
  • SEED=0 cargo test -p starknet_patricia — 107 passed, 0 failed.
  • Reviewed by an independent Opus pass; no blocking issues, two suggestion-level items addressed (documentation of the size_hint contract and the over-allocation tradeoff).

🤖 Generated with a scheduled Claude Code review of merged PR #14638.


Generated by Claude Code

claude added 2 commits July 15, 2026 11:03
PR #14638 intended to pre-allocate initialize_filled_tree_output_map_with_placeholders's
HashMap to avoid O(log N) rehashing while filling the tree (once per block, per tree),
but the filter_map().collect() it introduced doesn't achieve this: FilterMap::size_hint()
always reports a lower bound of 0, and HashMap's Extend impl reserves capacity from that
lower bound, so no pre-allocation happens. Reserve capacity from the raw node iterator
(exact size_hint, since it comes from a HashMap::iter()) before filtering instead.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015cTjgteomFa2zu4xh6L2yJ
…adeoff

Address Opus review feedback on the prior commit: document on the
get_nodes trait method that callers rely on an exact size_hint (so
future implementations don't silently regress the pre-allocation),
note the deliberate over-allocation for filtered UnmodifiedSubTree
nodes, and rename the local iterator binding for clarity.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015cTjgteomFa2zu4xh6L2yJ
@cursor

cursor Bot commented Jul 15, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
Performance-only change on Patricia Merkle tree map initialization with no auth, persistence, or API behavior changes; slight bounded memory over-reservation for filtered skeleton nodes.

Overview
Fixes a follow-up on merged pre-allocation work: building the filled-tree placeholder map via .filter_map(...).collect() never reserved capacity because filter_map forces size_hint lower bound to 0, so HashMap still rehashed on the per-block Merkle fill hot path.

initialize_filled_tree_output_map_with_placeholders now reads size_hint() from the raw get_nodes() iterator (exact for HashMap::iter()), uses HashMap::with_capacity + extend after filtering out UnmodifiedSubTree nodes, and documents the bounded over-allocation tradeoff vs a counting pass.

UpdatedSkeletonTree::get_nodes is documented so implementations must expose an exact size_hint and must not wrap the iterator in adapters that zero the lower bound, preventing a silent regression of this optimization.

Reviewed by Cursor Bugbot for commit 1eab780. Bugbot is set up for automated code reviews on this repo. Configure here.

@reviewable-StarkWare

Copy link
Copy Markdown

This change is Reviewable

@gkaempfer gkaempfer self-assigned this Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants