Skip to content

Conversation

@kevkevinpal
Copy link
Contributor

Description

I noticed that in both bench and bench_internal there was a help text mentioning to the users that you could use SECP256K1_BENCH_ITERS to customize the amount of iterations for each benchmark, but bench_ecmult did not have this text so I added it in.

There is a caveat that if SECP256K1_BENCH_ITERS is less than 3, then run_ecmult_multi_bench will not run. Should I add that to the help text?


Before

$ ./build/bin/bench_ecmult -h
Benchmark EC multiplication algorithms

Usage: ./build/bin/bench_ecmult <help|pippenger_wnaf|strauss_wnaf|simple>
The output shows the number of multiplied and summed points right after the
function name. The letter 'g' indicates that one of the points is the generator.
The benchmarks are divided by the number of points.

default (ecmult_multi): picks pippenger_wnaf or strauss_wnaf depending on the
                        batch size
pippenger_wnaf:         for all batch sizes
strauss_wnaf:           for all batch sizes
simple:                 multiply and sum each point individually

After

$ ./build/bin/bench_ecmult -h
Benchmark EC multiplication algorithms

The default number of iterations for each benchmark is 10000. This can be
customized using the SECP256K1_BENCH_ITERS environment variable.

Usage: ./build/bin/bench_ecmult <help|pippenger_wnaf|strauss_wnaf|simple>
The output shows the number of multiplied and summed points right after the
function name. The letter 'g' indicates that one of the points is the generator.
The benchmarks are divided by the number of points.

default (ecmult_multi): picks pippenger_wnaf or strauss_wnaf depending on the
                        batch size
pippenger_wnaf:         for all batch sizes
strauss_wnaf:           for all batch sizes
simple:                 multiply and sum each point individually

@real-or-random
Copy link
Contributor

There is a caveat that if SECP256K1_BENCH_ITERS is less than 3, then run_ecmult_multi_bench will not run. Should I add that to the help text?

I don't see why this is. Can you explain?

If that's the case, we could just check this condition and bail out (instead of documenting it).

@kevkevinpal
Copy link
Contributor Author

I don't see why this is. Can you explain?

Yea if you take a look at this block of code if iters is less than 3 we don't run run_ecmult_multi_bench

If that's the case, we could just check this condition and bail out (instead of documenting it).

We already bail out in this block, I was just suggesting if we add documentation in the help text for SECP256K1_BENCH_ITERS we can note that it will bail out. But we don't need to if that is too verbose.

@real-or-random
Copy link
Contributor

real-or-random commented Jan 7, 2026

Oh yes, I missed that if. What about adding an else block that prints a message like "Skipping some benchmarks due to SECP256K1_BENCH_ITERS <= 2"?

@kevkevinpal kevkevinpal force-pushed the addBenchItersEnvVarHelp branch from 3eabc77 to 8f331c9 Compare January 7, 2026 15:35
@kevkevinpal
Copy link
Contributor Author

Oh yes, I missed that if. What about adding an else block that prints a message like "Skipping some benchmarks due to SECP256K1_BENCH_ITERS <= 2"?

Sounds good I updated it in 8f331c9

@kevkevinpal
Copy link
Contributor Author

This might be good for a follow-up but I noticed that if we put any value for SECP256K1_BENCH_ITERS not greater than 0

It will print Floating point exception (core dumped)

A simple check would fix that

Example

(addBenchItersEnvVarHelp) secp256k1 $ SECP256K1_BENCH_ITERS=0 ./build/bin/bench
Benchmark                     ,    Min(us)    ,    Avg(us)    ,    Max(us)

Floating point exception (core dumped)
(addBenchItersEnvVarHelp) secp256k1 $ SECP256K1_BENCH_ITERS=abc ./build/bin/bench
Benchmark                     ,    Min(us)    ,    Avg(us)    ,    Max(us)

Floating point exception (core dumped)
(addBenchItersEnvVarHelp) secp256k1 $ SECP256K1_BENCH_ITERS= ./build/bin/bench
Benchmark                     ,    Min(us)    ,    Avg(us)    ,    Max(us)

Floating point exception (core dumped)

…h_ecmult

In addition a print message saying some tests were skipped was added
@kevkevinpal kevkevinpal force-pushed the addBenchItersEnvVarHelp branch from 8f331c9 to bd5ced1 Compare January 7, 2026 18:02
Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

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

utACK bd5ced1

@real-or-random
Copy link
Contributor

This might be good for a follow-up but I noticed that if we put any value for SECP256K1_BENCH_ITERS not greater than 0

It will print Floating point exception (core dumped)

Indeed, I had noticed the same when testing this earlier. Happy to see a fix for this.

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK bd5ced1, I have reviewed the code and it looks OK. Tested on Ubuntu 25.10.

While aligning implementation across all benchmarks, argv could be passed to the help() in bench.c and bench_internal.c.

@real-or-random
A side question. What does 77 refer to in:

/* This is disabled with low count of iterations because the loop runs 77 times even with iters=1
?

@kevkevinpal
Copy link
Contributor Author

@real-or-random
A side question. What does 77 refer to in:

/* This is disabled with low count of iterations because the loop runs 77 times even with iters=1
?

I see it was added in this PR #722.

It might have been added so Travis CI doesn't run this benchmark so many times, thus slowing down the CI.

@kevkevinpal
Copy link
Contributor Author

While aligning implementation across all benchmarks, argv could be passed to the help() in bench.c and bench_internal.c.

I can work on this and open in a separate refactor/bench PR to keep the scope of this one minimal.

Copy link
Contributor

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

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

ACK bd5ced1

@jonasnick jonasnick merged commit 4721e07 into bitcoin-core:master Jan 11, 2026
122 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tweak/refactor user-documentation user-facing documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants