bench: Make WalletLoading benchmark run faster

achow101 commented at 5:01 pm on April 19, 2022: member

minEpochIterations is probably unnecessary to set, so removing it makes the runtime much faster.

bench: Remove minEpochIterations from wallet loading benchmark

This is probably unnecessary and just makes it slower.

9e404a9831

achow101 commented at 5:02 pm on April 19, 2022: member

@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.

MarcoFalke commented at 5:19 pm on April 19, 2022: member

review ACK 9e404a98312d73c969adf4f8e87aad1ac4b3029d

MarcoFalke assigned fanquake on Apr 19, 2022

DrahtBot added the label Tests on Apr 19, 2022

w0xlt commented at 8:40 pm on April 19, 2022: contributor

On my machine:

Before PR #24913

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

After PR #24913

0|    1,525,136,424.90 |                0.66 |    0.2% |    189.67 | `WalletLoadingDescriptors`
1|      873,902,969.45 |                1.14 |    0.3% |    106.20 | `WalletLoadingLegacy`
2
3________________________________________________________
4Executed in  564.01 secs    fish           external
5   usr time  540.03 secs  427.00 micros  540.02 secs
6   sys time   13.35 secs   95.00 micros   13.35 secs

This PR

0|    1,740,638,493.00 |                0.57 |    0.6% |     26.13 | `WalletLoadingDescriptors`
1|    1,016,725,382.00 |                0.98 |    0.9% |     12.72 | `WalletLoadingLegacy`
2
3________________________________________________________
4Executed in  348.35 secs    fish           external
5   usr time  328.66 secs    1.05 millis  328.66 secs
6   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 commented at 8:55 am on April 20, 2022: member

@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:

0|           80,237.38 |           12,463.02 |    0.1% |      0.01 | `VerifyScriptBench`
1|            8,190.91 |          122,086.52 |    1.2% |      0.01 | `WalletBalanceClean`
2|          179,780.20 |            5,562.35 |    1.5% |      0.01 | `WalletBalanceDirty`
3|            7,929.42 |          126,112.61 |    0.1% |      0.01 | `WalletBalanceMine`
4|               42.47 |       23,546,683.62 |    0.0% |      0.01 | `WalletBalanceWatch`
5|      902,283,977.00 |                1.11 |    1.4% |    105.48 | `WalletLoadingDescriptors`
6^C
7src/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?

laanwj commented at 5:16 pm on April 20, 2022: member

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.

bench: use unsafesqlitesync in wallet loading benchmark 817c051364

bench: reduce number of epochs for wallet loading benchmark d94244c4bf

bench: Add transactions directly instead of mining blocks f85b54ed27

bench: reduce the number of txs in wallet for wallet loading bench 7c0d34476d

achow101 commented at 6:18 pm on April 20, 2022: member

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:

0________________________________________________________
1Executed in   11.37 secs    fish           external
2   usr time   11.36 secs    0.00 micros   11.36 secs
3   sys time    7.62 secs  435.00 micros    7.62 secs

After #24913:

0________________________________________________________
1Executed in  150.77 secs    fish           external
2   usr time  148.99 secs  334.00 micros  148.99 secs
3   sys time    9.42 secs   88.00 micros    9.42 secs

This PR:

0________________________________________________________
1Executed in   20.25 secs    fish           external
2   usr time   20.21 secs  319.00 micros   20.21 secs
3   sys time    7.92 secs  158.00 micros    7.92 secs

sipa commented at 3:47 pm on May 10, 2022: member

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)

MarcoFalke commented at 3:50 pm on May 10, 2022: member

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

walletdb: Create a mock database of specific type

We may want to make a mock database of either SQLite or BDB, not just
whatever the compiled default is.

a1080802f8

sqlite: Use in-memory db instead of temp for mockdb

The mock db can be in-memory rather than just at temp file.

49910f255f

bench: Use mock wallet database for wallet loading benchmark

Using in-memory only databases speeds up the benchmark, at the cost of
real world accuracy.

4af3547eba

bench: Enable loading benchmarks depending on what's compiled

Add descriptor wallet benchmark only if sqlite is compiled. Add legacy
wallet benchmark only if bdb is compiled.

e673d8b475

achow101 force-pushed on May 10, 2022

ajtowns commented at 4:05 pm on May 10, 2022: member

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

theStack commented at 9:52 pm on May 17, 2022: member

Concept ACK

Didn’t look much at the code yet, but ran the benchmarks for both legacy and descriptor wallet on my (rather slow) system after each commit and master via

0$ time ./src/bench/bench_bitcoin -filter=WalletLoadingLegacy
1$ time ./src/bench/bench_bitcoin -filter=WalletLoadingDescriptors

with the following results:

	Legacy	Descriptors
master	3m18.39s	5m40.95s
Remove minEpochIterations (9e404a98312d73c969adf4f8e87aad1ac4b3029d)	1m54.15s	2m51.50s
use unsafesqlitesync (817c051364208d3f9e7e2af5700bd2bee5c9f303)	1m53.68s	2m46.76s
reduce number of epochs (d94244c4bf37365272a16eb2ce6517605b4c8a47)	1m49.12s	2m38.29s
add txs directly instead of mining blocks (f85b54ed27bd6eddb1e7035db02d542575b3ab24)	1m27.61s	1m51.24s
reduce number of txs in wallet (7c0d34476df446e3825198b27c6f62bba4c0b974)	0m19.83s	0m27.06s
create mock db (a1080802f8d7c3d1251ec6f2be33031f568deafa)	0m19.56s	0m26.18s
use in-memory db for mock db (49910f255f77e14fccf189353d188efac00d1445)	0m19.36s	0m26.03s
use mock db (4af3547ebac672a2d516e8696fd3580a766c27eb)	0m09.16s	0m07.25s
load bench depending on what’s compiled (e673d8b475995075b696208386c9e45ae7ca3e20)	0m09.51s	0m07.19s

