bench, doc: benchmarking updates and fixups #22292

pull jonatack wants to merge 3 commits into bitcoin:master from jonatack:benchmarking-updates changing 3 files +34 −19

jonatack commented at 4:24 PM on June 20, 2021: member

Fixups and updates I noticed while writing benchmarks for #22284.
in src/bench/bench.h:25 in 3c48b78c47 outdated
```
  22 | +static void CodeToTime(benchmark::Bench& bench)
  23 |  {
  24 |      ... do any setup needed...
  25 | -    nanobench::Config().run([&] {
  26 | +
  27 | +    bench.run([&] {
```
jonatack commented at 5:40 PM on June 20, 2021:

Could mention options here if reviewers think it would be helpful.

bench.run([&] { ...or with options... bench.batch(NUM).epochs(NUM).epochIterations(NUM).unit(STRING).run([&] {
jarolrod commented at 10:08 PM on June 21, 2021:

I do think this would be helpful to document here

jonatack commented at 11:43 AM on June 22, 2021:

Thanks for the feedback! Done, and fixed up the help printed by src/bench/bench_bitcoin -?
DrahtBot added the label Docs on Jun 20, 2021
DrahtBot added the label Tests on Jun 20, 2021
kiminuo commented at 8:13 PM on June 21, 2021: contributor

Concept ACK

Thanks for improving this.
jarolrod commented at 10:08 PM on June 21, 2021: member

Concept ACK, is obviously an improvement to the docs
jonatack force-pushed on Jun 22, 2021
in doc/benchmarking.md:42 in 3f408dd7a6 outdated
```
  42 |  
  43 |  Help
  44 |  ---------------------
  45 |  
  46 | -    src/bench/bench_bitcoin --help
  47 | +```
```
fanquake commented at 3:18 AM on June 24, 2021:

I think having just src/bench/bench_bitcoin -? here is fine. Anyone wanting to see the options can run that, and what they see will be correct. Rather than copy-pasting blocks of output here, that will inevitably end up outdated.

jonatack commented at 9:16 AM on June 24, 2021:

Done.
in src/bench/bench.h:24 in 3f408dd7a6 outdated
```
  17 | @@ -18,16 +18,37 @@
  18 |  /*
  19 |   * Usage:
  20 |  
  21 | -static void CODE_TO_TIME(benchmark::Bench& bench)
  22 | +static void NameOfBenchmarkFunction(benchmark::Bench& bench)
  23 |  {
  24 | -    ... do any setup needed...
  25 | -    nanobench::Config().run([&] {
```
fanquake commented at 3:27 AM on June 24, 2021:

I think adding a note to look at nanobench.h for options is fine, but adding an arbitrary list of function signatures, most of which we don't use, seems like overkill (may be unlikely in the case of nanobench, but it's also just more opportunity for docs to diverge from the source). The user will likely have to look at the header anyways, to know what the options do.

jarolrod commented at 4:33 AM on June 24, 2021:

it's also just more opportunity for docs to diverge from the source

that's a good point, lower cost of maintenance

jonatack commented at 9:16 AM on June 24, 2021:

Done.
bench: bench.h fixes and improvements 10f4ce2078

bench: bench_bitcoin.cpp help fixups

- remove unneeded strprintf
- consistent punctuation (no EOL periods)
- sort helps by order they are printed (alphabetical order)

84e2d5b781

doc: update doc/benchmarking.md d8513fe411
jonatack force-pushed on Jun 24, 2021
jonatack commented at 9:17 AM on June 24, 2021: member

Thanks @fanquake, @jarolrod and @kiminuo for the reviews. Updated per feedback and fixed the last commit message (bencharking -> benchmarking).
theStack commented at 2:44 PM on June 24, 2021: member

Concept ACK, nice benchmark doc improvements! Planning to verify the changes more in detail within the next days.
theStack approved
theStack commented at 2:30 PM on June 27, 2021: member

ACK d8513fe41102dcbfc05235f3b95e33eb1878f880 🚤

Reviewed the documentation changes, ran ./src/bench/bench_bitcoin -filter="AddrManAdd|AddrManGetAddr|Base58CheckEncode|Base58Decode" to verify that the example output format in the last commit matches.
za-kk commented at 9:02 PM on July 4, 2021: contributor

ACK d8513fe41102dcbfc05235f3b95e33eb1878f880

Reviewed documentation and it's definitely an improvement to what was already there, also ran ./src/bench/bench_bitcoin -filter="AddrManAdd|AddrManGetAddr|Base58CheckEncode|Base58Decode" and verified format matches with documentation

Note - running ./src/bench/bench_bitcoin -? causes an error in zsh as ? is considered a wildcard, instead ./src/bench/bench_bitcoin "-?" must be run. Now that we are specifying -? in the docs instead of --help could cause confusion when using zsh (which is default on macos). But I'm not sure whether the intricacies of different shells should impact the documentation 🤷
fanquake merged this on Jul 5, 2021
fanquake closed this on Jul 5, 2021
jonatack deleted the branch on Jul 5, 2021
sidhujag referenced this in commit 9e48031257 on Jul 5, 2021
hebasto referenced this in commit 75b84b42cd on Aug 11, 2021
fanquake referenced this in commit 436cd98ffd on Aug 12, 2021
gwillen referenced this in commit 1dd57b5dc3 on Jun 1, 2022
DrahtBot locked this on Aug 16, 2022

Contributors

Labels

Linked (view graph)

#22284 p2p, refactor: performance improvements to ProtectEvictionCandidatesByRatio()