perf(executor): back the private-input region with a paged Box map#852
perf(executor): back the private-input region with a paged Box map#852Oppen wants to merge 8 commits into
Conversation
MAX_PRIVATE_INPUT_SIZE is 512 MiB (#843), but Memory::cells stores every guest word (program/stack/heap AND private input) in one HashMap<u64, [u8;4]>. A max-size private input costs ~134M entries there -- measured peak RSS ~9.7 GB for a single store_private_inputs call, and store_private_inputs itself takes ~2.2s. Route the private-input address range to a separate HashMap<u64, Box<[u8; 256 KiB]>>, one boxed page allocated lazily per touched page. Only an 8-byte pointer lives in the hashmap's table slot, so growing/rehashing the table only ever moves pointers, never page payloads -- avoiding the resize-memcpy cost a same-size inline-array page value would pay. Measured: peak RSS ~1.08 GB (vs ~9.7 GB), store_private_inputs ~28ms (vs ~2.2s), for the same 512 MiB input. Guest-visible behavior is unchanged; all load/store paths route by address only.
|
/ai-review |
Codex Code Review
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| let page_idx = private_page_index(address); | ||
| let offset = private_page_offset(address); | ||
| let page = self | ||
| .private_input_pages |
There was a problem hiding this comment.
store_byte is the only private-write path that uses infallible allocation (or_insert_with(|| Box::new([0u8; PRIVATE_INPUT_PAGE_SIZE]))), while store_word/store_doubleword/store_half/set_private_bytes_aligned all go through the fallible get_or_insert_private_page → try_allocate_private_page. Two consequences:
- Bypasses the OOM-safety design.
try_allocate_private_page's doc says a guest touching a page nearMAX_PRIVATE_INPUT_SIZEshould "fail cleanly withMemoryError::AllocationFailedinstead of aborting the host process." A guest-triggeredSBinto the private region (and unalignedstore_word/half/doubleword, which dispatch tostore_byte) still aborts the host on allocation failure. Box::new([0u8; 256 KiB])constructs the array on the stack before boxing in unoptimized builds — a 256 KiB stack allocation footgun thattry_allocate_private_page'sVec-based path deliberately avoids.
store_byte can't return a Result, but it can still allocate through the same heap path (e.g. a helper that builds the page via Vec and unwraps / falls back), rather than a stack-array + infallible-abort. At minimum, avoid the stack-array construction here.
| /// themselves. Chosen to match the prover's `DEFAULT_PAGE_SIZE` concept | ||
| /// (`prover/src/tables/page.rs`); redeclared locally since the executor must | ||
| /// not depend on the prover crate. | ||
| const PRIVATE_INPUT_PAGE_SIZE: usize = 256 * 1024; |
There was a problem hiding this comment.
Every aligned fast-path here relies on an unstated invariant: that PRIVATE_INPUT_START_INDEX and MAX_PRIVATE_INPUT_SIZE are both multiples of PRIVATE_INPUT_PAGE_SIZE (the comments in load_word/load_doubleword say "either fully inside one page or fully outside"). They currently are (0xFF000000 / 256 KiB = 16320, 512 MiB / 256 KiB = 2048), so there's no bug today — but nothing guards it. If someone later moves PRIVATE_INPUT_START_INDEX to a non-page-aligned address, a 4-aligned word could straddle a page boundary and the p[offset..offset + 4] slices would panic with an out-of-bounds range (or read wrong bytes), silently.
Consider pinning the invariant at compile time next to the constant:
const _: () = assert!(PRIVATE_INPUT_START_INDEX % PRIVATE_INPUT_PAGE_SIZE as u64 == 0);
const _: () = assert!(MAX_PRIVATE_INPUT_SIZE % PRIVATE_INPUT_PAGE_SIZE as u64 == 0);
| let mut buf: Vec<u8> = Vec::new(); | ||
| buf.try_reserve_exact(PRIVATE_INPUT_PAGE_SIZE) | ||
| .map_err(|_| MemoryError::AllocationFailed)?; | ||
| buf.resize(PRIVATE_INPUT_PAGE_SIZE, 0); |
There was a problem hiding this comment.
Semantic note on iter_bytes: for the private region this now yields every byte of each allocated page (262 144 addresses per touched page, including never-written words as zeros), whereas the old per-word cells design yielded only the 4-byte words that were actually written. So a single private write now surfaces a full 256 KiB run of (addr, 0) entries in the snapshot.
Reads still return 0 for those addresses either way, and iter_bytes appears to be used only in tests/benches (not the production proving path, which builds from touched_memory_cells), so this is low-impact — but it does change epoch-snapshot content/size and is worth calling out given the unexplained ~1.1% continuation divergence mentioned in the description. Worth double-checking the affected tests actually tolerate the extra zero entries rather than passing by coincidence.
|
Benchmark Results for unmodified programs 🚀
|
|
Review summary Reviewed the paged private-input backing store in Findings (all Medium/Low — no blockers)
Note: the working directory is on The unresolved ~1.1% continuation-proving divergence is acknowledged as a documented follow-up that reproduces on all prototyped backends, so it is not attributable to this diff specifically. |
AI ReviewPR #852 · 13 changed files Findings
Status column reflects the verdict from the verifier: deepseek-verifier (openrouter/deepseek/deepseek-v4-pro). AI-003: MultiProofView::get lacks bounds checking
Claim MultiProofView::get(i) panics if i >= len() with no safe alternative Evidence The method directly indexes p.proofs[i] / p.proofs.as_slice()[i] without bounds check. While current call sites (verify_l2g_commitment_binding_view, verify_epoch) guard with length checks, this is a footgun for future callers. Suggested fix Add a get_checked(&self, i: usize) -> Option<StarkProofView<'a, F, E, PI>> method that returns None on OOB, or make get() return Option and update call sites. AI-007: Memory::clone now deep-copies 256KB pages, slowing run_epochs-based test snapshots
Claim Memory derives Clone (line 144: #[derive(Default, Debug, Clone)]), and its pages field is Djb2HashMap<Box<[u8; MEMORY_PAGE_SIZE]>> with MEMORY_PAGE_SIZE=256KB. A derived clone deep-copies every boxed page's 262144 bytes. run_epochs (executor/src/vm/execution.rs:176-189) calls self.memory.clone() at every epoch boundary to snapshot end_memory into EpochExecution. With the old per-word cells (U64HashMap<[u8;4]>) a clone copied small 4-byte entries; now each epoch boundary clones all allocated 256KB pages. For tests using run_epochs with many epochs and several touched pages (e.g. all_loadstore_32 split into 8-cycle epochs across ~34 cycles, or larger programs), this is O(epochs * pages * 256KB) in time and memory. The production continuation path (prove_continuation) is unaffected: it uses resume_with_limit + a separate PagedMem image, never cloning Memory. Evidence Line 154: pages: Djb2HashMap<Box<[u8; MEMORY_PAGE_SIZE]>>. Line 114: const MEMORY_PAGE_SIZE: usize = 256 * 1024. execution.rs:186: end_memory: self.memory.clone(). run_epochs is only called from tests (grep shows no production callers), so the regression is test-only but real for multi-epoch test fixtures. Suggested fix If run_epochs snapshotting remains needed for tests, consider sharing page ownership via Rc<[u8; MEMORY_PAGE_SIZE]> with copy-on-write (Rc::make_mut) in get_or_insert_page, so a clone only bumps refcounts and a subsequent write copies only the one mutated page. Alternatively, have run_epochs produce a cheaper snapshot (e.g. page-base set + borrowed reference) instead of a full deep clone. AI-008: iter_bytes now yields all allocated-page bytes (incl. unwritten zeros), breaking bench footprint/density metrics
Claim Memory::iter_bytes now yields ALL bytes of ALL allocated pages (including unwritten zeros within an allocated page), instead of only written bytes as before. The doc comment acknowledges this ('Each allocated page expands into its byte addresses'), but it changes the semantics: a single byte write to a fresh 256KB page now causes iter_bytes to yield 262144 (addr, 0) pairs instead of 1 written byte. This breaks bench_continuation.rs's 'footprint: {total} touched bytes' and 'density' metrics (lines 77-107), which now report allocated-page-bytes (256KB per touched page) rather than actual touched bytes, making the diagnostic output misleading. It also inflates HashMap<u64,u8> images collected from end_memory.iter_bytes() in test paths (trace_builder_tests.rs:902,1066; prove_elfs_tests.rs:3044,3283) by orders of magnitude, though build_init_page_data treats the extra zeros idempotently so traces are unaffected. The production continuation path is unaffected (uses PagedMem, not iter_bytes). Evidence Lines 199-205: the new iter_bytes does Suggested fix Either (a) restore the old 'written bytes only' semantics by tracking a per-page dirty bitmap / written-set, or (b) update bench_continuation.rs's footprint mode to compute the metric from page count * page_size (acknowledging the new semantics) rather than counting iter_bytes output as 'touched bytes'. If the full-page iteration is acceptable for test paths, at minimum fix the bench's misleading 'touched bytes' label/metric. Reviewer Lanes
Verification Lanes
Native Codex and Claude reviews run separately and post their own comments. They are not included in this structured provenance report. Discarded candidates (8) — rejected by the verifier
Raw lane outputs, candidates, final issues, and model metrics are uploaded as workflow artifacts. |
…ate input store_byte used infallible allocation while every other private-input write path was fallible, and store_private_inputs could write 4 bytes past MAX_PRIVATE_INPUT_SIZE (the length prefix wasn't accounted for in the accepted payload size), corrupting the tail of a maximal-size input on readback. Rather than patch the private-input-only paged HashMap, drop the special case entirely: back the whole address space with one HashMap<page_idx, Box<page>>, so private input is just memory written at a fixed address like anything else. This removes the page/region alignment invariant the split design depended on, and every write (private or not) now goes through the same fallible page allocator.
…e input for continuation verify (#844) * feat(recursion): supply DECODE/global-memory-genesis roots via private input for continuation verify Mirrors #782's monolithic mechanism for the continuation path: a caller (the recursion guest) can supply the DECODE preprocessed root and each touched data page's genesis root instead of the verifier recomputing them from the ELF, skipping the in-VM FFT + Merkle build. Supplied roots are used verbatim; the binding shifts to the consumer's recompute-and-compare of the folded identity, exactly like the monolithic path. verify_global's genesis-page classification (ELF-backed vs zero-init) is derived from ELF segment address ranges instead of materializing a full byte-level image when roots are supplied, since the real bytes are never read once a root covers that page. Page lookups are keyed by page number (page_base >> log2(page_size)) rather than the raw page-aligned address, since the low bits are always zero. verify_continuation keeps its existing signature (trustless recompute); verify_continuation_with_roots is the new supplied-roots entry point, and continuation_precomputed_commitments lets a caller derive the roots to supply for a given bundle. * fix(recursion): reject spurious rejects/collisions and cover the supplied-roots page path verify_global now hard-rejects a missing page_genesis_commitments entry instead of relying on a debug_assert!, and validates alignment of the caller-supplied bases the same way touched_page_bases already is. Corrects two comments that claimed genesis "cannot be prover-chosen" without qualifying the supplied-roots exception. Drops the page-number rekeying (PAGE_SIZE_LOG2/page_number) added for a HashMap that never needed it under std's SipHash; keys by raw page_base like the monolithic path instead. Adds a data_page_touch asm fixture (a real ELF .data page, unlike the stack-only fixtures used elsewhere) so the supplied-roots test actually exercises the ELF-data-page branch, with tamper/all-zero rejection cases, plus a lock test asserting classify-only and byte-level page classification agree. * fix(recursion): bound num_private_input_pages in continuation_precomputed_commitments Mirrors the check verify_continuation_with_roots already applies: bundle is untrusted (rkyv-deserialized), and num_private_input_pages feeds a page_size multiplication downstream in is_private_input_page.
* perf(prover): verify continuation proofs in place via rkyv Adapts continuation verification to the same in-place rkyv pattern verify_recursion_blob already uses for the monolithic path (#769): verify_epoch/verify_global take a StarkProofView slice (owned or archived) instead of an owned MultiProof/EpochProof, so the new guest entry point (verify_continuation_and_attest_blob, via verify_continuation_archived) reads every per-epoch/global STARK proof straight out of the archive. Only small per-epoch metadata (table counts, reg_fini, l2g_root, public output) is materialized - the (large) per-epoch/global proof data is never copied into an owned MultiProof just to verify it. Adds the continuation guest's wire format (ContinuationGuestInput, encode_continuation_guest_input) mirroring GuestInput's magic-prefixed rkyv layout, and verify_continuation_and_attest[_blob] mirroring verify_and_attest_blob's program_id fold. replay_transcript_phase_a/compute_expected_commit_bus_balance (owned) lose their last production caller to this refactor; deleted rather than kept as compatibility wrappers now that every caller (production and test) goes through the _view variants already introduced by #769. * test(prover): cover continuation blob tamper path, drop dead API Adds a negative test for the archived continuation path: encode a bundle with a tampered epoch l2g_root, assert the blob verify rejects it. Drops verify_continuation_and_attest_blob's now-unused owned-bundle predecessor and renames the surviving function to verify_continuation_and_attest, since the _blob suffix only existed to disambiguate it from that owned variant. Also fixes a stale intra-doc link left over from the deleted compute_expected_commit_bus_balance. * fmt * refactor(prover,stark): replace MultiProof field-explosion with borrowed views verify_epoch/verify_global/verify_proof_parts had ballooned into long parameter lists (proof_views, table_counts, runtime_page_ranges, reg_fini, l2g_root, public_output, ...) built ad hoc at every call site via `.iter().map(StarkProofView::Owned/Archived).collect()`, a leftover from keeping owned and archived verify paths side by side. Adds MultiProofView (crypto/stark) mirroring StarkProofView, and EpochProofView/ContinuationProofView (prover) mirroring it one level up, so callers pass one view instead of exploding a bundle into loose fields. multi_verify_views, compute_expected_commit_bus_balance_view, and replay_transcript_phase_a_view are now generic over a ProofViewSource trait (impl'd for slices, Vecs, and MultiProofView) and iterate in place - no Vec materialization anywhere in the path. Collapses verify_continuation_with_roots/verify_continuation_archived (two ~130-line near-duplicates) into one verify_continuation_view, and unifies verify_l2g_commitment_binding/_views into one view-based fn. * perf(stark): match Owned/Archived once per MultiProofView iteration, not per element MultiProofView::iter() built each StarkProofView via get(i): a match on Owned/Archived plus a bounds-checked index, redone for every element on every one of multi_verify_views' several passes over the same proof set, even though a given MultiProofView is homogeneously one variant for its whole lifetime. MultiProofViewIter now matches once, in iter() itself, and then drives a plain slice::Iter for whichever representation was chosen - no per-step bounds check, no re-matching against the source enum. * Revert "perf(stark): match Owned/Archived once per MultiProofView iteration, not per element" This reverts commit f0cd8abb8d5b05e656072d462efae5f80e92c9a4. * perf(stark): force-inline MultiProofView's per-element accessors Closes the ~0.04% guest-cycle regression the MultiProofView refactor introduced (measured via scripts/bench_recursion_scaling.sh, blowup4/txs=4): len/get/last/iter and the ProofViewSource impls' view_len/view_iter cross a crate boundary (stark -> prover) and apparently weren't getting inlined into multi_verify_views' hot loops. An alternative fix - matching Owned/Archived once per MultiProofView instead of once per element, via a hand-rolled slice::Iter-backed enum iterator - was tried and measured WORSE (+2.3% cycles): it defeats LLVM's specialized Range<usize>::map optimization the closure-based iter() benefits from (see the previous two commits). #[inline(always)] alone, keeping the closure-based iter(), measured at parity with (very slightly better than) the pre-refactor baseline. * fix(prover): cover L2G binding rejection, dedupe ELF re-parse in continuation verify - Add tests that splice a different run's global proof onto valid epochs (same shape, different L2G data) so verify_l2g_commitment_binding_view's own reject branch actually executes, both for the owned and archived verify paths. The existing tampered-root tests were caught earlier by verify_epoch's per-epoch root check and never reached this binding. - Thread entry_point out of verify_continuation_archived so verify_continuation_and_attest can fold program_id via program_id_from_digest instead of re-parsing the ELF with program_id_from_elf. - Extract access_recursion_archive, sharing the aligned-fallback + rkyv::access boilerplate between verify_recursion_blob and verify_continuation_and_attest. * fmt
Several separate, related issues: - Using page index instead of address left many leading 0s - SwissTables use leading bits for a tag that speeds up matching, collisions there force more linear probing - Even without page index, bigger page size caused slowdown due to SwissTables - SipHash is expensive for this input - Use DJB2, a cheap hash consisting of a shift, an addition and an XOR - Similar to SipHash in collision resistance for this simple and reduced input, much faster if ever executed in the guest, yet free of the leading 0s issue - Page index reverted due to lack of real value, expected an improvement that ended up as regression - Page size incremented to match prover, better memory/alloc overhead per entry
|
/ai-review |
|
/bench |
|
/verify_bench |
|
/profile-recursion |
Codex Code Review
|
Benchmark — ethrex 20 transfers (median of 3)Table parallelism: auto (cores / 3)
Commit: 859b371 · Baseline: cached · Runner: self-hosted bench |
Review — paged
|
1 participant
Summary
MAX_PRIVATE_INPUT_SIZEis 512 MiB (#843), butMemory::cellsstores every guest word — program/stack/heap and private input — in oneHashMap<u64, [u8;4]>. A max-size private input costs ~134M entries there.Measured on this branch vs. the current per-word design, for a single
store_private_inputscall at exactlyMAX_PRIVATE_INPUT_SIZE:store_private_inputstime: ~2.2s → ~28msThis PR routes the private-input address range (
[PRIVATE_INPUT_START_INDEX, PRIVATE_INPUT_START_INDEX + MAX_PRIVATE_INPUT_SIZE)) to a separateHashMap<u64, Box<[u8; 256 KiB]>>, one boxed page allocated lazily per touched page — everything else (program/stack/heap) still goes through the originalcellsmap, unchanged. Only an 8-byte pointer lives in the page map's table slot, so growing/rehashing that table only ever moves pointers, never 256 KiB page payloads (this was compared against a sibling design storing the page array inline, which pays a real resize-memcpy cost as the table grows — the boxed version avoids that).How this was chosen
Five backing designs were prototyped and benchmarked (dense
Vec, dense-eager, pagedVec, paged inline-array, pagedBox) against both a synthetic max-size input and a real 4-tx ethrex block proven via the continuation system. This one had the best combination of RAM/time and no resize-cost risk. Two independent adversarial reviews (of this diff and the dense-Vecsibling) found no semantic divergence from the original per-word design for any load/store access pattern, at any width, including boundary/alignment edge cases — confirmed further by a raw-execution cycle-count test showing 0.0017% difference against the unmodified backend on the same real ethrex-block input.A separate ~1.1% divergence observed in full continuation-proving cycle/proof-size (not raw execution) between the old and new backends is still not root-caused — it reproduces on every prototyped backend (including a plain dense
Vecwith none of this diff's paging), is unaffected by disabling the STARK grinding parallelism, and isn't explained by anything in the accounting path (which is built from the ELF + raw private-input slice + execution logs, never fromMemory's internals). It's filed as a follow-up investigation, not a blocker for this change.Test plan
cargo build --workspacecargo test -p executor(all pass)cargo test -p lambda-vm-prover --lib(515 passed, 0 failed, 20 ignored)