Cache LDE + Scaling improvements#494
Conversation
Tables with effective width < 42 were sized at 2^21, producing single large chunks. Capping at 2^20 produces 2x more chunks of the standard size, improving parallel throughput and keeping peak memory per chunk uniform across all tables.
Tables sharing the same lde_size now reuse a single Arc<LdeTwiddles> instead of each allocating their own copy. This avoids redundant twiddle computation and allocation when multiple tables happen to have the same trace length (e.g., all 2^19-row tables after the cap change).
Under the `parallel` feature, fold each conjugate pair concurrently using `par_chunks(2).zip(inv_twiddles.par_iter())`, collecting into a new Vec of half length. The sequential path (no `parallel` feature) is unchanged. This avoids the index aliasing issue from an in-place interleaved write.
Replace the per-row `Vec` allocation inside the iterator with a thread-local buffer via Rayon's `map_init` (parallel path) or a single buffer allocated outside the loop (sequential path). This avoids one heap allocation per row during Merkle tree construction.
|
/bench |
Codex Code Review
No other security/correctness/performance issues stood out in this PR diff. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| let lde_size = domain.interpolation_domain_size * domain.blowup_factor; | ||
| let twiddles = twiddle_by_size | ||
| .entry(lde_size) | ||
| .or_insert_with(|| Arc::new(LdeTwiddles::new(&domain))); |
There was a problem hiding this comment.
Medium – Incorrect cache key if AIRs have different blowup_factor or coset_offset
LdeTwiddles::new bakes three domain-specific quantities into the struct:
invtwiddles — sized byinterpolation_domain_sizefwdtwiddles — sized bylde_sizecoset_weights— sized byinterpolation_domain_size, scaled bycoset_offset
Using only lde_size as the cache key is safe only when all AIRs share the same blowup_factor (so equal lde_size ⟹ equal interpolation_domain_size) and the same coset_offset. The comment acknowledges this but it isn't enforced. If violated, twiddles from one domain are silently reused for another, producing cryptographically wrong (but non-panicking) proofs.
Consider asserting the invariant at the cache hit, e.g.:
// or store (interpolation_domain_size, coset_offset) as key
debug_assert_eq!(
domain.interpolation_domain_size,
first_domain_with_this_lde_size.interpolation_domain_size
);Or use (lde_size, coset_offset_bits) as the key to make the assumption self-enforcing.
| let folded: Vec<FieldElement<E>> = evals | ||
| .par_chunks(2) | ||
| .zip(inv_twiddles.par_iter()) |
There was a problem hiding this comment.
Low – Parallel and sequential branches diverge on contract violation
If inv_twiddles.len() < evals.len() / 2 (broken contract):
- parallel:
zipsilently truncates to the shorter iterator →foldedis shorter thanevals.len() / 2, and*evals = foldedquietly installs a wrong-length vector. Downstream code proceeds with corrupted state. - sequential:
evals[j]withjreachinghalf - 1will index out ofinv_twiddles→ panic.
In practice the contract is maintained by callers, but if it breaks the parallel path gives no diagnostic while the sequential path at least terminates loudly. A debug_assert_eq!(inv_twiddles.len(), evals.len() / 2) at the top of the function would catch this in both builds without runtime cost in release.
Review: Cache LDE + Scaling improvementsOverall the changes are well-structured. Two issues worth addressing, plus a note on the table-size reductions. Medium – Twiddle cache key is under-specifiedSee inline comment on
Suggested fix: assert the invariant at cache-hit time or use Low – Parallel/sequential diverge on
|
Benchmark — fib_iterative_8M (median of 3)Table parallelism: 32 (auto = cores / 3)
Commit: ec49bf6 · Baseline: built from main · Runner: self-hosted bench |
|
Not a great difference |
2 participants
Merge of #492 with the first 5 commits of #483. Just to check bench numbers.