-
Notifications
You must be signed in to change notification settings - Fork 35.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bench: enable wallet creation benchmarks on all platforms #30122
bench: enable wallet creation benchmarks on all platforms #30122
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. 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. ConflictsNo 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: Run src\bench_bitcoin.exe -sanity-check
Running with -sanity-check option, output is being suppressed as benchmark results will be useless.
Error: Process completed with exit code 1. |
815f502
to
fe5cdbf
Compare
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. |
fe5cdbf
to
7323bf6
Compare
Concept ACK. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 7323bf6.
Maybe deduplicate code:
--- a/src/bench/wallet_create.cpp
+++ b/src/bench/wallet_create.cpp
@@ -35,8 +35,8 @@ static void WalletCreate(benchmark::Bench& bench, bool encrypted)
std::vector<bilingual_str> warnings;
int round = 0;
- fs::path wallet_path = test_setup->m_path_root / strprintf("test_wallet%d_%d", round, random.rand32()).c_str();
bench.run([&] {
+ auto wallet_path = test_setup->m_path_root / strprintf("test_wallet%d_%d", round++, random.rand32()).c_str();
auto wallet = CreateWallet(context, wallet_path.utf8string(), /*load_on_start=*/std::nullopt, options, status, error_string, warnings);
assert(status == DatabaseStatus::SUCCESS);
assert(wallet != nullptr);
@@ -44,7 +44,6 @@ static void WalletCreate(benchmark::Bench& bench, bool encrypted)
// Unload wallet and update wallet directory for the next round
RemoveWallet(context, wallet, /*load_on_start=*/ std::nullopt);
UnloadWallet(std::move(wallet));
- wallet_path = test_setup->m_path_root / strprintf("test_wallet%d_%d", ++round, random.rand32()).c_str();
});
}
?
7323bf6
to
b6b2d82
Compare
Applied, thanks @hebasto 👍🏼. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-ACK b6b2d82.
b6b2d82
to
7e30fda
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 7e30fda
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>
7e30fda
to
7c8abf3
Compare
utACK 7c8abf3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 7c8abf3 |
Simple fix for #29816.
Since the wallet is appended to the global
WalletContext
duringcreation, merely calling
reset()
on the benchmark shared_pointeris 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.