Drop CCheckQueue::nIdle #26776

1. 
   hebasto commented at 1:50 pm on December 30, 2022: member

   Every thread, being counted in CCheckQueue::nIdle, is counted in CCheckQueue::nTotal as well. That makes possible the sum (nTotal + nIdle + 1) exceeds the number of all threads available for a CCheckQueue instance, which is unreasonable.
2. Drop `CCheckQueue::nIdle`

   Every thread, being counted in `nIdle`, is counted in `nTotal` as well.
   That makes possible the sum (nTotal + nIdle + 1) exceeds the number of
   all threads available for `CCheckQueue` instance, which is unreasonable.

   20050dd2f6
3. 
   DrahtBot commented at 1:50 pm on December 30, 2022: contributor

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

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept NACK	sipa, ajtowns

   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:

   • #26762 (refactor: Make CCheckQueue RAII-styled by hebasto)
   • #26749 (refactor: Use move semanitcs in favour of custom swap functions by hebasto)

   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.

4. hebasto marked this as a draft on Dec 30, 2022
5. hebasto marked this as ready for review on Dec 30, 2022
6. 
   sipa commented at 10:14 pm on December 30, 2022: member

   NACK. That’s not the intent of the code.

   The change you’re proposing would immediately assign nearly all signature checks to one of the threads. The idea is that only roughly half of what remains gets assigned, so that the further checks can be assigned to whatever threads finish faster.

7. 
   hebasto commented at 10:03 am on December 31, 2022: member

   @sipa

   The change you’re proposing would immediately assign nearly all signature checks to one of the threads.

   No, it wouldn’t. The behavior, you’re pointing at, assumes nTotal==0, right? But after calling https://github.com/bitcoin/bitcoin/blob/65de8eeeca29e71378aa34602b287ab921b040e4/src/validation.cpp#L1890-L1893 all of the worker threads are awaiting for the m_worker_cv condition variable and the nTotal is equal the number of the worker threads.

8. 
   ajtowns commented at 6:58 am on January 3, 2023: contributor

   No, it wouldn’t. The behavior, you’re pointing at, assumes nTotal==0, right? But after calling

   No – if you have 1000 work units, and 100 total workers (all initially idle), then they’ll each initially take 5 work units, leaving 500 work units in the queue. (The first one will remove itself from the idle count, calculate 1000/(100+99+1)=5; the second will do likewise, calculating 995/(100+98+1)=5`, etc))

   Once each worker finishes its work, it will then take ~1/100th of the remaining work (5 units for the first worker, then 4.95 units for the next, rounded down to 4, etc)

   I think accounting for nIdle here makes sense to allow work to be assigned equally to threads initially; then assigning slightly more work to the threads that finish first also seems sensible. So Concept NACK from me too.

9. hebasto closed this on Jan 3, 2023

   ---

10. bitcoin locked this on Jan 3, 2024
11. 
    hebasto commented at 11:48 am on March 5, 2025: member

    Benchmarked in https://github.com/bitcoin-dev-tools/benchcoin/pull/140.

Contributors
hebasto DrahtBot sipa ajtowns