-
Notifications
You must be signed in to change notification settings - Fork 26
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
improve batching sumcheck concurrency to dominate by max num var polynomial #895
Merged
hero78119
merged 12 commits into
scroll-tech:master
from
hero78119:feat/optimize_sumcheck_concurrency
Apr 12, 2025
Merged
improve batching sumcheck concurrency to dominate by max num var polynomial #895
hero78119
merged 12 commits into
scroll-tech:master
from
hero78119:feat/optimize_sumcheck_concurrency
Apr 12, 2025
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-merge-queue bot
pushed a commit
that referenced
this pull request
Apr 9, 2025
Extract from PR #895 Remove forked transcript to simplify design. ### benchmark To assure no impact for e2e Fibonacci 2^20 ``` fibonacci_max_steps_1048576/prove_fibonacci/fibonacci_max_steps_1048576 time: [2.8696 s 2.8901 s 2.9127 s] change: [-1.0773% -0.1480% +0.8449%] (p = 0.79 > 0.05) No change in performance detected. ``` Fibonacci 2^21 ``` fibonacci_max_steps_2097152/prove_fibonacci/fibonacci_max_steps_2097152 time: [5.2648 s 5.2801 s 5.2959 s] change: [+0.4221% +0.9989% +1.5705%] (p = 0.00 < 0.05) Change within noise threshold. ```
spherel
approved these changes
Apr 11, 2025
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.
LGTM!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously (devirgo) sumcheck concurrency when batching with different number of variables, are dominated by the smallest num_vars. The reason is because for the num_var < log_2(threads) we can NOT divide it's evaluation into #thread region.
This PR addressed the issue by introducing a extra meta information for each polynomial, so we are able to differentiate those small poly and calculate its multiplicity correctly
Design rationale
For some extreme small polynomial (num_var << log(threads)), we define a new
PolyType::Phase2Only
to differentiate them. These type of small polynomial will be handle by main_worker only during phase 1 sumcheck. And when moving to phase2 sumcheck, those small poly will expand the size to match with all. This cost is negligible giving phase2 only need to run on #thread evaluations.benchmark
There is no different with before/after change, since we dont have batch sumcheck with different num_vars in critical path, which meet expected
e2e Fibonacci 2^20
e2e Fibonacci 2^21
e2e Fibonacci 2^22