Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Implement and integrate rlc function for simplified lin. combination #260

Merged
merged 2 commits into from
Jan 16, 2024

Conversation

huitseeker
Copy link
Member

Summary

This is mostly implemented as a proof-of-concept to align with @adr1anh on #223. The RLC implements Horner's scheme on any iterator operand with the correct MulAssign, AddAssign traits.

Details

  • Introduced a new function rlc, which implements Horner's scheme,
  • Applied traits Borrow, MulAssign, and AddAssign to define it, which exist on UniPoly,
  • Used rlc function in evaluate method and kzg_compute_batch_polynomial,
  • Updated closures in mlkzg.rs file to directly handle vector arguments, simplifying the overall operations.
  • Streamlined the code by eliminating unnecessary assertions in kzg_verify_batch closure and redundant inner function in kzg_compute_batch_polynomial.

Next steps

  1. extend the rlc to different parts of the code base, improve ergonomics, etc ...
    • I'd be happy for pointers!
    • for instance, do we want to extend this to iterated square powers (as we do in ML polys)?
  2. implement a really parallelized variant, by generating powers using a parallel prefix scan (which then allows parallelizing the final multiplication, Horner's scheme being inherently sequential).

@huitseeker huitseeker force-pushed the linear_combinations branch 2 times, most recently from ef84cb9 to 62963fa Compare January 13, 2024 20:36
adr1anh
adr1anh previously approved these changes Jan 15, 2024
Copy link
Contributor

@adr1anh adr1anh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good! Would it make sense to implement this as an extension of Iterator where the bounds on Item are the same as rlc?

There are many places where this can be used inside of ppsnark.rs, it will be a nice addition!

storojs72
storojs72 previously approved these changes Jan 15, 2024
@huitseeker
Copy link
Member Author

huitseeker commented Jan 15, 2024

Looks really good! Would it make sense to implement this as an extension of Iterator where the bounds on Item are the same as rlc?

We could, but it's essentially a difference in call style rlc(foo, f) vs. foo.rlc(f). In each case you need one more import.
LMK which you prefer.

There are many places where this can be used inside of ppsnark.rs, it will be a nice addition!

I think ppsnark mostly gathers this under the batch auxiliary function, but happy you like it.

In any case, beware the algorithmics are a bit different: the power generation is not parallel in the original, but the multiplications are. The rlc approach is all-sequential, only the muladd operation might be when one operand is a vector. I'll run timings for this PR, but would encourage a PR converting instances of power polynomial usages to rlc to do so as well, until I land a parallel prefix scan (which is nearly ready to go).

@huitseeker
Copy link
Member Author

Extract of the MLKZG bench (which is the one affected by the present PR):

group                                                                                                                                      dev                                    linear_combs
-----                                                                                                                                      ---                                    ------------
PCS-Proving 10/arecibo::provider::mlkzg::EvaluationEngine<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineKZG>/10         1.00      9.6±0.14ms            1.00      9.6±0.37ms
PCS-Proving 12/arecibo::provider::mlkzg::EvaluationEngine<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineKZG>/12         1.01     25.2±0.29ms            1.00     24.9±0.55ms
PCS-Proving 14/arecibo::provider::mlkzg::EvaluationEngine<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineKZG>/14         1.02     74.8±1.29ms            1.00     73.1±1.10ms
PCS-Proving 16/arecibo::provider::mlkzg::EvaluationEngine<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineKZG>/16         1.02    249.0±7.45ms            1.00    243.6±2.95ms
PCS-Proving 18/arecibo::provider::mlkzg::EvaluationEngine<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineKZG>/18         1.02   926.6±32.87ms            1.00   912.7±32.46ms
PCS-Proving 20/arecibo::provider::mlkzg::EvaluationEngine<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineKZG>/20         1.00       3.3±0.08s            1.01       3.3±0.12s
PCS-Verifying 10/arecibo::provider::mlkzg::EvaluationEngine<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineKZG>/10       1.14      4.7±0.36ms            1.00      4.1±0.33ms
PCS-Verifying 12/arecibo::provider::mlkzg::EvaluationEngine<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineKZG>/12       1.00      3.6±0.67ms            1.16      4.2±0.52ms
PCS-Verifying 14/arecibo::provider::mlkzg::EvaluationEngine<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineKZG>/14       1.01      3.8±0.45ms            1.00      3.7±0.49ms
PCS-Verifying 16/arecibo::provider::mlkzg::EvaluationEngine<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineKZG>/16       1.00      5.1±0.70ms            1.01      5.1±0.44ms
PCS-Verifying 18/arecibo::provider::mlkzg::EvaluationEngine<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineKZG>/18       1.08      5.5±0.45ms            1.00      5.1±0.69ms
PCS-Verifying 20/arecibo::provider::mlkzg::EvaluationEngine<halo2curves::bn256::engine::Bn256, arecibo::provider::Bn256EngineKZG>/20       1.07      5.3±0.47ms            1.00      4.9±0.71ms

…nation semantics

- Introduced a new function `rlc`, which implements Horner's scheme,
- Applied traits `Borrow`, `MulAssign`, and `AddAssign` to define it, which exist on `UniPoly`,
- Used `rlc` function in `evaluate` method and `kzg_compute_batch_polynomial`,
- Updated closures in `mlkzg.rs` file to directly handle vector arguments, simplifying the overall operations.
- Streamlined the code by eliminating unnecessary assertions in `kzg_verify_batch` closure and redundant inner function in `kzg_compute_batch_polynomial`.
@huitseeker
Copy link
Member Author

@adr1anh @storojs72 if you're happy with the extension trait, we can just stamp, otherwise I'll remove the latest commit.

@huitseeker huitseeker added this pull request to the merge queue Jan 16, 2024
Merged via the queue into argumentcomputer:dev with commit def59e2 Jan 16, 2024
9 checks passed
@huitseeker huitseeker deleted the linear_combinations branch January 16, 2024 21:38
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.

3 participants