perf: base-field constraint evaluation for F×E accumulation#510
perf: base-field constraint evaluation for F×E accumulation#510diegokingston wants to merge 2 commits into
Conversation
Add evaluate_prover method to TransitionConstraint trait and split the evaluator's accumulation loop into F×E (base) and E×E (extension) parts. Base-field constraints (all table-specific ones) now write to a separate base-field buffer, enabling cheaper F×E multiplication (3 base muls) instead of E×E multiplication (6 base muls) during accumulation. LogUp constraints remain in the extension field via the default implementation. Changes: - TransitionConstraint::evaluate_prover with default delegating to evaluate - AIR::num_base_transition_constraints + compute_transition_prover - AirWithBuses stores and reports num_base_constraints - Evaluator uses split accumulation (base_buf + transition_buf) - All 14 table constraint structs override evaluate_prover
|
/bench 3 |
Codex Code Review
No other concrete security/bug/performance issues stood out in this diff. |
Benchmark — fib_iterative_8M (median of 3)Table parallelism: 32 (auto = cores / 3)
Commit: 85845a3 · Baseline: cached · Runner: self-hosted bench |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| for e in base_evaluations.iter_mut() { | ||
| *e = FieldElement::zero(); | ||
| } | ||
| for e in ext_evaluations[num_base..].iter_mut() { |
There was a problem hiding this comment.
[High] Silent correctness gap: ext_evaluations[0..num_base] is never zeroed
The docstring says ext_evaluations has length num_transition_constraints(), so slots 0..num_base exist in the buffer but are skipped by the zeroing loop here. The accumulator in evaluator.rs only reads transition_buf[num_base..], so there's no bug today — but this relies on an implicit invariant that base-field constraints only ever write to base_evaluations, never to ext_evaluations[0..num_base].
If a constraint's evaluate_prover override is ever mis-implemented (or the default fallback is called unexpectedly) and writes into ext_evaluations[c_idx] with c_idx < num_base, stale data from a previous iteration would be accumulated silently, producing an incorrect proof with no panic or error.
Consider either zeroing the full ext_evaluations slice here, or adding a debug_assert in the accumulator that transition_buf[0..num_base] are all zero after each compute_transition_prover call.
| mut transition_constraints: Vec<Box<dyn TransitionConstraint<F, E>>>, | ||
| ) -> Self { | ||
| let num_interactions = auxiliary_trace_build_data.interactions.len(); | ||
| let num_base_constraints = transition_constraints.len(); |
There was a problem hiding this comment.
[Medium] Ordering invariant is implicit and unguarded
num_base_constraints is captured here, before LogUp constraints are appended. The correctness of the whole F×E optimization depends on the invariant that all base constraints occupy indices 0..num_base and all LogUp constraints occupy num_base..total. This ordering is never enforced.
If a refactor ever inserts LogUp constraints before the table-specific ones (e.g. a prepend instead of an append), num_base_constraints would silently count LogUp constraints as base-field and the F×E path would produce wrong evaluations.
Consider adding a debug assertion after LogUp constraints are pushed — verifying that every constraint in 0..num_base is not a LogUp type — or at minimum a comment cross-referencing where the append happens.
| .zip(transition_coefficients) | ||
| .fold(FieldElement::zero(), |acc, (eval, beta)| acc + eval * beta); | ||
| z * &sum | ||
| let acc_transition = if num_base > 0 { |
There was a problem hiding this comment.
[Medium] ~90 lines of logic duplicated between parallel and non-parallel paths
The num_base > 0 branch (both is_uniform and non-uniform sub-cases) is copy-pasted nearly verbatim in the #[cfg(not(feature = "parallel"))] path below. This PR doubled the duplication surface compared to the original. Any future correctness fix will need to be applied in two places.
Consider extracting the inner accumulation into a shared free function, e.g.:
fn accumulate_transition<F, E>(
base_buf: &[FieldElement<F>],
transition_buf: &[FieldElement<E>],
num_base: usize,
transition_coefficients: &[FieldElement<E>],
zerofier_data: &ZerofierData,
i: usize,
is_uniform: bool,
) -> FieldElement<E> { ... }Both map_init and the sequential loop can then call it with no duplication.
| TransitionEvaluationContext::Prover { frame, .. } => { | ||
| base_evaluations[self.constraint_idx] = self.compute(frame.get_evaluation_step(0)); | ||
| } | ||
| _ => unreachable!("evaluate_prover called in verifier context"), |
There was a problem hiding this comment.
[Low] unreachable! is a runtime panic, not a compile-time guarantee
This pattern is copy-pasted across all 12+ evaluate_prover overrides. It works because evaluate_prover is only called from compute_transition_prover (prover hot loop), but the TransitionConstraint trait is public — any future caller passing a Verifier context would panic at runtime.
A safer alternative is to change the signature so evaluate_prover takes &ProverFrame directly instead of &TransitionEvaluationContext, making the verifier path a compile-time error and eliminating the boilerplate match entirely across all 12 impls.
Review: perf: base-field constraint evaluation for F×E accumulationThe optimization is mathematically sound and the -7.4% proving time improvement is well-motivated. The core idea — splitting base-field and extension-field evaluations to use cheaper F×E multiplications — is correct. A few issues worth addressing before merge: High
Medium
Low
|
Replace N separate Box<dyn TransitionConstraint>::evaluate_prover() vtable calls per LDE point with ONE monolithic function that evaluates ALL base-field constraints at once. The compiler can now: - Inline all column reads and arithmetic across constraints - Share column reads between constraints (CSE) - Keep field elements in CPU registers across constraints - Optimize instruction scheduling across constraint boundaries For CPU table: 66 individual vtable calls → 1 closure call. Expected: ~10-15% faster constraint evaluation. The monolithic path is additive — per-constraint fallback still works for test AIRs and LogUp extension-field constraints.
|
/bench 3 |
1 participant
Summary
Split the prover's constraint evaluation accumulation into base-field (F×E, 3 base muls) and extension-field (E×E, 6 base muls) paths. Table-specific constraints (IS_BIT, ADD, SHIFT, etc.) now write
FieldElement<F>to a base-field buffer, while LogUp constraints stay in the extension field.Based on the approach from PR #418 (which measured -7.4% proving time). Minimal changes to the existing architecture — no new traits, no AirBuilder, no closures.
Changes
TransitionConstraint::evaluate_prover()— new method, writes base-field result for main trace constraintsAIR::num_base_transition_constraints()— tells evaluator how many constraints are base-fieldAIR::compute_transition_prover()— split evaluation into base/ext buffersevaluate_proverTest plan
-D warnings/bench