Merge all “external” benchmarks into a single bench binary #991

1. 
   sipa commented at 6:07 pm on October 17, 2021: contributor

   This combines bench_verify, bench_sign, bench_ecdh, bench_recovery, and bench_schnorrsig into a single bench binary.

   I don’t think there is a good reason to have this many binaries, and it complicates build config and CI.

2. sipa force-pushed on Oct 17, 2021
3. 
   real-or-random commented at 10:06 am on October 18, 2021: contributor

   I don’t think there is a good reason to have this many binaries, and it complicates build config and CI.

   One possible drawback is that if you want to run a really high iteration of tests (e.g., taking a minute of sig verify), then it’s somewhat annoying if your test is late in the list. Of course you can edit the source files but it’s not great either.

4. 
   sipa commented at 2:45 pm on October 18, 2021: contributor

   @real-or-random How about adding a way to select which benchmark(s) to run using keywords, like bench_intern has?
5. 
   real-or-random commented at 4:15 pm on October 18, 2021: contributor

   That’s the optimal solution of course. I thought it’s a little bit more work but indeed, we have the code already in the bench_internal! So Concept ACK for that idea.
6. Rogelio165 approved
7. 
   real-or-random commented at 8:21 am on October 29, 2021: contributor

   needs rebase
8. Combine bench_sign and bench_verify into single bench 2a7be678a6
9. Merge bench_ecdh into bench 855e18d8a8
10. Merge bench_recover into bench 3208557ae1
11. Merge bench_schnorrsig into bench 9f56bdf5b9
12. Make bench support selecting which benchmarks to run af6abcb3d0
13. sipa force-pushed on Nov 5, 2021
14. 
    sipa commented at 9:49 pm on November 5, 2021: contributor

    Rebased, and added the ability to select on the command line which benchmarks to run.
15. real-or-random approved
16. 
    real-or-random commented at 8:44 am on November 6, 2021: contributor

    ACK af6abcb3d0097a7f7892fb8b54a4c6363e5c2c7f diff looks good, command line options work, valgrind is happy
17. 
    siv2r commented at 4:19 pm on November 6, 2021: contributor

    tACK af6abcb, the command-line options work as expected on my ubuntu machine. The diff looks good.

    you can find my command-line output here.

    These are the few things that I observed: (not sure if we need to change them)

    1. Both ./bench and ./bench ecdsa runs all benchmark. (since the have_flag("edcsa") is present on both verify and sign benchmarks.)
    2. If a module is enabled (let’s say schnorrsig), the benchmark of that module cannot be run separately like we do for sign and verify.
18. 
    real-or-random commented at 4:54 pm on November 6, 2021: contributor

    Maybe I’m misunderstating your observations but I can’t confirm.

    1. Both `./bench` and `./bench ecdsa` runs all benchmark. (_since the `have_flag("edcsa")` is present on both verify and sign benchmarks._)
    

    Indeed but you can also specify ./bench ecdsa_verify or ./bench ecdsa_sign.

    2. If a module is enabled (let's say schnorrsig), the benchmark of that module cannot be run separately like we do for sign and verify.
    

    You can also specify ./bench schnorrsig_verify or ./bench schnorrsig_sign.

19. 
    siv2r commented at 5:56 pm on November 6, 2021: contributor

    You can also specify ./bench schnorrsig_verify or ./bench schnorrsig_sign.

    Ohh.. I didn’t review the src/modules/schnorrsig/bench_impl.h properly. This works. Thanks!

    Indeed but you can also specify ./bench ecdsa_verify or ./bench ecdsa_sign.

    Yes, I thought both the ./bench and ./bench ecdsa were equivalent. Now, I see that I was wrong. If we have a module enabled they act differently.

    Everything looks good!

20. 
    jonasnick commented at 10:59 pm on November 6, 2021: contributor

    Concept ACK, played around with this a bit and it works as one would expect

    A follow-up PR could add a --help which would also mention the get_iters environment variable (I have to look up in the source how it’s called exactly every time).

21. real-or-random merged this on Nov 8, 2021
22. real-or-random closed this on Nov 8, 2021

    ---

23. real-or-random cross-referenced this on Nov 8, 2021 from issue Improve documentation for benchmarks by real-or-random
24. sipa referenced this in commit d057eae556 on Dec 2, 2021
25. sipa cross-referenced this on Dec 2, 2021 from issue Update libsecp256k1 subtree to current master by sipa
26. fanquake referenced this in commit c4a1e09a8c on Dec 3, 2021
27. sipa referenced this in commit 86dbc4d075 on Dec 15, 2021
28. jonasnick cross-referenced this on Jan 2, 2022 from issue Sync Upstream by jonasnick
29. real-or-random referenced this in commit 21e2d65b79 on Jan 5, 2022
30. gwillen referenced this in commit 35d6112a72 on May 25, 2022
31. janus referenced this in commit 879a9a27b9 on Jul 10, 2022
32. patricklodder referenced this in commit 21badcf9d2 on Jul 25, 2022
33. patricklodder referenced this in commit 03002a9013 on Jul 28, 2022
34. backpacker69 referenced this in commit 77186f4a04 on Jan 18, 2023
35. str4d referenced this in commit 6de4698bf9 on Apr 21, 2023
36. vmta referenced this in commit e1120c94a1 on Jun 4, 2023
37. vmta referenced this in commit 8f03457eed on Jul 1, 2023

Contributors
sipa real-or-random Rogelio165 siv2r jonasnick