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.

Sign up for GitHub

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

Jump to bottom

chore: simplify and homogenize proving code for Nova and SuperNova proofs #1121

Merged
merged 1 commit into from
Feb 13, 2024
Merged

chore: simplify and homogenize proving code for Nova and SuperNova proofs #1121

merged 1 commit into from
Feb 13, 2024

Conversation

arthurpaulino
Copy link
Contributor

@arthurpaulino arthurpaulino commented Feb 12, 2024 •
edited
Loading

  • Remove code duplication in Nova proving
  • Factor out and reuse debugging code that was triggered only for Nova proving without "parallel steps"
  • Remove the cloning of recursive snarks on SuperNova
  • Drop crossbeam dependency because crossbeam::thread::scope has been soft-deprecated
    in favor of std::thread::scope since Rust 1.63 due to performance reasons

@arthurpaulino arthurpaulino requested a review from a team as a code owner February 12, 2024 22:35
@arthurpaulino arthurpaulino force-pushed the ap/clearer-folding branch 2 times, most recently from f2fc6a0 to 604143c Compare February 13, 2024 13:31
huitseeker
huitseeker previously approved these changes Feb 13, 2024
View reviewed changes
Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@arthurpaulino arthurpaulino added this pull request to the merge queue Feb 13, 2024
@github-actions github-actions bot mentioned this pull request Feb 13, 2024
Closed
@arthurpaulino arthurpaulino removed this pull request from the merge queue due to a manual request Feb 13, 2024
@arthurpaulino
Copy link
Contributor Author

Huh oh, performance regression?! I've removed it from the merge queue 🤔

@arthurpaulino
Copy link
Contributor Author

!gpu-benchmark

huitseeker
huitseeker approved these changes Feb 13, 2024
View reviewed changes
Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

Let's see if this is a fluke.

@arthurpaulino
Copy link
Contributor Author

I think it's not a fluke. It's probably because I removed the instantiation of the initial SNARK from the parallelization scheme. I'm thinking of a solution that doesn't make the code complex again

huitseeker
huitseeker suggested changes Feb 13, 2024
View reviewed changes
src/proof/nova.rs Outdated Show resolved Hide resolved
src/proof/nova.rs Outdated Show resolved Hide resolved
@arthurpaulino
Copy link
Contributor Author

!benchmark --features cuda,asm

@arthurpaulino
Copy link
Contributor Author

!benchmark --features cuda

huitseeker
huitseeker reviewed Feb 13, 2024
View reviewed changes
src/proof/nova.rs Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link
Contributor

Benchmark for 4eed503

Click to view benchmark
Test Base PR %
LEM Fibonacci Prove - rc = 100/fib/num-100 1450.8±8.91ms 1449.6±11.46ms -0.08%
LEM Fibonacci Prove - rc = 100/fib/num-200 2.8±0.01s 2.8±0.01s 0.00%
LEM Fibonacci Prove - rc = 600/fib/num-100 1827.9±8.42ms 1816.7±6.94ms -0.61%
LEM Fibonacci Prove - rc = 600/fib/num-200 3.0±0.01s 3.0±0.01s 0.00%

huitseeker
huitseeker suggested changes Feb 13, 2024
View reviewed changes
Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

Made a suggestion to use a closure. Please note I've left:

  • adapting the call sites,
  • probing for and using the debug variable to trigger the debug step,
    as an exercise to the reader.

src/proof/nova.rs Outdated Show resolved Hide resolved
@arthurpaulino
Copy link
Contributor Author

Aha! take was the thing I was missing.

@arthurpaulino
chore: simplify and homogenize proving code for Nova and SuperNova pr…
9103b74 
…oofs

* Remove code duplication in Nova proving
* Factor out and reuse debugging code that was triggered only for Nova proving without "parallel steps"
* Remove the cloning of recursive snarks on SuperNova
* Drop `crossbeam` dependency because `crossbeam::thread::scope` has been soft-deprecated
  in favor of `std::thread::scope` since Rust 1.63 due to performance reasons
@arthurpaulino
Copy link
Contributor Author

!benchmark --features cuda

@github-actions GitHub Actions
Copy link
Contributor

Benchmark for 4eed503

Click to view benchmark
Test Base PR %
LEM Fibonacci Prove - rc = 100/fib/num-100 1454.9±9.81ms 1455.8±6.57ms +0.06%
LEM Fibonacci Prove - rc = 100/fib/num-200 2.8±0.00s 2.8±0.01s 0.00%
LEM Fibonacci Prove - rc = 600/fib/num-100 1858.1±16.68ms 1847.1±6.93ms -0.59%
LEM Fibonacci Prove - rc = 600/fib/num-200 3.1±0.02s 3.1±0.01s 0.00%

huitseeker
huitseeker approved these changes Feb 13, 2024
View reviewed changes
Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

🙏

@arthurpaulino arthurpaulino added this pull request to the merge queue Feb 13, 2024
Merged via the queue into main with commit 1d4d308 Feb 13, 2024
11 checks passed
@arthurpaulino arthurpaulino deleted the ap/clearer-folding branch February 13, 2024 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@huitseeker huitseeker huitseeker approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@arthurpaulino @huitseeker