-
Notifications
You must be signed in to change notification settings - Fork 1k
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 ecmult_gen, ecmult_const and ecmult to benchmark #662
Conversation
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.
Approach ACK
/* Hashes x into [0, POINTS) twice and store the result in offset1 and offset2. */ | ||
static void hash_into_offset(bench_data* data, size_t x) { | ||
data->offset1 = (x * 0x537b7f6f + 0x8f66a481) % POINTS; | ||
data->offset2 = (x * 0x7f6f537b + 0x6a1a8f49) % POINTS; |
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.
where's this thing from?
I see it's also in bench_ecmult_setup
but can't find the source of this
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.
I think is this is just a standard integer hash function (akin to https://en.wikipedia.org/wiki/Universal_hashing#Hashing_integers) whose parameters sipa selected. But looking at it now it may be give poor results because POINTS
is not prime.
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.
I don't think points needs to be prime, so long as the multipliers are coprime to it.
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.
fwiw they're all coprime to POINTS.
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.
Looks good otherwise
src/bench_ecmult.c
Outdated
bench_ecmult_teardown_helper(data, &data->offset1, &data->offset2, NULL); | ||
} | ||
|
||
static void bench_ecmult_1g(void* arg) { |
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.
static void bench_ecmult_1g(void* arg) { | |
static void bench_ecmult_0g(void* arg) { |
src/bench_ecmult.c
Outdated
} | ||
} | ||
|
||
static void bench_ecmult_1g_teardown(void* arg) { |
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.
static void bench_ecmult_1g_teardown(void* arg) { | |
static void bench_ecmult_0g_teardown(void* arg) { |
and so on..
src/bench_ecmult.c
Outdated
bench_ecmult_teardown_helper(data, NULL, NULL, &data->offset1); | ||
} | ||
|
||
static void bench_ecmult_2g(void* arg) { |
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.
1g
@real-or-random I don't think so (unless we change it for ecmult_multi) but it took me a while to figure this out again. I added a commit that printfs an explanation. It's not pretty, but less confusing. |
Certainly pretty enough for a benchmark. Can you squash? |
5b3e10e
to
6a5673f
Compare
squashed |
ACK a419f3d I read the code and tried it |
5ebe192
to
82a5911
Compare
Rebased and added a |
src/bench_ecmult.c
Outdated
} else { | ||
fprintf(stderr, "%s: unrecognized argument '%s'.\n", argv[0], argv[1]); | ||
fprintf(stderr, "Use 'pippenger_wnaf', 'strauss_wnaf', 'simple' or no argument to benchmark a combined algorithm.\n"); | ||
fprintf(stderr, "Use '--help', 'pippenger_wnaf', 'strauss_wnaf', 'simple' or no argument to benchmark a combined algorithm.\n"); |
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.
simply call help
here?
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.
okay
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.
Ah sorry, my suggestion was just to replace the entire else
branch with help()
, not to replace the --help
by help
. Feel free to change back, or to ignore.
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
data.ecmult_multi = secp256k1_ecmult_multi_var; | ||
secp256k1_scratch_space_destroy(data.ctx, data.scratch); | ||
data.scratch = NULL; |
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.
That line got lost, and this was what made the difference between simple and combined? I see you merged this into "simple combined" in the printfs. Was this change on purpose?
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.
Good catch. I also forgot what the simple algorithm actually does. I fixed this and added an explanation to the help().
@real-or-random care to reACK :) ? |
ACK ee83dbf |
c6867e1
to
8f879c2
Compare
Rebased to let CI do its thing |
While we're at it: are no-g variants not interesting? |
Hm with-g seems more interesting (schnorrsig batch verify, taproot tweaks, pedersen commitments,...) but we could add a flag or something for no-g (preferably in a separate PR imho). |
ACK 8f879c2 |
tACK 8f879c2 nit, It is a little confusing that the first 5 benchmarks are unrelated to the flag being passed (Maybe we want to move the "Using XXX algorithm" print to just before the relevant benchmarks?) |
Indeed but this can be done in a separate PR, too. Let me take the opportunity to merge this PR now that we have two ACKs. |
I was trying to determine the impact of ecmult_gen in schnorrsig signing and noticed that there is no way to bench this right now. The new benchmarks look like this:
(Turns out ecmult_gen is 37% of the 55.8us that schnorrsig sign takes)