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

feat(nifs): impl protogalaxy::poly::compute_G #284

Merged
merged 9 commits into from
Jun 29, 2024
Merged

Conversation

cyphersnake
Copy link
Collaborator

@cyphersnake cyphersnake commented Jun 7, 2024

Motivation
Close #260

Overview

  • Implementation of compute_G via tree_reduce
  • Multithreaded implementation of fold_witness
  • Single-threaded fold_plonk_challenges implementation (there are not so many items, so there is no sense to use rayon)
  • An implementation of multi_product, analogous to itertools, but without Clone.
  • An implementation of try_multi_product, analogous to itertools, but with processing Result in Item. This allows not to finish processing when the first error occurs.
  • Allocate the folded_trace module to control the size of protogalaxy::poly

@cyphersnake cyphersnake self-assigned this Jun 7, 2024
@cyphersnake cyphersnake changed the title 260 compute g feat(nifs): impl protogalaxy compute G poly Jun 7, 2024
src/nifs/protogalaxy/poly.rs Outdated Show resolved Hide resolved
src/nifs/protogalaxy/poly.rs Outdated Show resolved Hide resolved
Base automatically changed from 259-feat-nifs-compute_f to main June 10, 2024 15:05
@cyphersnake cyphersnake force-pushed the 260-compute-G branch 3 times, most recently from d01110d to 6b0ae01 Compare June 11, 2024 14:18
@cyphersnake cyphersnake changed the title feat(nifs): impl protogalaxy compute G poly feat(nifs): impl protogalaxy::poly::compute_G Jun 11, 2024
@cyphersnake cyphersnake marked this pull request as ready for review June 11, 2024 14:50
@cyphersnake cyphersnake requested a review from chaosma June 11, 2024 14:50
@cyphersnake
Copy link
Collaborator Author

The algorithm separated from the context is here, for your reference

https://gist.github.com/rust-play/89ae0609d30feb22d2585a6f43ccc07e

Copy link
Collaborator

@chaosma chaosma left a comment

Choose a reason for hiding this comment

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

Finish the first round of review. The fold_challenges looks good to me (still left some comments). The 'fold_witness` has some issues to be addressed.

src/nifs/protogalaxy/poly/folded_trace.rs Outdated Show resolved Hide resolved
src/nifs/protogalaxy/poly/folded_trace.rs Outdated Show resolved Hide resolved
src/nifs/protogalaxy/poly/folded_trace.rs Outdated Show resolved Hide resolved
src/nifs/protogalaxy/poly/folded_trace.rs Show resolved Hide resolved
@cyphersnake
Copy link
Collaborator Author

I did not take into account that PlonkWitness is not a matrix, in general it is not hard to correct by making the indexes trickier

I will postpone this task for now due to higher priority, however, the fix will be not very large

@cyphersnake
Copy link
Collaborator Author

Benchmark data is still being collected, but there doesn't seem to be any code left there, I'm going back to this task

@cyphersnake
Copy link
Collaborator Author

cyphersnake commented Jun 27, 2024

Generally fixed, but because of cloning there is multiple rows traversal, which can be annoying. It is also possible to create result vectors more optimally.

But in order not to delay, I suggest to return to this optimization, if it becomes bottle neck

@cyphersnake cyphersnake requested a review from chaosma June 27, 2024 13:49
src/nifs/protogalaxy/poly/folded_trace.rs Outdated Show resolved Hide resolved
src/nifs/protogalaxy/poly/folded_trace.rs Show resolved Hide resolved
src/nifs/protogalaxy/poly/folded_trace.rs Show resolved Hide resolved
src/nifs/protogalaxy/poly/mod.rs Show resolved Hide resolved
src/nifs/protogalaxy/poly/mod.rs Outdated Show resolved Hide resolved
src/nifs/protogalaxy/poly/mod.rs Show resolved Hide resolved
src/util.rs Show resolved Hide resolved
@cyphersnake
Copy link
Collaborator Author

@chaosma
If you don't understand this big iterator, let me know, we may need a call to write a good doc to it together. It's really not very intuitive, but I had to sacrifice understandability for optimality

@cyphersnake cyphersnake requested a review from chaosma June 28, 2024 10:27
@chaosma
Copy link
Collaborator

chaosma commented Jun 28, 2024

@chaosma If you don't understand this big iterator, let me know, we may need a call to write a good doc to it together. It's really not very intuitive, but I had to sacrifice understandability for optimality

I think I got it. Other comments are addressed. I leave a new comment there.

@cyphersnake cyphersnake merged commit a17c479 into main Jun 29, 2024
1 check passed
@cyphersnake cyphersnake deleted the 260-compute-G branch June 29, 2024 10:21
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.

feat(nifs): compute_G
2 participants