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

UltraHonk Benchmarks #477

Merged
merged 11 commits into from
Jun 2, 2023
Merged

UltraHonk Benchmarks #477

merged 11 commits into from
Jun 2, 2023

Conversation

ledwards2225
Copy link
Collaborator

@ledwards2225 ledwards2225 commented May 24, 2023

Description

First component of #480: Introduces a suite of UltraHonk benchmarks for proof construction for four different circuits, each of which contains a specified number (currently 10) of one of the following individual operations:

  • sha256
  • keccak
  • ecdsa verification
  • merkle membership

Additional required changes in this PR include instantiation of stdlib types with UltraHonkComposer and introduction of several required methods in the UltraHonkComposer (all of which are pass-throughs to existing methods in the UltraCircuitConstructor).

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 force-pushed the lde/benchmark_workflow branch 5 times, most recently from dc8085e to 5712466 Compare May 30, 2023 16:58
@ledwards2225 ledwards2225 marked this pull request as ready for review May 30, 2023 21:18
@ledwards2225 ledwards2225 force-pushed the lde/benchmark_workflow branch from f4935d8 to 2bf4cad Compare May 31, 2023 14:34
@ledwards2225 ledwards2225 changed the title Benchmark workflow UltraHonk Benchmarks May 31, 2023
@ledwards2225 ledwards2225 self-assigned this Jun 1, 2023
@ledwards2225 ledwards2225 linked an issue Jun 1, 2023 that may be closed by this pull request
@ledwards2225 ledwards2225 removed a link to an issue Jun 1, 2023
@ledwards2225 ledwards2225 force-pushed the lde/benchmark_workflow branch from 5140837 to 7307ff6 Compare June 1, 2023 13:00
@ledwards2225 ledwards2225 requested a review from Rumata888 June 1, 2023 15:46
@@ -58,12 +61,15 @@ class UltraCircuitConstructor;

#define EXTERN_STDLIB_ULTRA_TYPE(stdlib_type) \
extern template class stdlib_type<proof_system::UltraCircuitConstructor>; \
extern template class stdlib_type<plonk::UltraPlonkComposer>;
extern template class stdlib_type<plonk::UltraPlonkComposer>; \
// extern template class stdlib_type<honk::UltraHonkComposer>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this commented out?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

whoops - thanks

{
auto num_gates = 1 << (size_t)state.range(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use range multiplier (https://github.com/google/benchmark/blob/main/docs/user_guide.md#custom-counters) as an alternative if you want the exponent to be the metric.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, I feel like there's lots of fun capabilities here that I need to dig into. For now I'll leave it as is since I'm not even sure we care about the standard honk benchmarks much anymore

// Number of times to perform operation of interest in the benchmark circuits, e.g. # of hashes to perform
constexpr size_t MIN_NUM_ITERATIONS = 10;
constexpr size_t MAX_NUM_ITERATIONS = 10;
// Numeber of times to repeat each benchmark
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo

Copy link
Contributor

@Rumata888 Rumata888 left a comment

Choose a reason for hiding this comment

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

LGTM

@ledwards2225 ledwards2225 force-pushed the lde/benchmark_workflow branch from 378fe74 to e92b89e Compare June 2, 2023 00:02
@ledwards2225 ledwards2225 merged commit 5229cf7 into master Jun 2, 2023
@ledwards2225 ledwards2225 deleted the lde/benchmark_workflow branch June 2, 2023 00:24
@ledwards2225 ledwards2225 linked an issue Jun 21, 2023 that may be closed by this pull request
ludamad pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Jul 22, 2023
* basic benchmarks for ultra honk

* remove github action workflow altogether for now

* update compare script
ludamad pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Jul 24, 2023
* basic benchmarks for ultra honk

* remove github action workflow altogether for now

* update compare script
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.

Benchmarks for UltraHonk
2 participants