@fanquake Is this good enough or do you want faster? -unsafesqlitesync does not appear to have any effect. The next thing would be to reduce the number of txs created, but I'm concerned that doing so will make it harder to observe actual performance issues that this benchmark is intended to measure.

review ACK 9e404a9

On my machine:

Before PR #24913

Executed in   57.34 secs    fish           external
   usr time   57.60 secs  843.00 micros   57.60 secs
   sys time    1.43 secs    0.00 micros    1.43 secs

After PR #24913

|    1,525,136,424.90 |                0.66 |    0.2% |    189.67 | `WalletLoadingDescriptors`
|      873,902,969.45 |                1.14 |    0.3% |    106.20 | `WalletLoadingLegacy`

________________________________________________________
Executed in  564.01 secs    fish           external
   usr time  540.03 secs  427.00 micros  540.02 secs
   sys time   13.35 secs   95.00 micros   13.35 secs

This PR

|    1,740,638,493.00 |                0.57 |    0.6% |     26.13 | `WalletLoadingDescriptors`
|    1,016,725,382.00 |                0.98 |    0.9% |     12.72 | `WalletLoadingLegacy`

________________________________________________________
Executed in  348.35 secs    fish           external
   usr time  328.66 secs    1.05 millis  328.66 secs
   sys time   12.46 secs    0.60 millis   12.46 secs

There's a good improvement (348.35 secs) over PR #24913 (540.03 secs), but it still takes a lot longer than before it (57.34 secs).

@fanquake Is this good enough or do you want faster?

I think to be good enough, it needs to run in a similar time to all of the other benchmarks, i.e max ~1-2 seconds, otherwise anyone running make check on macOS is going to be opening issues asking why running make check (has suddenly started) taking 10+ minutes. I tested this branch, but stopped after WalletLoadingDescriptors took 12 minutes:

|           80,237.38 |           12,463.02 |    0.1% |      0.01 | `VerifyScriptBench`
|            8,190.91 |          122,086.52 |    1.2% |      0.01 | `WalletBalanceClean`
|          179,780.20 |            5,562.35 |    1.5% |      0.01 | `WalletBalanceDirty`
|            7,929.42 |          126,112.61 |    0.1% |      0.01 | `WalletBalanceMine`
|               42.47 |       23,546,683.62 |    0.0% |      0.01 | `WalletBalanceWatch`
|      902,283,977.00 |                1.11 |    1.4% |    105.48 | `WalletLoadingDescriptors`
^C
src/bench/bench_bitcoin  59.66s user 24.31s system 11% cpu 12:01.03 total

-unsafesqlitesync does not appear to have any effect. The next thing would be to reduce the number of txs created, but I'm concerned that doing so will make it harder to observe actual performance issues that this benchmark is intended to measure.

Given that, it seems like we've got another macOS only-performance issue, that we should probably figure out the root cause of?

I think to be good enough, it needs to run in a similar time to all of the other benchmarks, i.e max ~1-2 seconds,

Agree. It's unacceptable to have a benchmark take in the order of minutes. Remember that benchmarks are there to measure small timespans (such as inner loop functions), not very long processes.

I've also seen a CI timeout due to this. Remember that we do bench/bench_bitcoin &> /dev/null in one of the CI runs.

I've added the following changes:

• The number of benchmark epochs is reduced from the default 11 to 5.
• Use unsafesqlitesync
• Use mock databases
  • Update sqlite's mock database implementation to use in-memory dbs rather than just temp dbs
• Instead of mining blocks to wallet addresses, we make transactions and add them directly with AddToWallet.
• Reduced the number of txs added to the wallet from 5000 to 1000.

This still doesn't make it as fast as the other benchmarks though.

Before #24913:

________________________________________________________
Executed in   11.37 secs    fish           external
   usr time   11.36 secs    0.00 micros   11.36 secs
   sys time    7.62 secs  435.00 micros    7.62 secs

After #24913:

________________________________________________________
Executed in  150.77 secs    fish           external
   usr time  148.99 secs  334.00 micros  148.99 secs
   sys time    9.42 secs   88.00 micros    9.42 secs

This PR:

________________________________________________________
Executed in   20.25 secs    fish           external
   usr time   20.21 secs  319.00 micros   20.21 secs
   sys time    7.92 secs  158.00 micros    7.92 secs

This PR makes ./src/bench/bench_bitcoin -filter=WalletLoadingDescriptors go from taking 246s to 2.3s for me, making "make check" a lot more usable.

(Ubuntu 22.04 on a Ryzen 5950X system; my ZFS filesystem setup may make flushing/syncing relatively slower)

Looks like this also introduces memory corruption, looking at the CI output?

Haven't reviewed the code at all, but e673d8b reduces the runtime of the WalletLoadingDescriptors test from 4m25s to 0.1s for me

tACK e673d8b

This solves the make check issue with bench_bitcoin reported here

if I (manually) revert commits 817c051 (using unsafesqlitesync)

I think -unsafesqlitesync has more of an effect on macs than linux?

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #25466 (ci: add unused-using-decls to clang-tidy by fanquake)

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.