-
Notifications
You must be signed in to change notification settings - Fork 138
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
Conversation
benches/group.rs
Outdated
let a = G::generator(); | ||
let a_affine = G::AffineExt::generator(); |
There was a problem hiding this comment.
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 (
halo2curves/src/derive/curve.rs
Line 965 in 093c4dc
if s1 == s2 { |
halo2curves/src/derive/curve.rs
Line 1006 in 093c4dc
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)) |
There was a problem hiding this comment.
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.
53b9616
to
e4381e0
Compare
There was a problem hiding this 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.
benches/less_than.rs
Outdated
mod group; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems unnecessary?
There was a problem hiding this comment.
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
[[bench]] | ||
name = "group" | ||
harness = false | ||
|
There was a problem hiding this comment.
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]]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 9420284
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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.