Simple fix for #29816.
Since the wallet is appended to the global WalletContext during
creation, merely calling reset() on the benchmark shared_pointer
is insufficient to destruct the wallet. This no destruction of the
wallet object results in keeping the db connection open, which
was causes the fs::remove_all() failure on Windows.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | maflcko, hebasto, kevkevinpal |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
No conflicts as of last run.
The native Windows CI (https://github.com/bitcoin/bitcoin/actions/runs/9112695910/job/25052498545?pr=30122#step:24:64) is failing with:
0Run src\bench_bitcoin.exe -sanity-check
1Running with -sanity-check option, output is being suppressed as benchmark results will be useless.
2Error: Process completed with exit code 1.
It’d be good if the commit message could explain why this is being done. It seems like the current approach is just to try and refactor away the Windows bug, even though it might not have been properly identified (prior to #29816 (comment)). However now that the bug might have been found, it’d be good to at least document what it is, and if possible, fix/workaround it in a more targetted way, rather than refactor code that is currently perfectly fine for all other platforms.
Sure with a small nuance. This code is working fine for all other platforms but that does not mean the code is perfect. When I wrote this benchmark, I was probably more concerned about the number of files the benchmark could open in parallel than the delays/inaccuracies introduced by the, OS dependent, directory removal line. Essentially, we shouldn’t be removing a directory within the benchmarked code scope. But yeah, np on splitting this in two well described commits.
ACK 7323bf6a2ecc3a2f1e2fdebce53b961767b06e08.
Maybe deduplicate code:
0--- a/src/bench/wallet_create.cpp
1+++ b/src/bench/wallet_create.cpp
2@@ -35,8 +35,8 @@ static void WalletCreate(benchmark::Bench& bench, bool encrypted)
3 std::vector<bilingual_str> warnings;
4
5 int round = 0;
6- fs::path wallet_path = test_setup->m_path_root / strprintf("test_wallet%d_%d", round, random.rand32()).c_str();
7 bench.run([&] {
8+ auto wallet_path = test_setup->m_path_root / strprintf("test_wallet%d_%d", round++, random.rand32()).c_str();
9 auto wallet = CreateWallet(context, wallet_path.utf8string(), /*load_on_start=*/std::nullopt, options, status, error_string, warnings);
10 assert(status == DatabaseStatus::SUCCESS);
11 assert(wallet != nullptr);
12@@ -44,7 +44,6 @@ static void WalletCreate(benchmark::Bench& bench, bool encrypted)
13 // Unload wallet and update wallet directory for the next round
14 RemoveWallet(context, wallet, /*load_on_start=*/ std::nullopt);
15 UnloadWallet(std::move(wallet));
16- wallet_path = test_setup->m_path_root / strprintf("test_wallet%d_%d", ++round, random.rand32()).c_str();
17 });
18 }
19
?
33@@ -34,16 +34,16 @@ static void WalletCreate(benchmark::Bench& bench, bool encrypted)
34 bilingual_str error_string;
35 std::vector<bilingual_str> warnings;
36
37- fs::path wallet_path = test_setup->m_path_root / strprintf("test_wallet_%d", random.rand32()).c_str();
38+ int round = 0;
39 bench.run([&] {
40+ auto wallet_path = test_setup->m_path_root / strprintf("test_wallet%d_%d", round++, random.rand32()).c_str();
random needed, when using a counter?
44
45- // Cleanup
46+ // Release wallet
47 RemoveWallet(context, wallet, /*load_on_start=*/ std::nullopt);
48 UnloadWallet(std::move(wallet));
49- fs::remove_all(wallet_path);
is it required to remove this remove_all call? I read the commit message:
This approach will provide more accurate benchmark results, regardless of any delays the OS-dependent directory removal call could have.
However, there is also a call to RemoveWallet, and UnloadWallet in this WalletCreate benchmark. The additional overhead of the remove_all should be fine? The benefit would be that the number of folders is kept constant and does not pile up as the bench runs, which could (in theory?) lead to issues when running for a long time?
However, there is also a call to RemoveWallet, and UnloadWallet in this WalletCreate benchmark. The additional overhead of the remove_all should be fine? The benefit would be that the number of folders is kept constant and does not pile up as the bench runs, which could (in theory?) lead to issues when running for a long time?
hmm, I don’t think they are comparable. The overhead of remove_all depend on external factors such us the OS and the hard drive. But agree that running this for a long time isn’t good neither. Seems that the best solution would be to create a mechanism to cleanup state in-between benchmark rounds in the framework.
Will drop the second commit for now, np.
33@@ -34,29 +34,25 @@ static void WalletCreate(benchmark::Bench& bench, bool encrypted)
34 bilingual_str error_string;
35 std::vector<bilingual_str> warnings;
36
37- fs::path wallet_path = test_setup->m_path_root / strprintf("test_wallet_%d", random.rand32()).c_str();
38+ int round = 0;
39 bench.run([&] {
40+ auto wallet_path = test_setup->m_path_root / strprintf("test_wallet%d_%d", round++, random.rand32()).c_str();
PathFromString, because the c_str interface requires ascii-encoding, and ascii-encoded bytes are unchanged in unicode (via PathFromString).
33@@ -34,29 +34,25 @@ static void WalletCreate(benchmark::Bench& bench, bool encrypted)
34 bilingual_str error_string;
35 std::vector<bilingual_str> warnings;
36
37- fs::path wallet_path = test_setup->m_path_root / strprintf("test_wallet_%d", random.rand32()).c_str();
38+ auto wallet_path = fs::PathToString(test_setup->m_path_root / fs::PathFromString(strprintf("test_wallet")));
0 auto wallet_path = fs::PathToString(test_setup->m_path_root / "test_wallet");
nit: For pure ascii literals, it is not needed
Since the wallet is appended to the global WalletContext during
creation, merely calling 'reset()' on the benchmark shared_pointer
is insufficient to destruct the wallet. This no destruction of the
wallet results in the db connection remaining open, which was the
cause of the 'fs::remove_all()' error in Windows.
Co-Authored-By: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
