Skip to content

Feed bitwise collectors straight into the histogram; cut reduce allocations#793

Merged
MauroToscano merged 4 commits into
review/pr786-u32-domainfrom
review/pr786-histogram-direct
Jul 7, 2026
Merged

Feed bitwise collectors straight into the histogram; cut reduce allocations#793
MauroToscano merged 4 commits into
review/pr786-u32-domainfrom
review/pr786-histogram-direct

Conversation

@MauroToscano

@MauroToscano MauroToscano commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Final follow-up from the #786 review, stacked on #792. Applies the PR's own "never materialize a Vec whose only consumer is the multiplicity count" principle to the remaining big sources, and removes the rayon reduce-identity allocations. Multiplicities are unchanged (same counts, same cells).

Changes

  • Collectors now bump a BitwiseHistogram directly (Fn(&mut BitwiseHistogram)) instead of returning Vec<BitwiseOperation>:
    • MEMW_R bumps one IS_HALFWORD per register row — previously an ~4 B/op vector over tens of millions of rows existed just to be counted.
    • PAGE bumps one ARE_BYTES per byte of every touched page — previously a full op vector per page sweep.
    • CPU padding collapses to two bump_n calls (3n ARE_BYTES[0,0] + 4n IS_HALF[0,0]) instead of an O(padding_rows) vector of identical zero ops (which also over-reserved 2×).
    • Small sources still use their pure collect_* functions; the closure folds the transient Vec in and drops it.
  • reduce_with replaces reduce(identity, ..) in the rayon tree-reduce: no more zeroed 80 MiB identity histogram per reduce leaf (up to ~16 × 80 MiB of pure alloc_zeroed + merge traffic on many-core machines).
  • RegRow.address shrinks to u16 (register index 0..=255): the walk's largest persisted array goes from 40 to 32 B/row.

Measurements

Local tests

cargo build --release --features instruments, cli prove --time, Apple Silicon, best of 2 runs, this branch vs its base (#792):

workload p4_bitwise_collect before after Δ
fib_iterative_1M 112.0 ms 97.8 ms −13%
fib_iterative_4M 131.1 ms 83.3 ms −36%

prove_total and peak RSS unchanged within run-to-run noise on this machine (p4 is <1% of total here, and macOS RSS jitters ~1.5 GB between runs). The reduce-identity savings scale with core count, so the effect should be larger on the many-core provers #786 targets. The remaining serial base.add_ops over the in-walk lookups was left as-is: parallelizing it adds code for a phase already down to ~85 ms.

Server

image

Testing

  • Full cargo test -p lambda-vm-prover --lib on this change: 503 passed (same 5 known missing-fixture failures, which fail at file-read).
  • make lint passes; clippy --all-targets no warnings; fmt clean.

…ations

- Collectors now take &mut BitwiseHistogram instead of returning
  Vec<BitwiseOperation>. The heavy sources count with no per-source
  vector at all: MEMW_R bumps one IS_HALFWORD per row (tens of
  millions of rows), PAGE bumps one ARE_BYTES per byte of every
  touched page, and CPU padding collapses to two bump_n calls
  instead of an O(padding_rows) vector of identical zero ops.
- reduce_with replaces reduce(identity, ..) in the rayon tree-reduce,
  eliminating one zeroed 80 MiB identity histogram per reduce leaf.
- RegRow.address shrinks to u16 (register index 0..=255), taking the
  largest persisted array of the walk from 40 to 32 bytes per row.
@MauroToscano MauroToscano force-pushed the review/pr786-u32-domain branch from 19b75b4 to 1287123 Compare July 7, 2026 14:44
@MauroToscano MauroToscano force-pushed the review/pr786-histogram-direct branch from 55bbe35 to 47948a2 Compare July 7, 2026 14:44
@MauroToscano

Copy link
Copy Markdown
Contributor Author

Rebased and composed with 3ba4aac: kept its par_chunks cap (peak memory no longer scales with core count), and layered this PR's two changes on top — collectors bump the chunk histogram directly (no per-source op vectors for MEMW_R / PAGE / padding), and reduce_with removes the remaining per-leaf 80 MiB identity allocations that reduce(identity, ..) still paid. Full suite re-run on the rebased stack: 502 passed (one test fewer than before — the flaky privacy byte-scan test was removed upstream in #789), same 5 known missing-fixture failures.

@MauroToscano

Copy link
Copy Markdown
Contributor Author

/bench

@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown

Benchmark — ethrex 20 transfers (median of 3)

Table parallelism: auto (cores / 3)

Metric main PR Δ
Peak heap 72964 MB 72876 MB -88 MB (-0.1%) ⚪
Prove time 57.860s 38.595s -19.265s (-33.3%) 🟢

🎉 Improvement detected — heap or time decreased by more than 5%.

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

Commit: 47948a2 · Baseline: cached · Runner: self-hosted bench

The bitwise-histogram comments were phrased against the pre-refactor code
("byte-identical to the previous serial .extend() chain", "instead of an
O(n) op vector") — narration that is meaningless once this merges and the
old path is gone. Restate them as standalone rationale: the multiplicities
are order-independent (permutation-invariant bus) so parallel collection is
sound, and the reduce_with note now explains why to prefer it over
reduce(identity) rather than referencing a removed path. Also fix the stale
add_padding_byte_checks doc (it described the pre-shrink 14-op CPU layout).
Repo-wide sweep of the trace-generation code for comments that narrate a
change the next reader can't see, or that contradict the code:

- MemwBuckets doc and collect_all_ops no longer reference "the old two-stage
  partition" / "byte-identical to partition-ing one combined vec" — that
  partition sweep no longer exists anywhere (grep '.partition(' → 0 hits).
  Restated as the current invariant (register-first-then-aligned, deterministic
  insertion order the multiplicity counts rely on).
- Fix the MEMW_R ADDRESS column doc: register index range is 0-255 (x0-x31 plus
  the x254 commit index and x255 PC), not 0-31.
The Bus-14 comment in collect_store_op_from_cpu said the packed value "must
match CPU M7 which sends full rv2 as [lo32, hi32]", but M7 (the CPU's old
inline store MEMW at timestamp+1) was removed in the ALU-bus migration. The
store value now flows CPU -> MEMORY/MEMOP (rv2 as [lo32, hi32]) -> STORE chip
-> MEMW write. Reword to describe that path; the byte-layout requirement it
gestured at is unchanged.
@MauroToscano MauroToscano merged commit abc4ab4 into review/pr786-u32-domain Jul 7, 2026
14 checks passed
@MauroToscano MauroToscano deleted the review/pr786-histogram-direct branch July 7, 2026 17:49
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.

1 participant