Skip to content

starknet_patricia: actually preallocate filled-tree output map capacity#14809

Open
gkaempfer wants to merge 1 commit into
mainfrom
claude/perf/starknet_patricia-filled-tree-capacity-73412
Open

starknet_patricia: actually preallocate filled-tree output map capacity#14809
gkaempfer wants to merge 1 commit into
mainfrom
claude/perf/starknet_patricia-filled-tree-capacity-73412

Conversation

@gkaempfer

Copy link
Copy Markdown
Contributor

Follow-up to #14638, which intended to fix an O(log N) rehashing hot path in initialize_filled_tree_output_map_with_placeholders (called once per block per tree — storage trie and class trie) by preallocating the output HashMap's capacity.

That PR's fix doesn't actually work: it switched to updated_skeleton.get_nodes().filter_map(...).collect(). Rust's FilterMap iterator adapter always reports a size_hint() lower bound of 0, because the predicate can drop every item — so HashMap's FromIterator/extend reserves 0 capacity up front, and the map still grows via repeated reallocation and rehashing as elements are inserted, exactly the cost the PR meant to eliminate.

This PR reserves capacity from the unfiltered node iterator instead — updated_skeleton.get_nodes() wraps a HashMap::iter(), which is an ExactSizeIterator, so its size_hint().0 is the true node count. We call HashMap::with_capacity on that count, then .extend() the filter_map'd iterator into the presized map. This reserves an upper bound (it includes UnmodifiedSubTree nodes that get filtered out), which is a good tradeoff — a bit of extra headroom is far cheaper than the rehash cascade it avoids.

No behavior change: output map contents are identical, only the allocation strategy differs.

Verified:

  • cargo build -p starknet_patricia — success
  • SEED=0 cargo test -p starknet_patricia — 107 passed, 0 failed
  • cargo clippy -p starknet_patricia --all-targets — clean
  • scripts/rust_fmt.sh — clean
  • Reviewed by an independent agent pass; no blocking issues found

Generated by Claude Code

PR #14638 switched initialize_filled_tree_output_map_with_placeholders to
build the map via filter_map(...).collect(), intending to preallocate
capacity from the node count. But FilterMap's size_hint lower bound is
always 0 (the predicate can drop every item), so collect() through it
still starts the HashMap at zero capacity and rehashes as nodes are
inserted -- the same O(log N) rehash cost the PR meant to eliminate.
This is a hot path, invoked once per block per tree (storage + class).

Reserve capacity from the raw (unfiltered) node iterator, which has an
exact size_hint since it wraps a HashMap::iter(), then extend into the
presized map.
@cursor

cursor Bot commented Jul 15, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
Single-function allocation tweak in merkle tree setup with no logic or API changes; slightly over-reserved capacity is bounded by skeleton node count.

Overview
Fixes ineffective HashMap preallocation in initialize_filled_tree_output_map_with_placeholders, which runs once per block per Patricia tree (storage and class tries).

The prior approach built the placeholder map via get_nodes().filter_map(...).collect(). FilterMap always reports a size_hint lower bound of 0, so HashMap still started at zero capacity and grew through rehashing—the performance issue #14638 meant to remove.

This change reserves capacity from the unfiltered get_nodes() iterator (backed by HashMap::iter(), an exact-size iterator), then extends the same filter_map logic into that map. Capacity is an upper bound because UnmodifiedSubTree entries are skipped; extra headroom is intentional. Map contents and behavior are unchanged—only allocation strategy differs.

Reviewed by Cursor Bugbot for commit 25a877f. 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