refactor: Clamp worker threads in ChainstateManager constructor #31313

pull TheCharlatan wants to merge 1 commits into bitcoin:master from TheCharlatan:chainstatemanagerArgs changing 5 files +7 −5
  1. TheCharlatan commented at 9:56 am on November 18, 2024: contributor

    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

  2. DrahtBot commented at 9:56 am on November 18, 2024: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31313.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK furszy, stickies-v, maflcko

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #31132 (validation: fetch block inputs on parallel threads ~17% faster IBD by andrewtoth)
    • #29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)

    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.

  3. DrahtBot added the label Refactoring on Nov 18, 2024
  4. in src/checkqueue.h:134 in 09c9ba846e outdated
    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);
    


    l0rinc commented at 10:03 am on November 18, 2024:
    Seems deliberate to not clamp here, could you explain the reason?

    maflcko commented at 10:05 am on November 18, 2024:
    nit: For new code it would be good to use LogInfo instead of the deprecated alias. Also, no need for the trailing newline.

    TheCharlatan commented at 10:29 am on November 18, 2024:
    We already control the batch size from the chainman, so seemed logical to me to control the number of threads too.

    l0rinc commented at 10:42 am on November 18, 2024:
    Do we expect every call-site to do the clamping themselves?

    TheCharlatan commented at 10:51 am on November 18, 2024:
    I don’t think this really matters, but seems consistent to control the arguments in the same place, just like with the batch size.
  5. in src/validation.h:82 in 09c9ba846e outdated
    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};
    


    l0rinc commented at 10:03 am on November 18, 2024:
    Do you happen to know how this threshold was derived and why it’s needed in the first place? If it’s because GetNumCores is considered only a hint, why not clamp that instead?

    TheCharlatan commented at 10:23 am on November 18, 2024:

    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.


    l0rinc commented at 10:40 am on November 18, 2024:
    Is script verification completely CPU bound? If so, this limit may still be reasonable for a few more years - but unlike in 2013, it’s not an unreasonably big value.
  6. maflcko approved
  7. maflcko commented at 10:05 am on November 18, 2024: member
    lgtm ACK 09c9ba846ec44ba903de2b32a1cb0f49c7f93cb0
  8. refactor: Clamp worker threads in ChainstateManager constructor
    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.
    8f85d36d68
  9. in src/node/chainstatemanager_args.cpp:60 in 09c9ba846e outdated
    59@@ -60,8 +60,7 @@ util::Result<void> ApplyArgsManOptions(const ArgsManager& args, ChainstateManage
    60         script_threads += GetNumCores();
    


    l0rinc commented at 10:07 am on November 18, 2024:
    Slightly unrelated, but this doesn’t actually return the number of cores, but number of concurrent threads
  10. in src/validation.cpp:6263 in 09c9ba846e outdated
    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)},
    


    l0rinc commented at 10:12 am on November 18, 2024:
    as mentioned in the other comment, could we push this into CCheckQueue instead to avoid having to repeat this for other such calls, e.g. https://github.com/bitcoin/bitcoin/pull/31132/files#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R6278
  11. TheCharlatan force-pushed on Nov 18, 2024
  12. TheCharlatan commented at 10:41 am on November 18, 2024: contributor

    Updated 09c9ba846ec44ba903de2b32a1cb0f49c7f93cb0 -> 8f85d36d68ab33ba237407a2ed16667eb149d61f (chainstatemanagerArgs_0 -> chainstatemanagerArgs_1, compare)

  13. in src/node/chainstatemanager_args.cpp:64 in 8f85d36d68
    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);
    


    furszy commented at 3:07 pm on November 18, 2024:

    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.
    

    TheCharlatan commented at 4:32 pm on November 18, 2024:

    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?


    furszy commented at 4:46 pm on November 18, 2024:
    Yeah, never mind. The subtraction happens before the clamping, not after as I wrote above. So the behavior remains the same. I guess I’m a bit sleepy today.. sorry.
  14. furszy commented at 6:44 pm on November 18, 2024: member
    Code ACK 8f85d36d68ab33ba237407a2ed16667eb149d61f
  15. DrahtBot requested review from maflcko on Nov 18, 2024
  16. in src/checkqueue.h:134 in 8f85d36d68
    130@@ -130,6 +131,7 @@ class CCheckQueue
    131     explicit CCheckQueue(unsigned int batch_size, int worker_threads_num)
    132         : nBatchSize(batch_size)
    133     {
    134+        LogInfo("Script verification uses %d additional threads", worker_threads_num);
    


    stickies-v commented at 11:48 am on November 22, 2024:
    nit: CCheckQueue is used for other things than ScriptCheck (in test code), but I don’t see a better place to put the log statement, and making it a non-refactor commit by e..g updating the wording to “Verification queue uses …” is probably not worth it either, so disregarding this nit might be the best way forward.

    TheCharlatan commented at 9:15 pm on November 22, 2024:
    Yeah, I thought of different approaches here too, happy to take an alternative here if somebody comes up with something. Otherwise we’ll handle this once we get more queues, or a unified thread pool.
  17. in src/node/chainstatemanager_args.h:14 in 8f85d36d68
     9@@ -10,8 +10,6 @@
    10 
    11 class ArgsManager;
    12 
    13-/** Maximum number of dedicated script-checking threads allowed */
    14-static constexpr int MAX_SCRIPTCHECK_THREADS{15};
    15 /** -par default (number of script-checking threads, 0 = auto) */
    16 static constexpr int DEFAULT_SCRIPTCHECK_THREADS{0};
    


    stickies-v commented at 2:27 pm on November 22, 2024:

    Related (but not required for this PR), the below patch removing default value duplication would ensure the default work_threads_num value doesn’t depend on argsmanager being used either:

     0diff --git a/src/init.cpp b/src/init.cpp
     1index 94a5a08463..61c6f060f1 100644
     2--- a/src/init.cpp
     3+++ b/src/init.cpp
     4@@ -32,6 +32,7 @@
     5 #include <interfaces/ipc.h>
     6 #include <interfaces/mining.h>
     7 #include <interfaces/node.h>
     8+#include <kernel/chainstatemanager_opts.h>
     9 #include <kernel/context.h>
    10 #include <key.h>
    11 #include <logging.h>
    12diff --git a/src/kernel/chainstatemanager_opts.h b/src/kernel/chainstatemanager_opts.h
    13index 1b605f3d55..95827c5d00 100644
    14--- a/src/kernel/chainstatemanager_opts.h
    15+++ b/src/kernel/chainstatemanager_opts.h
    16@@ -8,6 +8,7 @@
    17 #include <kernel/notifications_interface.h>
    18 
    19 #include <arith_uint256.h>
    20+#include <common/system.h>
    21 #include <dbwrapper.h>
    22 #include <script/sigcache.h>
    23 #include <txdb.h>
    24@@ -23,6 +24,8 @@ class ValidationSignals;
    25 
    26 static constexpr bool DEFAULT_CHECKPOINTS_ENABLED{true};
    27 static constexpr auto DEFAULT_MAX_TIP_AGE{24h};
    28+/** -par default (number of script-checking threads) */
    29+static const int DEFAULT_SCRIPTCHECK_THREADS{GetNumCores() - 1};
    30 
    31 namespace kernel {
    32 
    33@@ -48,7 +51,7 @@ struct ChainstateManagerOpts {
    34     Notifications& notifications;
    35     ValidationSignals* signals{nullptr};
    36     //! Number of script check worker threads. Zero means no parallel verification.
    37-    int worker_threads_num{0};
    38+    int worker_threads_num{DEFAULT_SCRIPTCHECK_THREADS};
    39     size_t script_execution_cache_bytes{DEFAULT_SCRIPT_EXECUTION_CACHE_BYTES};
    40     size_t signature_cache_bytes{DEFAULT_SIGNATURE_CACHE_BYTES};
    41 };
    42diff --git a/src/node/chainstatemanager_args.cpp b/src/node/chainstatemanager_args.cpp
    43index cdc8bdd43e..e4a2546b2b 100644
    44--- a/src/node/chainstatemanager_args.cpp
    45+++ b/src/node/chainstatemanager_args.cpp
    46@@ -53,14 +53,13 @@ util::Result<void> ApplyArgsManOptions(const ArgsManager& args, ChainstateManage
    47     ReadDatabaseArgs(args, opts.coins_db);
    48     ReadCoinsViewArgs(args, opts.coins_view);
    49 
    50-    int script_threads = args.GetIntArg("-par", DEFAULT_SCRIPTCHECK_THREADS);
    51-    if (script_threads <= 0) {
    52+    if (auto script_threads{args.GetIntArg("-par")}) {
    53         // -par=0 means autodetect (number of cores - 1 script threads)
    54         // -par=-n means "leave n cores free" (number of cores - n - 1 script threads)
    55-        script_threads += GetNumCores();
    56+        if (*script_threads <= 0) *script_threads += GetNumCores();
    57+        // Subtract 1 because the main thread counts towards the par threads.
    58+        opts.worker_threads_num = *script_threads - 1;
    59     }
    60-    // Subtract 1 because the main thread counts towards the par threads.
    61-    opts.worker_threads_num = script_threads - 1;
    62 
    63     if (auto max_size = args.GetIntArg("-maxsigcachesize")) {
    64         // 1. When supplied with a max_size of 0, both the signature cache and
    65diff --git a/src/node/chainstatemanager_args.h b/src/node/chainstatemanager_args.h
    66index af13aa8d3c..701515953e 100644
    67--- a/src/node/chainstatemanager_args.h
    68+++ b/src/node/chainstatemanager_args.h
    69@@ -10,9 +10,6 @@
    70 
    71 class ArgsManager;
    72 
    73-/** -par default (number of script-checking threads, 0 = auto) */
    74-static constexpr int DEFAULT_SCRIPTCHECK_THREADS{0};
    75-
    76 namespace node {
    77 [[nodiscard]] util::Result<void> ApplyArgsManOptions(const ArgsManager& args, ChainstateManager::Options& opts);
    78 } // namespace node
    

    TheCharlatan commented at 9:13 pm on November 22, 2024:
    I’m a bit apprehensive on moving system-related code into the options. It just makes it a bit more complicated to patch out system-related code if we ever choose to ship support for the kernel library on bare metal. It is also why moved system.h into common/ in the first place.

    stickies-v commented at 10:28 pm on November 25, 2024:
    Ah, I hadn’t considered that. I suppose there’s also no real harm in default values being different when accessed from different contexts. Can be marked as resolved.
  18. stickies-v approved
  19. stickies-v commented at 2:47 pm on November 22, 2024: contributor

    ACK 8f85d36d68ab33ba237407a2ed16667eb149d61f


    I noticed this issue while reviewing #30595, and started working on an alternative approach before @TheCharlatan made me aware of this PR. I thought it would be nice to introduce a new BoundedInt type, that avoids relying on input validation and instead forces the underlying value to be bound to a (compile time constant) certain range.

    However, there were much less places than I’d anticipated where this could be an immediate drop-in, because the exact bounds often depend on other runtime variables (including worker_threads_num after my comment here). So, I think the current approach in this PR is most suitable - and I’ll look into if introducing BoundedInt might be useful in other places in the code, in a (potential) separate PR.

  20. maflcko commented at 9:45 am on November 26, 2024: member

    ACK 8f85d36d68ab33ba237407a2ed16667eb149d61f 🛳

    Signature:

    0untrusted 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}"
    1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
    2trusted comment: ACK 8f85d36d68ab33ba237407a2ed16667eb149d61f 🛳
    3734xeHF9f3ity3igNXyAMAL6TkSeQwh210l3ePSperJeZRaGjWiwT2JXpcoFGGpGKEJeuhQNMdAHMP1/krz5CQ==
    

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-03 21:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me