Marked the commits in bold where the most gains (at least 2x) were observed. Looking closer at the descriptor wallet benchmark, if I (manually) revert commits 817c051364208d3f9e7e2af5700bd2bee5c9f303 (using unsafesqlitesync) and 49910f255f77e14fccf189353d188efac00d1445 (use in-memory mode for mock db) on the top commit via the patch

 0diff --git a/src/bench/wallet_loading.cpp b/src/bench/wallet_loading.cpp
 1index f61138378..994811c79 100644
 2--- a/src/bench/wallet_loading.cpp
 3+++ b/src/bench/wallet_loading.cpp
 4@@ -85,7 +85,6 @@ static std::unique_ptr<WalletDatabase> DuplicateMockDatabase(WalletDatabase& dat
 5 static void WalletLoading(benchmark::Bench& bench, bool legacy_wallet)
 6 {
 7     const auto test_setup = MakeNoLogFileContext<TestingSetup>();
 8-    test_setup->m_args.ForceSetArg("-unsafesqlitesync", "1");
 9
10     WalletContext context;
11     context.args = &test_setup->m_args;
12diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp
13index 72d483e61..26908b377 100644
14--- a/src/wallet/walletdb.cpp
15+++ b/src/wallet/walletdb.cpp
16@@ -1203,7 +1203,7 @@ std::unique_ptr<WalletDatabase> CreateMockWalletDatabase(DatabaseOptions& option
17
18     if (format == DatabaseFormat::SQLITE) {
19 #ifdef USE_SQLITE
20-        return std::make_unique<SQLiteDatabase>(":memory:", "", options, true);
21+        return std::make_unique<SQLiteDatabase>("", "", options, true);
22 #endif
23         assert(false);
24     }

and got a run-time of 0m07.41s. From that result, I’d conclude that commits 817c051364208d3f9e7e2af5700bd2bee5c9f303 and 49910f255f77e14fccf189353d188efac00d1445 barely have any effect on the run-time and could in theory be removed from this PR (no hard feelings though).

Rspigler commented at 5:20 am on May 22, 2022: contributor

tACK e673d8b475995075b696208386c9e45ae7ca3e20

This solves the make check issue with bench_bitcoin reported here

in src/bench/wallet_loading.cpp:120 in 4af3547eba outdated

118     bench.epochs(5).run([&] {
119-        wallet = BenchLoadWallet(context, options);
120+        wallet = BenchLoadWallet(std::move(database), context, options);
121 
122         // Cleanup
123+        database = DuplicateMockDatabase(wallet->GetDatabase(), options);

furszy commented at 3:10 pm on May 25, 2022:

In 4af3547e:

I’m not so sure why do we need to update the database here, guess that what you wanted to do is rollback the db to the first mocked database created at line 103 and not the recently loaded one? So in case of any modification, we ensure that the benchmark is always running from the same static db input?

achow101 commented at 4:52 pm on May 26, 2022:

Actually it’s because deleting the wallet so that we can load it again will delete the database.

furszy commented at 1:12 pm on May 27, 2022:

k, that is because of the db destructor, good.

furszy commented at 3:38 pm on May 25, 2022: member

Code review ACK e673d8b4, nice PR.

With a note: I think that we could keep generating a chain instead of adding the dummy txs directly, so the benchmark is more accurate to what happens in a real wallet (otherwise we are skipping some flows during the LoadToWallet(tx) process like the tx initial state update that requires a chain lookup).

What do you think about generating the chain manually instead of using the whole generatetoaddress flow, so we skip the entire BlockAssembler::CreateNewBlock, follow up TestBlockValidity, finding the correct nonce etc.. that are surely pretty slow.

As the wallet doesn’t care about chain validity, we directly could: create, concatenate and append blocks into the chain index manually, which should be pretty fast, so the wallet can use them during the load flow.

achow101 commented at 4:53 pm on May 26, 2022: member

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

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

in src/wallet/walletdb.cpp:1214 in a1080802f8 outdated

1213+    }
1214+
1215+#ifdef USE_BDB
1216     return std::make_unique<BerkeleyDatabase>(std::make_shared<BerkeleyEnvironment>(), "", options);
1217 #endif
1218+    assert(false);

glozow commented at 9:26 am on June 22, 2022:

In a1080802f8d7c3d1251ec6f2be33031f568deafa “walletdb: Create a mock database of specific type “: I hit this line in testing with both USE_SQLITE and USE_BDB off? I may have just messed something up, but does this need e673d8b475995075b696208386c9e45ae7ca3e20 “Enable loading benchmarks depending on what’s compiled " squashed in?

glozow commented at 9:52 am on June 22, 2022: member

Concept ACK e673d8b475995075b696208386c9e45ae7ca3e20. For each commit, verified that there was a performance improvement without negating the purpose of the bench, and made some effort to verify that the code is correct.

Would be nice to get this merged. I haven’t used make check without quitting at bench in months :(

Nit: could you update the OP to include the list of things you’ve changed to make it run faster? It currently only mentions minEpochIterations.

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

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

Fwiw, I don’t know exactly what -unsafesqlitesync does, but tried dropping the commit on my mac and ubuntu machines. It seems like neither had any change (specifically on my machines though, not sure about others).

DrahtBot commented at 6:32 am on June 25, 2022: member

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.

fanquake merged this on Jun 28, 2022

fanquake closed this on Jun 28, 2022

sidhujag referenced this in commit a25ce51692 on Jun 29, 2022

DrahtBot locked this on Jun 28, 2023

bench: Make WalletLoading benchmark run faster #24924

Conflicts