The globals setup by the function calls when creating a new kernel context only need to be setup once. Calling them multiple times may be wasteful and has no apparent benefit.
Besides kernel users potentially creating multiple contexts, this change may also be useful for tests creating multiple setups.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | stickies-v, tdb3, maflcko |
| Stale ACK | danielabrozzoni |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
18 | - LogPrintf("Using the '%s' SHA256 implementation\n", sha256_algo); 19 | - RandomInit(); 20 | + static bool initialized{false}; 21 | + if (!initialized) { 22 | + std::string sha256_algo = SHA256AutoDetect(); 23 | + LogPrintf("Using the '%s' SHA256 implementation\n", sha256_algo);
While this code is being touched, do we want to use more specific logging instead of LogPrintf? (e.g. LogDebug, LogInfo, etc.)
https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#logging
Aye, pushed.
Approach ACK
Updated de7853277625a4a483d157a9a85df9b51d07c9d5 -> 50c14e5a170950f30aa6b474bb231bbbfdd82427 (staticInitKernel_0 -> staticInitKernel_1, compare)
ACK 50c14e5a170950f30aa6b474bb231bbbfdd82427 Nice optimization. Ran unit/functional tests locally (passed).
tACK 50c14e5a170950f30aa6b474bb231bbbfdd82427
15 | Context::Context()
16 | {
17 | - std::string sha256_algo = SHA256AutoDetect();
18 | - LogPrintf("Using the '%s' SHA256 implementation\n", sha256_algo);
19 | - RandomInit();
20 | + static bool initialized{false};
nit: std::call_once would be appropriate here I think, but the current approach is clear too so not important
<details> <summary>git diff on 50c14e5a17</summary>
diff --git a/src/kernel/context.cpp b/src/kernel/context.cpp
index a7c3457c8c..c34dc84704 100644
--- a/src/kernel/context.cpp
+++ b/src/kernel/context.cpp
@@ -8,19 +8,18 @@
#include <logging.h>
#include <random.h>
+#include <mutex>
#include <string>
namespace kernel {
Context::Context()
{
- static bool initialized{false};
- if (!initialized) {
+ static std::once_flag is_initialized;
+ std::call_once(is_initialized, [](){
std::string sha256_algo = SHA256AutoDetect();
LogInfo("Using the '%s' SHA256 implementation\n", sha256_algo);
RandomInit();
- }
- initialized = true;
+ });
}
-
} // namespace kernel
</details>
nit: since the flag is about the globals, and not the context, being initialized - would globals_initialized be a better name? It doesn't make a difference now, but it will when the constructor does more than just setting up globals.
static bool globals_initialized{false};
ACK 50c14e5a170950f30aa6b474bb231bbbfdd82427 . No behaviour change for bitcoind and bitcoin-qt, only affects the tests (and downstream).
The globals setup by the function calls when creating a new kernel
context only need to be setup once. Calling them multiple times may be
wasteful and has no apparent benefit.
Besides kernel users potentially creating multiple contexts, this change
may also be useful for tests creating multiple setups.
Co-authored-by: stickies-v <stickies-v@protonmail.com>
Thanks for the suggestions @stickies-v,
50c14e5a170950f30aa6b474bb231bbbfdd82427 -> 93fb0e7897000072ea790a91816aea876718ad27 (staticInitKernel_1 -> staticInitKernel_2, compare)
std::call_once instead of static boolean flag. This makes the behavior consistent on the off chance that a context is created concurrently.initialized to globals_initialized.
re-ACK 93fb0e7897000072ea790a91816aea876718ad27
re ACK 93fb0e7897000072ea790a91816aea876718ad27
Concept ACK. The context constructor isn't thread-safe. This can be tested with tsan and:
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index abe3d6a505..05d00eaf97 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -62,6 +62,7 @@
#include <algorithm>
#include <functional>
+#include <future>
#include <stdexcept>
using kernel::BlockTreeDB;
@@ -195,6 +196,13 @@ BasicTestingSetup::BasicTestingSetup(const ChainType chainType, TestOpts opts)
AppInitParameterInteraction(*m_node.args);
LogInstance().StartLogging();
m_node.warnings = std::make_unique<node::Warnings>();
+ {
+ auto fun{[] { kernel::Context{}; }};
+ auto futs{std::array{
+ std::async(std::launch::async, fun),
+ std::async(std::launch::async, fun),
+ }};
+ }
m_node.kernel = std::make_unique<kernel::Context>();
m_node.ecc_context = std::make_unique<ECC_Context>();
SetupEnvironment();
Which should print a data-race. So it seems fine to guard the code and only execute it once.
ACK 93fb0e7897000072ea790a91816aea876718ad27 👝
<details><summary>Show signature</summary>
Signature:
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 93fb0e7897000072ea790a91816aea876718ad27 👝
qDwtQZZCJDX/9k96k2fh3KtTVl9a3ZmElpWNyBB9S0OOk47oYfmKnOTgbM0NiJD/TKvFbXiAMTKqGYgiLK/LAQ==
</details>
Post-merge ACK 93fb0e7897000072ea790a91816aea876718ad27.
