This ensures the options are applied consistently from contexts where they might not pass through the args manager, such as in some tests, or when used through the kernel library.
This is similar to the patch applied in 09ef322acc0a88a9e119f74923399598984c68f6, used to make applying the mempool options consistent.
This is part of the libbitcoinkernel project #27587
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31313.
See 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.
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.
130@@ -130,6 +131,7 @@ class CCheckQueue
131 explicit CCheckQueue(unsigned int batch_size, int worker_threads_num)
132 : nBatchSize(batch_size)
133 {
134+ LogPrintf("Script verification uses %d additional threads\n", worker_threads_num);
LogInfo instead of the deprecated alias. Also, no need for the trailing newline.
77@@ -78,6 +78,9 @@ static constexpr int DEFAULT_CHECKLEVEL{3};
78 // Setting the target to >= 550 MiB will make it likely we can respect the target.
79 static const uint64_t MIN_DISK_SPACE_FOR_BLOCK_FILES = 550 * 1024 * 1024;
80
81+/** Maximum number of dedicated script-checking threads allowed */
82+static constexpr int MAX_SCRIPTCHECK_THREADS{15};
GetNumCores is considered only a hint, why not clamp that instead?
Looking at the original PR there seems to be little justification for it. It was also submitted in 2013, where machines with more than 16 cores were not common.
This current PR is just a refactor, if this should be changed, a separate PR should be opened.
This ensures the options are applied consistently from contexts where
they might not pass through the args manager, such as in some tests, or
when used through the kernel library.
This is similar to the patch applied in 09ef322acc0a88a9e119f74923399598984c68f6.
59@@ -60,8 +60,7 @@ util::Result<void> ApplyArgsManOptions(const ArgsManager& args, ChainstateManage
60 script_threads += GetNumCores();
6259@@ -6260,7 +6260,7 @@ static ChainstateManager::Options&& Flatten(ChainstateManager::Options&& opts)
6260 }
6261
6262 ChainstateManager::ChainstateManager(const util::SignalInterrupt& interrupt, Options options, node::BlockManager::Options blockman_options)
6263- : m_script_check_queue{/*batch_size=*/128, options.worker_threads_num},
6264+ : m_script_check_queue{/*batch_size=*/128, std::clamp(options.worker_threads_num, 0, MAX_SCRIPTCHECK_THREADS)},
CCheckQueue instead to avoid having to repeat this for other such calls, e.g. https://github.com/bitcoin/bitcoin/pull/31132/files#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R6278
Updated 09c9ba846ec44ba903de2b32a1cb0f49c7f93cb0 -> 8f85d36d68ab33ba237407a2ed16667eb149d61f (chainstatemanagerArgs_0 -> chainstatemanagerArgs_1, compare)
59@@ -60,8 +60,7 @@ util::Result<void> ApplyArgsManOptions(const ArgsManager& args, ChainstateManage
60 script_threads += GetNumCores();
61 }
62 // Subtract 1 because the main thread counts towards the par threads.
63- opts.worker_threads_num = std::clamp(script_threads - 1, 0, MAX_SCRIPTCHECK_THREADS);
64- LogPrintf("Script verification uses %d additional threads\n", opts.worker_threads_num);
The max number of workers threads before was 15 and now seems to be 14. E.g. provide -par=16:
Previous Code:
0script_threads = 16.
1worker_threads_num = std::clamp(script_threads-1, 0, 15);
2...
3Outcome: 15 workers
Current Code
0script_threads = std::clamp(16, 0, 15); -> output 15
1worker_threads_num = script_threads - 1;
2..
3Outcome: 14 workers.
I’m confused, this is printing Script verification uses 15 additional threads for both master and this commit when I provide -par=16. As far as I can tell the logic as applied is:
0worker_threads_num = script_threads - 1;
1worker_threads_num = std::clamp(worker_threads_num, 0 , 15);
Which should be equivalent?