WHIR switch to Plonky3 #858
Conversation
|
Current benchmarking result:
|
| { | ||
| match digest { | ||
| Digest::Base(digest) => { | ||
| digest |
There was a problem hiding this comment.
Is it possible to avoid the clone by implementing Iterator on digest? I tried but it wasn't as easy as I thought. Perhaps you can find a better way?
There was a problem hiding this comment.
Discussed with Yuncong on this similar issue and unfortunately we dont have easier way to avoid this after introducing field-agnostic generalization. But clone on digest is ok and the prover cost is negligible for now.
| let mut res = Vec::with_capacity(folding_factor); | ||
|
|
||
| for _ in 0..folding_factor { | ||
| let sumcheck_poly = self.sumcheck_prover.compute_sumcheck_polynomial(); | ||
| merlin.add_scalars(sumcheck_poly.evaluations())?; | ||
| let [folding_randomness]: [F; 1] = merlin.challenge_scalars()?; | ||
| sumcheck_polys.push(sumcheck_poly.evaluations().to_vec()); |
There was a problem hiding this comment.
How about:
let sumcheck_poly = self.sumcheck_prover.compute_sumcheck_polynomial();
transcript.append_field_element_exts(sumcheck_poly.evaluations());
let folding_randomness = transcript
.sample_and_append_challenge(b"folding_randomness")
.elements;
res.push(folding_randomness);
self.sumcheck_prover
.compress(E::ONE, &[folding_randomness], &sumcheck_poly);
sumcheck_polys.push(sumcheck_poly.evaluations);Need to make sumcheck_poly.evaluations public but avoids a to_vec().
There was a problem hiding this comment.
Makes sense. But the save is very small, because sumcheck_poly is just a three-coefficient polynomial.
There was a problem hiding this comment.
You can submit a PR to this branch.
| @@ -103,80 +67,104 @@ where | |||
|
|
|||
| #[cfg(test)] | |||
| mod tests { | |||
There was a problem hiding this comment.
Do these tests all pass? I seem to encounter a problem with batch_commit
There was a problem hiding this comment.
They all pass on my laptop. Can you share the error message?
| let mut res = Vec::with_capacity(folding_factor); | ||
|
|
||
| for _ in 0..folding_factor { | ||
| let sumcheck_poly = self.sumcheck_prover.compute_sumcheck_polynomial(); | ||
| merlin.add_scalars(sumcheck_poly.evaluations())?; | ||
| let [folding_randomness]: [F; 1] = merlin.challenge_scalars()?; | ||
| sumcheck_polys.push(sumcheck_poly.evaluations().to_vec()); |
There was a problem hiding this comment.
Move to bottom to avoid to_vec(). Same as above,
| let n = p3::util::log2_strict_usize(evals.height()); | ||
|
|
||
| evals.par_row_chunks_mut(2).for_each(|mut chunk| { | ||
| let to_subtract = chunk.row(0).collect::<Vec<_>>(); |
There was a problem hiding this comment.
Would chunk by 1 and then iterate by 2 prevent the copy of row 0?
There was a problem hiding this comment.
The compiler complains I can't have both mutable and immutable reference to the chunk when I try to subtract the second row from the first row by only referencing into them without copying.
Can you provide the code for your suggestion?
There was a problem hiding this comment.
Awesome work with huge clean up! 👍
Some sub-crate within whir didnt review in detail: ntt/sumcheck as I feel they should be able to combine with other existing functionality in Ceno.
Overall LGTM! except other review's comments regarding to minor optimisation, I think it's good to go and have enhancement in subsequent PR.
Compilation and tests passed.
Current benchmark results: