Skip to content

perf: build PAGE trace from dense memory store - #858

Open
ColoCarletti wants to merge 6 commits into
mainfrom
perf-page-dense-gen
Open

perf: build PAGE trace from dense memory store#858
ColoCarletti wants to merge 6 commits into
mainfrom
perf-page-dense-gen

Conversation

@ColoCarletti

Copy link
Copy Markdown
Collaborator

What

Generate the PAGE trace tables by reading each cell's final (value, timestamp) straight from the dense per-page memory store, instead of first building a sparse FinalStateMap and doing one (mostly-miss) lookup per page offset.

Why

Those per-offset sparse-map lookups were the dominant cost of PAGE generation.
Reading the dense page slice directly is one indexed read per offset, no hashing.

Impact (ethrex 5-tx block, RTX 5090)

  • PAGE table generation: 658ms → 260ms
  • Trace generation: 2.63s → 2.25s (−15%)
  • Total prove: 12.8s → 12.5s (−2.4%)

Produces the exact same PAGE trace — prove + verify pass.

Changes (3 files)

  • prover/src/paged_mem.rs: add PagedMem::page_data() (dense per-page slice).
  • prover/src/tables/page.rs: add generate_page_trace_from_dense().
  • prover/src/tables/trace_builder.rs: generate_page_tables uses the dense path.

@ColoCarletti

Copy link
Copy Markdown
Collaborator Author

/bench 5

@ColoCarletti

Copy link
Copy Markdown
Collaborator Author

/bench-gpu

@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown

GPU Benchmark (ABBA) — 44bcbf24f3 vs main (14 pairs)

RTX 5090 · Vast.ai datacenter @ $0.8423148148148147/hr · prover/cuda · drift-free A/B/B/A

=== ABBA paired result  (improvement: - = PR faster) ===
  pairs: 14   mean A (PR): 17.797s   mean B (base): 18.178s

  [parametric] paired-t   mean -2.09%   sd 1.11%   se 0.30%
               95% CI: [-2.73%, -1.45%]   (t df=13 = 2.16)
  [robust]     median -2.21%   Wilcoxon W+=0 W-=105  p(exact)=0.0001  (z=-3.26)

  --- server stability (this run; compare across servers) ---
  run-to-run jitter:    A CV 0.67%   B CV 0.84%        (lower = steadier)
  within-session drift: +0.73% over the run, 1st->2nd half +0.25%
    (jitter -> Tier-1 cached gate floor; drift -> whether the cached baseline can be trusted)

  VERDICT: REAL IMPROVEMENT - PR faster by ~2.09% (t-CI and Wilcoxon agree)

  raw pairs: /tmp/abba_run/pairs.csv

- = PR faster. Trust the verdict when paired-t and Wilcoxon agree.

@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown

Benchmark — ethrex 20 transfers (median of 3)

Table parallelism: auto (cores / 3)

Metric main PR Δ
Peak heap 73028 MB 72504 MB -524 MB (-0.7%) ⚪
Prove time 37.771s 37.531s -0.240s (-0.6%) ⚪

✅ No significant change.

🔬 Looks like a small speedup (-0.6%) — below what 3 runs can confirm. Comment /bench-abba to run the drift-free ABBA tiebreaker (paired-t CI + exact Wilcoxon). Note: it occupies the bench server for ~30–40 min.
Optional pair count: /bench-abba 32 (20 resolves ~1%, 32 for ~0.6%).

✅ Low variance (time: 1.6%, heap: 0.6%)

Commit: 44bcbf2 · Baseline: cached · Runner: self-hosted bench

@ColoCarletti

Copy link
Copy Markdown
Collaborator Author

/bench 10

