improve: apply tachyon optimizations(1) #342
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #342 +/- ##
==========================================
- Coverage 81.90% 81.89% -0.01%
==========================================
Files 82 82
Lines 17017 17019 +2
==========================================
Hits 13937 13937
- Misses 3080 3082 +2 ☔ View full report in Codecov by Sentry. |
| @@ -447,12 +454,13 @@ impl<C: CurveAffine> Evaluator<C> { | |||
| // - z_i(X) \prod_j (p(X) + \delta^j \beta X + \gamma) | |||
| // ) | |||
| let mut current_delta = delta_start * beta_term; | |||
| for ((set, columns), cosets) in sets | |||
| for (((set_idx, _), columns), cosets) in sets | |||
There was a problem hiding this comment.
How about iterating over permutation_product_cosets so that we don't need to manually index at each iteration? Something like this:
let mut current_delta = delta_start * beta_term;
for ((permutation_product_coset, columns), cosets) in permutation_product_cosets
.iter()
.zip(p.columns.chunks(chunk_len))
.zip(pk.permutation.cosets.chunks(chunk_len))
{
let mut left = permutation_product_coset[r_next];I think the code looks slightly more clear this way.
| @@ -25,7 +25,6 @@ use halo2_middleware::poly::Rotation; | |||
|
|
|||
| pub(crate) struct CommittedSet<C: CurveAffine> { | |||
| pub(crate) permutation_product_poly: Polynomial<C::Scalar, Coeff>, | |||
| pub(crate) permutation_product_coset: Polynomial<C::Scalar, ExtendedLagrangeCoeff>, | |||
There was a problem hiding this comment.
After removing this, how is CommittedSet different from ConstructedSet?
This part of the code is quite poorly documented so it's a bit hard to follow the function of each struct...
There was a problem hiding this comment.
The function construct() for Committed may be simplified now to a simple clone() (not sure, some types may need to be fixed).
In fact, after this change I think we should re-evaluate the need of Commited and Constructed, as these may be redundant.
There was a problem hiding this comment.
How about this update? 4628c24
I am sure that we don't need Constructed struct any more.
It also removes unnecessary conversion, which needs .clone().
| pub(crate) struct Evaluated<C: CurveAffine> { | ||
| constructed: Constructed<C>, | ||
| constructed: Committed<C>, |
There was a problem hiding this comment.
| constructed: Committed<C>, | |
| commited: Committed<C>, |
There was a problem hiding this comment.
Thanks for suggestion!
Here, I believe it is better to use constructed.
One reason is that it is also the constructed in lookup::prover::Evaluated & shuffle::prover::Evaluated.
Description
Analyze and apply the tachyon optimizations - 1(Lazy IFFT)
Related issues
Changes
permutation_product_cosetfrom(permutation::)Committed(permutation::)Constructed& useCommitteddirectly