bench: add fluent API for untimed setup steps in nanobench
#34208
Conversation
Running a few benchmarks (which will be migrated in the next commit to use the new setup method) several times to showcase the spread: ./build/bin/bench_bitcoin -filter='^(BnBExhaustion|AddrManAddThenGood|DeserializeBlockTest|DeserializeAndCheckBlockTest|CheckBlockTest|LoadExternalBlockFile|FindByte|WalletCreatePlain|WalletCreateEncrypted|WalletLoadingDescriptors)$' | ns/op | op/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 26,400,542.00 | 37.88 | 0.4% | 0.29 | `AddrManAddThenGood` | 189,075.00 | 5,288.91 | 0.4% | 0.01 | `BnBExhaustion` | ns/block | block/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 1,237,000.00 | 808.41 | 2.4% | 0.01 | `DeserializeAndCheckBlockTest` | 893,333.00 | 1,119.40 | 0.6% | 0.01 | `DeserializeBlockTest` | ns/op | op/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 31.62 | 31,622,370.70 | 0.2% | 0.01 | `FindByte` | 5,506,875.00 | 181.59 | 1.4% | 0.06 | `LoadExternalBlockFile` | 593,480,333.00 | 1.68 | 0.4% | 6.53 | `WalletCreateEncrypted` | 174,305,167.00 | 5.74 | 0.7% | 1.93 | `WalletCreatePlain` | 160,833,875.00 | 6.22 | 0.2% | 0.80 | `WalletLoadingDescriptors` | ns/op | op/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 26,005,125.00 | 38.45 | 1.3% | 0.29 | `AddrManAddThenGood` | 181,909.67 | 5,497.23 | 0.1% | 0.01 | `BnBExhaustion` | ns/block | block/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 1,223,000.00 | 817.66 | 2.8% | 0.01 | `DeserializeAndCheckBlockTest` | 892,917.00 | 1,119.92 | 0.7% | 0.01 | `DeserializeBlockTest` | ns/op | op/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 31.58 | 31,660,608.70 | 0.5% | 0.01 | `FindByte` | 5,612,750.00 | 178.17 | 1.1% | 0.06 | `LoadExternalBlockFile` | 594,012,250.00 | 1.68 | 0.2% | 6.53 | `WalletCreateEncrypted` | 174,668,334.00 | 5.73 | 0.8% | 1.92 | `WalletCreatePlain` | 158,494,375.00 | 6.31 | 0.3% | 0.79 | `WalletLoadingDescriptors` Note that the `Default is 1ms, so we are mostly relying ...` change is simply an update from latest nanobench master.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34208. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
./build/bin/bench_bitcoin -filter='^(BnBExhaustion|AddrManAddThenGood|DeserializeBlockTest|DeserializeAndCheckBlockTest|CheckBlockTest|LoadExternalBlockFile|FindByte|WalletCreatePlain|WalletCreateEncrypted|WalletLoadingDescriptors)$' | ns/op | op/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 15,088,500.00 | 66.28 | 0.2% | 0.17 | `AddrManAddThenGood` | 179,208.00 | 5,580.11 | 2.0% | 0.00 | `BnBExhaustion` | ns/block | block/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 318,166.00 | 3,143.01 | 3.5% | 0.00 | `CheckBlockTest` | 886,750.00 | 1,127.71 | 0.8% | 0.01 | `DeserializeBlockTest` | ns/op | op/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 42.00 | 23,809,523.81 | 2.4% | 0.00 | `FindByte` | 5,473,208.00 | 182.71 | 0.4% | 0.06 | `LoadExternalBlockFile` | 584,168,041.00 | 1.71 | 0.3% | 6.43 | `WalletCreateEncrypted` | 168,040,458.00 | 5.95 | 1.1% | 1.85 | `WalletCreatePlain` | 155,446,625.00 | 6.43 | 0.7% | 0.78 | `WalletLoadingDescriptors` | ns/op | op/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 14,894,917.00 | 67.14 | 0.3% | 0.16 | `AddrManAddThenGood` | 177,667.00 | 5,628.51 | 1.3% | 0.00 | `BnBExhaustion` | ns/block | block/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 313,791.00 | 3,186.83 | 3.8% | 0.00 | `CheckBlockTest` | 888,208.00 | 1,125.86 | 0.7% | 0.01 | `DeserializeBlockTest` | ns/op | op/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 41.00 | 24,390,243.90 | 2.4% | 0.00 | `FindByte` | 5,445,208.00 | 183.65 | 1.0% | 0.06 | `LoadExternalBlockFile` | 581,800,500.00 | 1.72 | 0.4% | 6.40 | `WalletCreateEncrypted` | 166,035,583.00 | 6.02 | 0.5% | 1.82 | `WalletCreatePlain` | 153,574,792.00 | 6.51 | 0.1% | 0.77 | `WalletLoadingDescriptors`
l0rinc
force-pushed
the
l0rinc/untimed-setup-nanobench
branch
from
3fff67a to
e9e3ea1
Compare
There was a problem hiding this comment.
ACK e9e3ea1
PR patches Vendored Code from Nanobench to include untimed setup phase.
Personally I like the fluent style way we can now setup a benchmark, after the patch. The patch looks solid and is backwards compatible. A downside (i see) of patching vendored code is that it now contains "untested" code. (Yes it's only benchmark code, so not on a critical path, I get it) I has tests upstream 👍
Have thought about how to "fix" this, maybe include a "test" benchmark that shows the code works (with and without the setup) but that does not proves a lot imho. Anyhow not blocking.
Thanks for the review, please see the tests in the upstream PR: martinus/nanobench@58350cf#diff-88160f647ce57661afe7d755fa70a5fa342a2b79d72d3511596878e69ed5cdc3 |
|
ACK e9e3ea1
|
4 participants
Context
As described in martinus/nanobench#130, we have a few benchmarks where we have to reset the state between runs, otherwise the repetitions will do something else than the first iteration.
Upstream
I have opened a PR to
nanobenchto introduce an untimed setup phase, see: martinus/nanobench#136Unfortunately the author has health issues and couldn't contribute to it in the past 2 years.
Tests
Tests were only added upstream, would be a bit awkward to wire in
nanobench.houtside the benchmarking setup:martinus/nanobench@58350cf#diff-88160f647ce57661afe7d755fa70a5fa342a2b79d72d3511596878e69ed5cdc3
Fix
I have moved the changes here as well and applied it to a few simple benchmarks as demonstration.
We can revert ones that are controversial and add other ones in follow-ups, this PR is mostly meant to add the
setupfeature.Benchmarks
Most benchmarks show a modest "speedup", others a "slowdown" - but it's only the effect of the setup that's not measured anymore - and a
runphase which does the same operation in each epoch iteration (wallet benchmark changes were reverted since for simplicity):