@ColoCarletti
ColoCarletti force-pushed the perf-page-dense-gen branch from 086761e to 82a3d7a Compare July 24, 2026 18:49
Comment thread prover/src/tables/page.rs
Comment on lines +324 to +330
assert!(
config.page_base.is_multiple_of(page_size as u64),
"Page base must be page-aligned"
);
if let Some(page) = final_page {
debug_assert_eq!(page.len(), page_size, "dense page slice must span the page");
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These could be enforced at the type system level instead:

  1. Make page not a slice but a reference to a [(u8, u64); PAGE_SIZE] array;
  2. Make page_base refer to the right-shifted address, as pages are powers of 2.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, both are cleaner, but they need PagedMem/PageConfig changes that ripple through the whole walk. Keeping this PR focused and opening a follow-up.

Comment thread prover/src/tables/page.rs
/// `FinalStateMap`-miss branch of [`generate_page_trace`]).
pub fn generate_page_trace_from_dense(
config: &PageConfig,
final_page: Option<&[(u8, u64)]>,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is 43.75% wasted space. A pair of slices would be more efficient: (&[u8], &[u64]).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point, but that (u8, u64) is the existing PagedMem<MemoryCell> layout, not something this PR adds. Making it SoA means changing PagedMem, which the whole walk hits on every access. Better as a follow-up, and it'd help the whole walk, not just page gen.

@MauroToscano MauroToscano left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ AI-generated review. Produced by an automated agent and posted from this account — not written by hand. Findings below are leads to verify, not confirmed conclusions.

This PR makes the dense store the single PAGE path, with page_final as the sole source of FINI/TIMESTAMP and the ARE_BYTES multiplicities. The PagedMem invariants behind the binary search hold, both call sites derive bases from page_bases(), and nothing moves on the verifier/transcript/CUDA side. Two items before merge, plus nits.

prover/src/tables/page.rs:297page_final collapses every ts == 0 cell to (init, 0), equivalent to the sparse path only under the unenforced invariant that every ts == 0 cell in memory_state.cells holds exactly the init byte for its offset. It holds today (MemoryState::from_image seeds (byte, 0) from the same initial_image that build_init_page_data/PageConfig read, trace_builder.rs:104-110/4244; unset offsets read back as fill (0,0) against a zero init; every runtime write carries op.timestamp = i*4 + 4 >= 4, trace_builder.rs:347), but nothing checks it and the new test assumes it rather than probing it. Breakage is silent: per bus_interactions (page.rs:517-597) PAGE receives memory[0, addr, 0, init] and sends memory[0, addr, ts, fini], so for an untouched cell with init I, stored V, I != V, the old path emitted INIT=I/FINI=V/TS=0 — two non-cancelling tuples, a Memory-bus imbalance that failed the proof — while the dense path emits INIT=I/FINI=I/TS=0, which self-cancels. The trace stays provable while PAGE's final memory disagrees with executor state and with the image carried across a continuation chain. Not a soundness break (FINI is committed main trace, bus-constrained only), but the only signal that caught divergence is gone; debug_assert!(timestamp == 0 => value == init) inside page_final restores it and is inherited by collect_bitwise_from_page.

prover/src/tables/page.rs:237generate_page_trace, FinalStateMap (page.rs:101) and FinalByteState (page.rs:93) lose their last non-test callers here; only prover/src/tests/page_tests.rs still references them, yet all three stay pub in the public module tree (tables/mod.rs:47) and generate_page_trace's doc still reads "Generates a PAGE trace table for a single page." with no hint it is retired (the dense fn, the differential test and both trace_builder comments do say which path is live). Mark it as the reference oracle for generate_page_trace_dense_matches_sparse, or better gate it and the two types behind #[cfg(test)] — feasible since lib.rs:27-28 declares #[cfg(test)] pub mod tests;. Retargeting page_tests.rs:33/65/96 at the dense generator is polish, not a coverage gap.

Nits:

  • prover/src/tests/page_tests.rs:262 — the #[cfg(not(feature = "disk-spill"))] gate rests on a false rationale and can be dropped: table.rs:268-269 provides #[cfg(feature = "disk-spill")] impl<F: IsField> PartialEq for Table<F> (element-wise via .get(), unbounded in F), while the not(disk-spill) derive adds an implicit F: PartialEq that GoldilocksField satisfies. The second sentence is correct and should stay: TraceTable's derive (trace.rs:22) adds E: PartialEq, which Degree3GoldilocksExtensionField (extensions_goldilocks.rs:281) lacks. No CI gain — the disk-spill job is name-filtered (pr_main.yaml:233).
  • prover/src/tables/trace_builder.rs:2131 — comment says "page_final computes (init, fini)", but page_final(init, value, timestamp, exclude_touched) -> (u8, u64) takes init as input and returns (fini_value, fini_ts), with call site 2144 dropping the ts half and init computed at 2141; reword to say it derives (fini, ts) exactly as generate_page_trace_from_dense does, which is the real justification for ARE_BYTES matching PAGE's FINI column.
  • prover/src/tables/page.rs:350 — the None arm (final_page.map_or((0u8, 0u64), |p| p[offset])) is unreachable: both callers (trace_builder.rs:2637, page_tests.rs:292) pass Some, since bases come from page_bases(), exactly what page_data() searches. One comparison in the existing loop against generate_page_trace(&config, &FinalStateMap::new()) covers the (0u8, 0u64) constant and passes as written; note the fallback is duplicated at page.rs:350 and trace_builder.rs:2143.
  • prover/src/paged_mem.rs:110page_data alone does not normalize its address (get/set go through Self::split(addr); this binary-searches the raw base), so an unaligned argument can never match and returns Err(_) => None, indistinguishable from "no set cell in this page". Unreachable today, but the unguarded consumer is collect_bitwise_from_page, where a spurious None skews ARE_BYTES multiplicities and unbalances the bus — add debug_assert!(base.is_multiple_of(DEFAULT_PAGE_SIZE as u64)) rather than normalizing via Self::split(base).0, which would hide the misuse.

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