bench: add benchmark for wallet 'AvailableCoins' function. #25234

furszy · 2022-05-28T14:19:40Z

Rationale

AvailableCoins is part of several important flows for the wallet; from RPC commands that create transactions like fundrawtransaction, send, walletcreatefundedpsbt, get the available balance, list the available coins with listunspent etc. to GUI connected processes that perform the same or similar actions: tx creation, available balance calculation, present the spendable coins in the coin control dialog.

As we are improving this process in #24699, #25005 and there are more structural changes coming on the way. This benchmark aims to ensure us that, at least, there are no regressions (obviously performance improvements are great but, at least for me, this heads into the direction of having a base metric to compare future structural changes).

Implementation Notes

There are 5 new benchmarks, one per wallet supported output type (LEGACY, P2SH_SEGWIT, BECH32, BECH32M), plus a multi-output-type wallet benchmark which contains outputs from all the descriptor types.

The test, by default, fills-up the wallet with 1k transactions, 2k outputs. Mainly to not consume much time if the user just want to verify that no substantial regressions were introduced. But, my expectation for those who are focused on this process is to use a much higher number locally to really note the differences across commits.

DrahtBot · 2022-05-28T15:08:24Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	achow101, hernanmarino, aureleoules
Concept ACK	laanwj, pablomartin4btc

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

src/test/util/wallet.cpp

laanwj · 2022-06-01T20:59:02Z

Concept ACK on benchmarking these, however there have been recent complaints that the benchmarks are slow, and I've noticed these add more fairly slow to run benchmarks:

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|      359,740,298.60 |                2.78 |    0.0% |     19.79 | `WalletAvailableCoinsMulti`
|      289,759,212.20 |                3.45 |    0.0% |     15.93 | `WalletAvailableCoinsOnlyBech32`
|      559,973,432.00 |                1.79 |    0.0% |     30.80 | `WalletAvailableCoinsOnlyBech32M`
|      196,348,457.20 |                5.09 |    0.0% |     10.80 | `WalletAvailableCoinsOnlyLegacy`
|      304,247,935.40 |                3.29 |    0.0% |     16.73 | `WalletAvailableCoinsOnlyP2SH_SEGWIT`

E.g. a total of 94 seconds to the runtime of bench_bitcoin with default settings.

furszy · 2022-06-02T12:13:43Z

Thanks laanwj for the review!

It's interesting to see other results, locally this are my results:

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|      132,289,908.40 |                7.56 |    0.3% |      7.27 | `WalletAvailableCoinsMulti`
|      104,691,758.40 |                9.55 |    0.1% |      5.77 | `WalletAvailableCoinsOnlyBech32`
|      196,688,875.00 |                5.08 |    0.2% |     10.82 | `WalletAvailableCoinsOnlyBech32M`
|       70,386,791.80 |               14.21 |    0.2% |      3.88 | `WalletAvailableCoinsOnlyLegacy`
|      111,477,558.20 |                8.97 |    0.1% |      6.14 | `WalletAvailableCoinsOnlyP2SH_SEGWIT`

But let me do something, going to decrease the bench number of iterations from 5 to 2. It should decrease the total time by half.

furszy · 2022-06-02T12:24:22Z

Done, decreased the number of iterations to 2. And this are the results:

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|      129,406,083.50 |                7.73 |    0.0% |      2.85 | `WalletAvailableCoinsMulti`
|      105,704,646.00 |                9.46 |    0.2% |      2.33 | `WalletAvailableCoinsOnlyBech32`
|      206,642,667.00 |                4.84 |    0.6% |      4.55 | `WalletAvailableCoinsOnlyBech32M`
|       69,838,895.50 |               14.32 |    0.0% |      1.54 | `WalletAvailableCoinsOnlyLegacy`
|      112,263,500.00 |                8.91 |    0.0% |      2.47 | `WalletAvailableCoinsOnlyP2SH_SEGWIT`

Total time: ~15 seconds to run the 5 new benchmarks at most here now.
Previously, with 5 iterations, total time was ~40 seconds. So, you should see more or less a 40% total time decrease there as well.

furszy · 2022-06-02T12:44:39Z

Aside from that, I think that a good future question to ask ourselves would be whether we want to always run all the benchmarks all the time or not. Because, sooner or later, as the project isn't going to stop growing, we could end up sacrificing accuracy, implementing not entirely real scenarios (skipping parts of the flow that is being benchmarked) in favor of decreasing the total bench time (there is where my comment in #24924 (review) heads to for example).

But well, this is just me thinking out loud.. after #24924, the total bench time will be pretty decent to not have to worry about this for a while.

src/bench/wallet_available_coins.cpp

achow101 · 2022-06-02T18:46:25Z

src/bench/wallet_available_coins.cpp

+static void WalletAvailableCoinsOnlyBech32M(benchmark::Bench& bench) { WalletAvailableCoins(bench, {OutputType::BECH32M}); }
+static void WalletAvailableCoinsOnlyBech32(benchmark::Bench& bench) { WalletAvailableCoins(bench, {OutputType::BECH32}); }
+static void WalletAvailableCoinsOnlyP2SH_SEGWIT(benchmark::Bench& bench) { WalletAvailableCoins(bench, {OutputType::P2SH_SEGWIT}); }
+static void WalletAvailableCoinsOnlyLegacy(benchmark::Bench& bench) { WalletAvailableCoins(bench, {OutputType::LEGACY}); }
+static void WalletAvailableCoinsMulti(benchmark::Bench& bench) { WalletAvailableCoins(bench, {OutputType::LEGACY, OutputType::BECH32M, OutputType::BECH32, OutputType::P2SH_SEGWIT}); }
+
+BENCHMARK(WalletAvailableCoinsOnlyBech32M);
+BENCHMARK(WalletAvailableCoinsOnlyBech32);
+BENCHMARK(WalletAvailableCoinsOnlyP2SH_SEGWIT);
+BENCHMARK(WalletAvailableCoinsOnlyLegacy);
+BENCHMARK(WalletAvailableCoinsMulti);


In b791c2a "bench: add benchmark for wallet 'AvailableCoins' function."

I don't really think it is useful to have AvailableCoins run for different OutputTypes. There really isn't any difference for how AvailableCoins behaves depending on the OutputType.

Hmm, wouldn't we be having the same benchmark times for all the cases if that would be the case? (we are adding the same amount of txs and outputs to each of them).

I mean, your #24699 improves exactly this. The time difference there is for the solving provider and IsMine related calls.

Yes, it should be the same benchmark times for all cases, so it doesn't make sense to run it separately for each OutputType.

furszy · 2022-08-31T14:01:01Z

Note: haven't updated this PR because a similar benchmark is introduced in #25685.

So, once/if that one gets merged, will rebase this work on top. The amount of changes here will be pretty small.

hernanmarino

Approach ACK fd583c8. @furszy are you planning on rebasing this one, now that #25685 was merged ?

pablomartin4btc · 2022-11-08T14:53:40Z

Aside from that, I think that a good future question to ask ourselves would be whether we want to always run all the benchmarks all the time or not. Because, sooner or later, as the project isn't going to stop growing, we could end up sacrificing accuracy, implementing not entirely real scenarios...

I can see you also introduced that idea and the improvement is already merged and available; good stuff!

pablomartin4btc

Code review ACK.
I've verified most changes were already merged on #25685, waiting for your next commit to re ACK this PR again.

furszy · 2022-11-08T21:50:47Z

yeah, will rebase + rearrange it in the coming days. Thanks for the ping 👍🏼 .

src/bench/wallet_create_tx.cpp

achow101 · 2022-12-15T20:47:58Z

ACK 3a4f8bc

hernanmarino

ACK 3a4f8bc

hernanmarino · 2022-12-22T14:53:51Z

This is my output :

|               ns/op |                op/s |    err% |          ins/op |         bra/op |   miss% |     total | benchmark
|--------------------:|--------------------:|--------:|----------------:|---------------:|--------:|----------:|:----------
|       10,914,205.50 |               91.62 |    0.1% |  136,775,849.50 |  12,819,423.00 |    0.0% |      0.24 | `WalletAvailableCoins`

pablomartin4btc

tested ACK.

Here my results:

|--------------------:|--------------------:|--------:|----------:|:----------
|       20,730,690.00 |               48.24 |    3.8% |      0.47 | `WalletAvailableCoins`

From time to time I get recommendations to increase the iters as it warns unstability:

|--------------------:|--------------------:|--------:|----------:|:----------
|       20,150,356.50 |               49.63 |    5.3% |      0.48 | :wavy_dash: `WalletAvailableCoins` (Unstable with ~2.0 iters. Increase `minEpochIterations` to e.g. 20)

Not sure if this could be an issue.

aureleoules

ACK 3a4f8bc

aureleoules · 2022-12-28T14:52:56Z

src/bench/wallet_create_tx.cpp

 BENCHMARK(WalletCreateTxUseOnlyPresetInputs, benchmark::PriorityLevel::LOW)
 BENCHMARK(WalletCreateTxUsePresetInputsAndCoinSelection, benchmark::PriorityLevel::LOW)
+BENCHMARK(WalletAvailableCoins, benchmark::PriorityLevel::LOW);


nit: missing new line

… function. 3a4f8bc bench: add benchmark for wallet 'AvailableCoins' function. (furszy) Pull request description: #### Rationale `AvailableCoins` is part of several important flows for the wallet; from RPC commands that create transactions like `fundrawtransaction`, `send`, `walletcreatefundedpsbt`, get the available balance, list the available coins with `listunspent` etc. to GUI connected processes that perform the same or similar actions: tx creation, available balance calculation, present the spendable coins in the coin control dialog. As we are improving this process in bitcoin#24699, bitcoin#25005 and there are more structural changes coming on the way. This benchmark aims to ensure us that, at least, there are no regressions (obviously performance improvements are great but, at least for me, this heads into the direction of having a base metric to compare future structural changes). #### Implementation Notes There are 5 new benchmarks, one per wallet supported output type (LEGACY, P2SH_SEGWIT, BECH32, BECH32M), plus a multi-output-type wallet benchmark which contains outputs from all the descriptor types. The test, by default, fills-up the wallet with 1k transactions, 2k outputs. Mainly to not consume much time if the user just want to verify that no substantial regressions were introduced. But, my expectation for those who are focused on this process is to use a much higher number locally to really note the differences across commits. ACKs for top commit: achow101: ACK 3a4f8bc hernanmarino: ACK 3a4f8bc aureleoules: ACK 3a4f8bc Tree-SHA512: d0bb4c165f1efa181b47cb31561e6217eff9135bcd1b6761a7292f9018e456d13d18a1b886c2e2268d35c52f9e1fd8e0f252972424e5c5f00c280620b79c5a1b

DrahtBot added the Build system label May 28, 2022

DrahtBot mentioned this pull request May 28, 2022

refactor: introduce generic 'Result' class and connect it to CreateTransaction and GetNewDestination #25218

Merged

laanwj added Wallet Tests and removed Build system labels Jun 1, 2022

laanwj reviewed Jun 1, 2022

View reviewed changes

src/test/util/wallet.cpp Outdated Show resolved Hide resolved

furszy force-pushed the 2022_bench_wallet_available_coins branch from ca8ef4c to 4029293 Compare June 2, 2022 12:14

achow101 reviewed Jun 2, 2022

View reviewed changes

DrahtBot mentioned this pull request Jun 3, 2022

wallet: remove extra wtx lookup in 'AvailableCoins' + several code cleanups. #25005

Merged

furszy force-pushed the 2022_bench_wallet_available_coins branch from 4029293 to fd583c8 Compare June 26, 2022 22:37

DrahtBot added the Needs rebase label Jul 12, 2022

hernanmarino reviewed Nov 8, 2022

View reviewed changes

pablomartin4btc reviewed Nov 8, 2022

View reviewed changes

furszy force-pushed the 2022_bench_wallet_available_coins branch from fd583c8 to b2ec705 Compare November 14, 2022 13:55

DrahtBot removed the Needs rebase label Nov 14, 2022

achow101 reviewed Dec 15, 2022

View reviewed changes

src/bench/wallet_create_tx.cpp Outdated Show resolved Hide resolved

bench: add benchmark for wallet 'AvailableCoins' function.

3a4f8bc

furszy force-pushed the 2022_bench_wallet_available_coins branch from b2ec705 to 3a4f8bc Compare December 15, 2022 18:43

furszy requested review from pablomartin4btc and hernanmarino and removed request for pablomartin4btc December 19, 2022 13:05

furszy requested review from pablomartin4btc and removed request for hernanmarino December 19, 2022 13:06

hernanmarino reviewed Dec 22, 2022

View reviewed changes

hernanmarino approved these changes Dec 22, 2022

View reviewed changes

pablomartin4btc reviewed Dec 22, 2022

View reviewed changes

aureleoules approved these changes Dec 28, 2022

View reviewed changes

achow101 merged commit 139ba2b into bitcoin:master Jan 4, 2023

ajtowns mentioned this pull request Jan 12, 2023

ci: Use macos-ventura-xcode:14.1 image for "macOS native" task bitcoin-inquisition/bitcoin#14

Merged

furszy deleted the 2022_bench_wallet_available_coins branch May 27, 2023 01:50

bitcoin locked and limited conversation to collaborators May 26, 2024

bench: add benchmark for wallet 'AvailableCoins' function. #25234

bench: add benchmark for wallet 'AvailableCoins' function. #25234

Uh oh!

Conversation

furszy commented May 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale

Implementation Notes

Uh oh!

DrahtBot commented May 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

Uh oh!

laanwj commented Jun 1, 2022

Uh oh!

furszy commented Jun 2, 2022

Uh oh!

furszy commented Jun 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

furszy commented Jun 2, 2022

Uh oh!

Uh oh!

achow101 Jun 2, 2022

Choose a reason for hiding this comment

Uh oh!

furszy Jun 26, 2022

Choose a reason for hiding this comment

Uh oh!

achow101 Jul 6, 2022

Choose a reason for hiding this comment

Uh oh!

furszy commented Aug 31, 2022

Uh oh!

hernanmarino left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pablomartin4btc commented Nov 8, 2022

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

furszy commented Nov 8, 2022

Uh oh!

Uh oh!

achow101 commented Dec 15, 2022

Uh oh!

hernanmarino left a comment

Choose a reason for hiding this comment

Uh oh!

hernanmarino commented Dec 22, 2022

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

aureleoules left a comment

Choose a reason for hiding this comment

Uh oh!

aureleoules Dec 28, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

furszy commented May 28, 2022 •

edited

Loading

DrahtBot commented May 28, 2022 •

edited

Loading

furszy commented Jun 2, 2022 •

edited

Loading

hernanmarino left a comment •

edited

Loading