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

Add basic group operation benchmarks #26

Merged
merged 3 commits into from
Aug 7, 2023
Merged

Add basic group operation benchmarks #26

merged 3 commits into from
Aug 7, 2023

Conversation

davidnevadoc
Copy link
Contributor

@davidnevadoc davidnevadoc commented Feb 6, 2023

The purpose of this PR is to add benchmarks for all basic group operations.
In particular we want to evaluate the impact on performance that the changes introduced by #19 have had.

@han0110 han0110 self-requested a review February 7, 2023 05:53
benches/group.rs Outdated
Comment on lines 12 to 13
let a = G::generator();
let a_affine = G::AffineExt::generator();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use some random value here? Since the old algorithm seems to have some optimization for adding the same point (

if s1 == s2 {
and
if self.y == s2 {
)

benches/group.rs Outdated
b.iter(|| black_box(a).double())
});
c.bench_function(&format!("{} addition", name), move |b| {
b.iter(|| black_box(a).add(&a))
Copy link
Contributor

Choose a reason for hiding this comment

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

Same reason as above, we might want to introduce another different value as added.

@davidnevadoc davidnevadoc marked this pull request as ready for review August 4, 2023 21:19
@davidnevadoc davidnevadoc changed the title [WIP ]Add basic group operation benchmarks Add basic group operation benchmarks Aug 4, 2023
Copy link
Contributor

@han0110 han0110 left a comment

Choose a reason for hiding this comment

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

LGTM! Just 1 more question.

Comment on lines 3 to 4
mod group;

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems unnecessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, fixed in 9420284

Cargo.toml Outdated
Comment on lines 12 to 15
[[bench]]
name = "group"
harness = false

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we could move this to the bottom along with other [[bench]]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 9420284

Copy link
Contributor

@han0110 han0110 left a comment

Choose a reason for hiding this comment

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

LGTM

@davidnevadoc davidnevadoc added this pull request to the merge queue Aug 7, 2023
Merged via the queue into main with commit 526c07c Aug 7, 2023
7 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.

2 participants