-
Notifications
You must be signed in to change notification settings - Fork 158
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
perf: gkr improvements #328
Conversation
@@ -113,7 +113,7 @@ func TestSumcheckFromSingleInputTwoIdentityGatesGateTwoInstances(t *testing.T) { | |||
pool := polynomial.NewPool(256, 1<<11) | |||
workers := utils.NewWorkerPool() | |||
o.pool = &pool | |||
o.workers = &workers | |||
o.workers = workers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a must? In the gnark API, I was creating a pool of workers for the GKR solver first and then passing the same one to the prover instead of taking it down and setting it up again, hence the pointer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utils.NewWorkerPool
returns a pointer 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(that being said, the pool is lightweight and it's OK to have 2 pools that are properly stopped using the "Stop" method between Solver and Prover; memory wise with stack / heap usage of the go routine of the pool that may actually work better (to be seen :))
On my M1 laptop for bn254 gkr+mimc benchmarks (didn't test on the hpc machine, can you do it @Tabaie ? )