Skip to content

starknet_patricia_storage: flatten mget chunk keys into one buffer - #15004

Open
gkaempfer wants to merge 2 commits into
mainfrom
claude/perf/starknet-patricia-flatten-keys-05911
Open

starknet_patricia_storage: flatten mget chunk keys into one buffer#15004
gkaempfer wants to merge 2 commits into
mainfrom
claude/perf/starknet-patricia-flatten-keys-05911

Conversation

@gkaempfer

Copy link
Copy Markdown
Contributor

Problem

Follow-up to #15000, which split RocksDbStorage::mget across spawn_blocking tasks for read parallelism on the committer's per-block Patricia witness-fetch hot path (sometimes 100,000+ keys in one call).

Each task built its chunk of keys as Vec<Vec<u8>> by individually cloning every key's Vec<u8>:

let raw_keys: Vec<Vec<u8>> = chunk.iter().map(|key| key.0.clone()).collect();

That's one heap allocation per key, purely to give spawn_blocking's 'static closure owned data — on a 100k-key witness fetch, tens of thousands of small allocations per mget call.

Fix

flatten_keys concatenates a chunk's key bytes into one Vec<u8> buffer plus a Vec<(usize, usize)> of (start, end) byte spans (sized exactly via a single pass, so Vec::with_capacity never reallocates). Each spawn_blocking task now moves in two allocations total instead of one per key; the byte slices are reconstructed from the buffer inside the closure body, after the move, so there's no self-referential-struct concern.

This only touches how the chunk's keys travel into the blocking task — it doesn't change what's queried or the order values come back in.

Scope note (from review)

rust-rocksdb 0.44.1's multi_get_opt still copies each key into its own Box<[u8]> internally before calling into the C API, so this change takes per-key allocations from 2 down to 1 (our clone + RocksDB's own), not to zero. Getting rid of the remaining one would mean switching to batched_multi_get_cf_opt, which hands RocksDB raw pointers into the caller's buffer instead — a larger change (needs a ColumnFamily handle, returns DBPinnableSlice) left as a possible follow-up rather than folded in here.

Testing

RUSTC_WRAPPER="" cargo test -p starknet_patricia_storage --features rocksdb_storage,mdbx_storage — 13/13 pass, including the existing test_mget_preserves_key_order (both chunked and single-task) and a new test_mget_varying_key_lengths covering a chunk that mixes key lengths (1–7 bytes) plus a zero-length key, which would surface any off-by-one in the span bookkeeping. Mutation-checked: corrupting a span's end fails both tests.

Reviewed by an Opus subagent for correctness (span math, closure/lifetime soundness, code-style compliance) before opening this PR; clippy and rust_fmt.sh are clean.

Related: #15000


Generated by Claude Code

claude added 2 commits August 20, 2026 13:14
Each blocking read task in RocksDbStorage::mget cloned every key's Vec<u8>
individually, allocating once per key. On a large witness fetch (100k+
keys) that is tens of thousands of small heap allocations per mget call.
Concatenate a chunk's key bytes into one buffer plus (start, end) spans
instead, so each task moves into spawn_blocking with two allocations
total rather than one per key.
Clarify flatten_keys' doc comment on span semantics (half-open, zero-length
keys), and add test_mget_varying_key_lengths to cover a chunk mixing key
lengths including an empty key, mutation-verified against a corrupted span.
@cursor

cursor Bot commented Aug 20, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
Performance-only refactor of how keys are passed into existing multi_get; behavior is covered by new and existing mget order tests.

Overview
RocksDbStorage::mget no longer clones each key into its own Vec before spawn_blocking. A new flatten_keys helper packs a chunk’s key bytes into one buffer plus (start, end) spans, so each blocking task moves two allocations instead of one per key on large witness fetches.

Query semantics and result ordering are unchanged; slices are rebuilt inside the closure from the flattened buffer.

Adds test_mget_varying_key_lengths (chunked and single-task) with mixed key lengths (1–7 bytes) and a zero-length key to catch span bookkeeping bugs.

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

@reviewable-StarkWare

Copy link
Copy Markdown

This change is Reviewable

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.

4 participants