bench: add fluent API for untimed setup steps in nanobench #34208

pull l0rinc wants to merge 2 commits into bitcoin:master from l0rinc:l0rinc/untimed-setup-nanobench changing 6 files +101 −59
  1. l0rinc commented at 11:45 am on January 6, 2026: contributor

    Context

    As described in https://github.com/martinus/nanobench/issues/130, we have a few benchmarks where we have to reset the state between runs; otherwise, the repetitions will do something different than the first iteration.

    Upstream

    I have opened a PR to nanobench to introduce an untimed setup phase, see: https://github.com/martinus/nanobench/pull/136

    Tests

    Tests were only added upstream. It would be a bit awkward to wire them into nanobench.h outside the benchmarking setup: https://github.com/martinus/nanobench/pull/136/changes/58350cfe5912d7e0533ccfcd759eea5d62eb55bb#diff-88160f647ce57661afe7d755fa70a5fa342a2b79d72d3511596878e69ed5cdc3

    Fix

    I have moved the changes here as well and applied them to a few simple benchmarks as a demonstration. We can revert the ones that are controversial and add others in follow-ups. This PR is mostly meant to add the setup feature.

    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 run phase that does the same operation in each epoch iteration (wallet benchmark changes were reverted for simplicity):

  2. DrahtBot added the label Tests on Jan 6, 2026
  3. DrahtBot commented at 11:45 am on January 6, 2026: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34208.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK sipa, sedited, furszy, davidgumberg
    Stale ACK bensig, janb84

    If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #33160 (bench: Add more realistic Coin Selection Bench by murchandamus)
    • #32729 (test,refactor: extract script template helpers & widen sigop count coverage by l0rinc)
    • #32554 (bench: replace embedded raw block with configurable block generator by l0rinc)
    • #31868 ([IBD] specialize block serialization by l0rinc)
    • #31682 ([IBD] specialize CheckBlock’s input & coinbase checks by l0rinc)

    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.

    LLM Linter (✨ experimental)

    Possible typos and grammar issues:

    • positioned to the start of the file -> positioned at the start of the file [preposition “to” is incorrect here; “positioned at” correctly describes location]

    2026-03-08 12:03:01

  4. DrahtBot added the label CI failed on Jan 6, 2026
  5. DrahtBot commented at 1:02 pm on January 6, 2026: contributor

    🚧 At least one of the CI tasks failed. Task tidy: https://github.com/bitcoin/bitcoin/actions/runs/20747321605/job/59567659454 LLM reason (✨ experimental): clang-tidy failed the build due to a use-after-move error in wallet_loading.cpp, causing the CI to fail.

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  6. l0rinc force-pushed on Jan 6, 2026
  7. janb84 commented at 2:21 pm on January 6, 2026: contributor

    ACK e9e3ea1341caeab87aef8cb25dc0b58f7c3b27c8

    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.

  8. l0rinc commented at 2:26 pm on January 6, 2026: contributor

    vendored code is that it now contains “untested” code

    Thanks for the review, please see the tests in the upstream PR: https://github.com/martinus/nanobench/pull/136/changes/58350cfe5912d7e0533ccfcd759eea5d62eb55bb#diff-88160f647ce57661afe7d755fa70a5fa342a2b79d72d3511596878e69ed5cdc3

  9. DrahtBot removed the label CI failed on Jan 6, 2026
  10. bensig commented at 9:54 pm on January 8, 2026: contributor

    ACK e9e3ea1341caeab87aef8cb25dc0b58f7c3b27c8

    • Tested -
  11. DrahtBot added the label Needs rebase on Jan 14, 2026
  12. l0rinc force-pushed on Jan 15, 2026
  13. l0rinc commented at 12:50 pm on January 15, 2026: contributor
    Rebased after the benchmark priority cleanup. Ready for review again.
  14. DrahtBot removed the label Needs rebase on Jan 15, 2026
  15. DrahtBot added the label Needs rebase on Jan 21, 2026
  16. l0rinc force-pushed on Jan 21, 2026
  17. DrahtBot removed the label Needs rebase on Jan 21, 2026
  18. DrahtBot added the label CI failed on Jan 22, 2026
  19. DrahtBot commented at 0:25 am on January 22, 2026: contributor

    🚧 At least one of the CI tasks failed. Task tidy: https://github.com/bitcoin/bitcoin/actions/runs/21228258011/job/61081107788 LLM reason (✨ experimental): CI failed due to a clang-tidy error (unused return value in coin_selection.cpp).

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  20. l0rinc force-pushed on Jan 22, 2026
  21. DrahtBot removed the label CI failed on Jan 22, 2026
  22. janb84 commented at 12:40 pm on January 23, 2026: contributor

    re ACK 1f4c9b6f093522cdbdd55e9f99b239106e6d6daa

    Removing the setup time from the actual benchmark timing will result in a more representative benchmark, still think this is an improvement over benchmarking with setup steps included.

  23. sipa commented at 9:18 pm on January 27, 2026: member
    Concept ACK. This would help with a number of benchmarks I’ve written before.
  24. sedited commented at 8:46 am on March 8, 2026: contributor

    Concept ACK

    Can you also add the description for the changes in the first commit from the upstream PR?

  25. bench: add fluent API for untimed setup steps in `nanobench`
    Some benchmarks need per-epoch state reset so every measured run does the same work.
    Add `Bench::setup(...).run(...)` for untimed per-epoch setup.
    The existing `run()` now delegates to `runImpl()` with an empty setup lambda, keeping the old API unchanged.
    
    This vendors the upstream change from `martinus/nanobench`.
    The upstream PR also adds tests that verify setup is excluded from timing and runs once before each epoch's iterations.
    Those tests are not copied here because wiring them into `src/bench/nanobench.h` outside the benchmarking setup would be awkward.
    
    The `Default is 1ms, so we are mostly relying ...` comment update matches current upstream `nanobench` master.
    
    -------
    
    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`
    83b8528ddb
  26. refactor: improve benchmark setup and execution for various tests
    Note that `make_hard_case` already clears the UTXO pool in `coin_selection.cpp`.
    
    ./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`
    8825051e08
  27. l0rinc force-pushed on Mar 8, 2026
  28. l0rinc commented at 12:05 pm on March 8, 2026: contributor

    Added in the latest rebase: updated the first commit message with the upstream description of the setup(...).run(...) change, noted that run() now delegates to runImpl() with an empty setup lambda, and mentioned that the corresponding tests were only added upstream. No code changes, just commit message updates.

    0B=1f4c9b6f093522cdbdd55e9f99b239106e6d6daa A=8825051e0845e036ca284717806468193d4501b4 && git fetch origin $B $A && git range-diff --creation-factor=95 $B...$A
    
  29. l0rinc requested review from janb84 on Mar 8, 2026
  30. furszy commented at 5:07 pm on March 8, 2026: member
    Concept ACK. I thought way too many times about adding something like this. Even suggested it recently #31774 (review).
  31. davidgumberg commented at 11:38 pm on March 10, 2026: contributor
    Concept ACK, have wanted this numerous times e.g. #31774.

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-03-11 06:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me