bench: update logging benchmarks #26957
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. 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. |
d2f7abd to
9d708db
Compare
There was a problem hiding this comment.
Concept ACK
The addition of the new macros makes sense as well as the new names for the benchmarks.
Just one observation: although I initially was sceptical about the {No,Yo} naming scheme, looking at the updated {With,Without} it seems that is now harder to skim through the list.
9d708db to
a0e092e
Compare
|
Thank you @kouloumos and @LarryRuane for the excellent reviewing. Updated per |
There was a problem hiding this comment.
ACK a0e092e
$ ./src/bench/bench_bitcoin -filter='LogP*.*'
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 2,716.93 | 368,062.78 | 0.5% | 0.01 | `LogPrintLevelWithThreadNames`
| 2,691.57 | 371,529.69 | 0.4% | 0.01 | `LogPrintLevelWithoutThreadNames`
| 2,657.64 | 376,273.25 | 0.3% | 0.01 | `LogPrintWithCategory`
| 4.15 | 241,089,995.32 | 0.3% | 0.01 | `LogPrintWithoutCategory`
| 2,705.19 | 369,659.92 | 0.2% | 0.01 | `LogPrintfCategoryWithThreadNames`
| 2,651.54 | 377,138.94 | 0.1% | 0.01 | `LogPrintfCategoryWithoutThreadNames`
| 2,649.92 | 377,370.03 | 0.3% | 0.01 | `LogPrintfWithThreadNames`
| 2,613.43 | 382,638.59 | 0.3% | 0.01 | `LogPrintfWithoutThreadNames`
| 48.00 | 20,832,403.39 | 0.3% | 0.01 | `LogWithoutWriteToFile`
I believe these are the (rough) results we expect. If you retouch, consider adding a comment at the top of the file probably, stating the expected results -- that is, that all but two of the tests should have similar performance, but that LogPrintWithoutCategory should be about 3 orders of magnitude faster than most of the tests, and LogWithoutWriteToFile should be about 2 orders of magnitude faster than most of the tests. Maybe even explain briefly why -- for example, LogWithoutWriteToFile is still logging somewhere (?) but not to a file, which is why it's faster, while LogPrintWithoutCategory should be almost a no-op. Something like that, just so when people run this benchmark, they'll know roughly what's expected without having to dig in and really understand the code.
The global logging object instance is not re-created for each run, so when multiple logging benchmarks are run, each one after the first one still has the logging categories enabled from the previous ones. This commit disables all categories at the start of each benchmark.
to better track which benchmark corresponds to which log macro.
for these new macros that our logging is planned to migrate to. At some point it may be feasible to drop some of the previous logging benchmarks.
Good idea @LarryRuane (thanks!). Done. |
and clarify the intention behind the -nodebuglogfile bench. Co-authored-by: "kouloumos <kouloumosa@gmail.com>" Co-authored-by: "Larry Ruane <larryruane@gmail.com>"
a0e092e to
8c47d59
Compare
|
ACK 8c47d59 |
|
code review & tested ACK 8c47d59, here are my benchmark results:
By the way, here is a nice way to create & share detailed benchmark results, see the results here: https://share.firefox.dev/3Z02boN How to do this:
|
|
Here's a suggestion about benchmarking in general, wonder what you think. The idea is a kind of performance regression testing. If there are a set of related benchmarks, as there are here, there may be some expected performance relationship between them. (Above I suggested that Jon add a comment describing this relationship, and he did.) It would be cool if this could somehow be specified in the code and checked automatically, and a failure raised if the runtime ratio between the sub-benchmarks exceeded some threshold. So for example, if a bug was introduced that made What do you think? (Obviously not for this PR, but as a separate PR.) |
|
@LarryRuane Perhaps reviving https://bitcoinperf.com/ to track changes in benchmarks over time, and alert when it observes a deviation threshold reached, could be a way to implement it. Maybe it could be hooked into the CI at some point. |
|
ACK 8c47d59 |
8c47d59 doc: improve -debuglogfile help to be a bit clearer (jonatack) 20d89d6 bench: document expected results in logging benchmarks (jonatack) d8deba8 bench: add LogPrintfCategory and LogPrintLevel benchmarks (Jon Atack) 102b203 bench: order the logging benchmark code by output (Jon Atack) 4b3fdbf bench: update logging benchmark naming for clarity (Jon Atack) 4684aa8 bench: allow logging benchmarks to be order-independent (Larry Ruane) Pull request description: Update our logging benchmarks for evaluating ongoing work like bitcoin#25203 and refactoring proposals like bitcoin#26619 and bitcoin#26697. - make the logging benchmarks order-independent (Larry Ruane) - add missing benchmarks for the `LogPrintLevel` and `LogPrintfCategory` macros that our logging is migrating to; at some later point it should be feasible to drop some of the previous logging benchmarks - update the logging benchmark naming to be clear which benchmark corresponds to which log macro, and update the ordering to be the same as the output - add clarifying documentation to the logging benchmarks - improve the `-debuglogfile` config option help to be clearer; can be tested by running `./src/bitcoind -help | grep -A4 '\-debuglogfile'` Reviewers can run the logging benchmarks with: ```bash ./src/bench/bench_bitcoin -filter='LogP*.*' ``` ACKs for top commit: LarryRuane: ACK 8c47d59 martinus: code review & tested ACK 8c47d59, here are my benchmark results: achow101: ACK 8c47d59 Tree-SHA512: 705f8720c9ceaf14a1945039c7578a0c17a12215cbc44908099af4ac444561c3f95d833c5a91b325cdd4470737d8a01e2da64db2d542dd7c9a3747fbfdbf213e
Update our logging benchmarks for evaluating ongoing work like #25203 and refactoring proposals like #26619 and #26697.
LogPrintLevelandLogPrintfCategorymacros that our logging is migrating to; at some later point it should be feasible to drop some of the previous logging benchmarks-debuglogfileconfig option help to be clearer; can be tested by running./src/bitcoind -help | grep -A4 '\-debuglogfile'Reviewers can run the logging benchmarks with:
./src/bench/bench_bitcoin -filter='LogP*.*'