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

Update & clean benchmarks #165

Merged
merged 2 commits into from
Jun 21, 2024
Merged

Update & clean benchmarks #165

merged 2 commits into from
Jun 21, 2024

Conversation

davidnevadoc
Copy link
Contributor

@davidnevadoc davidnevadoc commented Jun 19, 2024

Description

This PR is a general cleanup of the benchmarks.

Changes

  • Rename bn256_field -> field_arith
  • Rename group -> curve
  • Remove less_than benchmark.
  • Add pairing benchmark.
  • Use bn256 in all benchmarks. ( All benchmarks are generic so they can be easily run for any other field or curve).
  • Other small fixes.

@davidnevadoc davidnevadoc marked this pull request as ready for review June 19, 2024 13:38
@davidnevadoc davidnevadoc requested a review from adria0 June 19, 2024 14:50
@@ -38,8 +42,9 @@ fn generate_data(k: u32) -> Vec<Scalar> {

fn fft(c: &mut Criterion) {
Copy link
Member

@adria0 adria0 Jun 20, 2024

Choose a reason for hiding this comment

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

AFAIS best_fft can use different algorithms, depending on log_n <= log_threads.
Asking myself if it's a good performance test if we do not control the number of threads here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm 🤔 I'd say it only makes sense to compare results when the number of threads used is the same (and therefore the same algorithm is used). It is true that this can show up as a weird trend when comparing the performance of the fft at different sizes in the same system.

benches/hash_to_curve.rs Outdated Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
@guorong009
Copy link

The PR looks good to me! @davidnevadoc
Once @adria0 approves the PR, I believe it can be merged. 🙂

@davidnevadoc davidnevadoc added this pull request to the merge queue Jun 21, 2024
Merged via the queue into main with commit 63d44ee Jun 21, 2024
12 checks passed
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.

3 participants