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

feat: Benchmark suite update #508

Merged
merged 24 commits into from
Jun 15, 2023
Merged

feat: Benchmark suite update #508

merged 24 commits into from
Jun 15, 2023

Conversation

ledwards2225
Copy link
Collaborator

@ledwards2225 ledwards2225 commented Jun 5, 2023 •
edited
Loading

Description

This PR has two primary components:

  1. Establishes some common circuits for benchmarking the Standard and Ultra variants of Plonk and Honk and introduces scripts for generating comparisons between Plonk and Honk for both Standard/Ultra. It also updates the script for comparing a development branch to master to use Ultra Honk. (This is the script to be used during development to evaluate the effectiveness of an optimization).

  2. Sets up a GitHub Action to run the UltraHonk benchmarks on pushes to master. The results are automatically used to update the plots here.

Checklist:

  • I have reviewed my diff in github, line by line.
  • Every change is related to the PR description.
  • I have linked this pull request to the issue(s) that it resolves.
  • There are no unexpected formatting changes, superfluous debug logs, or commented-out code.
  • There are no circuit changes, OR specifications in /markdown/specs have been updated.
  • There are no circuit changes, OR a cryptographer has been assigned for review.
  • I've updated any terraform that needs updating (e.g. environment variables) for deployment.
  • The branch has been rebased against the head of its merge target.
  • I'm happy for the PR to be merged at the reviewer's next convenience.
  • New functions, classes, etc. have been documented according to the doxygen comment format. Classes and structs must have @brief describing the intended functionality.
  • If existing code has been modified, such documentation has been added or updated.

@ledwards2225 ledwards2225 self-assigned this Jun 6, 2023
@ledwards2225 ledwards2225 force-pushed the lde/ultra_bench_update branch from afdbf8c to 0000ce9 Compare June 6, 2023 16:39
@ledwards2225 ledwards2225 marked this pull request as ready for review June 7, 2023 19:52
@ledwards2225 ledwards2225 changed the title Benchmark suite update feat: Benchmark suite update Jun 12, 2023
@ledwards2225 ledwards2225 force-pushed the lde/ultra_bench_update branch from cd02df3 to e7498b3 Compare June 12, 2023 20:52
@ledwards2225 ledwards2225 linked an issue Jun 14, 2023 that may be closed by this pull request
Benchmark Workflow #480
Closed
@ledwards2225 ledwards2225 requested a review from Rumata888 June 14, 2023 14:17
Rumata888
Rumata888 reviewed Jun 14, 2023
View reviewed changes
.github/workflows/benchmarks.yml
# Enable Job Summary for PRs
summary-always: true
# Show alert with commit comment on detecting possible performance regression
alert-threshold: '120%' # alert if bench result is 1.2x worse
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this be enough if there are incremental changes that drain performance?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To be honest I'm viewing the GH Action portion of this work as just a nice to have with pretty pictures. I think it needs to be incumbent upon the developer to manually check the benchmarks in their branch any time they suspect a change. I suppose we could also automate running the benchmarks and comparing with master but the dev would still have to manually check the result

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a nice facility, so why not use it? If it's possible to add several alerts, I'd do a 90% alert, too. It can be even more disturbing if the circuit shrinks for no reason

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looking at the docs it seems that there's no way to have an upper and lower bound for the alert. The nice thing about having the autogenerated plots is that unexpected changes will not go unnoticed and the offending commit is displayed. Maybe I'll change this threshold to 1.05x and we'll see how that goes. I'm not sure what the random variation will be

Rumata888
Rumata888 reviewed Jun 14, 2023
View reviewed changes
cpp/src/barretenberg/benchmark/honk_bench/benchmark_utilities.hpp Outdated

typename curve::g1_bigfr_ct public_key = curve::g1_bigfr_ct::from_witness(&composer, account.public_key);

proof_system::plonk::stdlib::ecdsa::signature<Composer> sig{ typename curve::byte_array_ct(&composer, rr),
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd generate several separate unique signatures, because at some point this can devolve into basically one, if we do optimisations correctly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good call, done

Rumata888
Rumata888 reviewed Jun 14, 2023
View reviewed changes
cpp/src/barretenberg/benchmark/honk_bench/benchmark_utilities.hpp
field_ct root = witness_ct(&composer, db.root());

for (size_t i = 0; i < num_iterations; i++) {
merkle_tree::check_membership(
Copy link
Contributor

@Rumata888 Rumata888 Jun 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Here, too. Ideally for several iterations of benchmarks it shouldn't be absolutely the same action. I assume we'll add filtering at some point and a repeat action on the same witness indices would not lead to additional gates

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated to check a different leaf each time

Rumata888
Rumata888 reviewed Jun 14, 2023
View reviewed changes
cpp/src/barretenberg/benchmark/honk_bench/benchmark_utilities.hpp
@@ -0,0 +1,224 @@
#include "barretenberg/crypto/ecdsa/ecdsa.hpp"
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not pressing now, but this file could be split into a cpp and hpp file, so that all instantiations of benchmarks don't reinclude all these hpp files

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Leaving this as a TODO. I'm sure I'll be mucking around in here again soon

@ledwards2225 ledwards2225 force-pushed the lde/ultra_bench_update branch from 4a4b88c to 5bd1b00 Compare June 15, 2023 00:48
@Rumata888 Rumata888 merged commit d7b1499 into master Jun 15, 2023
@Rumata888 Rumata888 deleted the lde/ultra_bench_update branch June 15, 2023 11:54
@github-actions github-actions bot mentioned this pull request Jun 15, 2023
Merged
@ledwards2225 ledwards2225 linked an issue Jun 21, 2023 that may be closed by this pull request
Benchmarks for UltraHonk #453
Closed
@github-actions github-actions bot mentioned this pull request Jul 11, 2023
Merged
ludamad pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Jul 22, 2023
@ledwards2225
feat: Benchmark suite update (AztecProtocol/barretenberg#508)
5d56206 
feat: Benchmark suite update
ludamad pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Jul 24, 2023
@ledwards2225
feat: Benchmark suite update (AztecProtocol/barretenberg#508)
d22e0a9 
feat: Benchmark suite update
This was referenced Jul 25, 2023
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevaundray kevaundray kevaundray left review comments

@Rumata888 Rumata888 Rumata888 approved these changes

@phated phated phated left review comments

Assignees

@ledwards2225 ledwards2225

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Benchmark Workflow Benchmarks for UltraHonk
4 participants
@ledwards2225 @phated @Rumata888 @kevaundray