Add local thread pool to CCheckQueue #18710

hebasto · 2020-04-20T14:51:38Z

This PR:

gets rid of boost::thread_group in the CCheckQueue class
allows thread safety annotation usage in the CCheckQueue class
is alternative to refactor: make checkqueue manage the threads by itself (also removed some boost dependencies) #14464 (Add local thread pool to CCheckQueue #18710 (comment), Add local thread pool to CCheckQueue #18710 (comment))

Also, with this PR (I hope) it could be easier to resurrect a bunch of brilliant ideas from #9938.

Related: #17307

maflcko · 2020-04-20T15:13:48Z

Concept ACK on getting rid of boost in consensus code. This pull looks similar to #14464?

maflcko · 2020-04-20T15:14:21Z

Also, with this PR (I hope) it could be easier to resurrect a bunch of brilliant ideas from #9938.

cc @JeremyRubin

hebasto · 2020-04-20T15:16:08Z

@MarcoFalke

Concept ACK on getting rid of boost in consensus code. This pull looks similar to #14464?

Thanks for pointing to #14464. I'll look into it.

DrahtBot · 2020-04-20T16:06:54Z

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

Conflicts

No conflicts as of last run.

hebasto · 2020-04-20T17:48:42Z

Updated 9764d81 -> f4cd37e (pr18710.01 -> pr18710.02, diff):

fixed linter issue with boost headers

JeremyRubin · 2020-04-21T06:27:07Z

Nice! From a simple code review PoV it looks good.

I remember there being a bunch of subtle nasties in the specifics of using std::threads v.s. boost threads around interrupt system and the details of how condition variables work. I can't remember what they all were though now. I think #14464 closed because of that...

But generally huge concept ack on anything that makes #9938 more likely to move forward! Note though that the cuckoocache sort of stole it's thunder -- a lot of the contention in the CheckQueue was around the sigcache locking and the cuckoocache made that stuff way faster so it's not obvious #9938 will show as impressive gains, and the trade offs for consensus code may be less worth it. Can discuss more via IRC; I'd love to get some of the improvements through in any case.

hebasto · 2020-04-21T17:08:41Z

@MarcoFalke

Concept ACK on getting rid of boost in consensus code. This pull looks similar to #14464?

The differences from #14464 are:

absence of Interrupt() function
an additional atomic m_destruction_in_progress that replaces the boost "interrupt" feature:

bitcoin/src/checkqueue.h

Lines 96 to 97 in f4cd37e

if (m_destruction_in_progress) return;

cond.wait(lock); // wait

and

bitcoin/src/checkqueue.h

Lines 172 to 173 in f4cd37e

m_destruction_in_progress = true;

m_worker_cv.notify_all();

hebasto · 2020-04-21T21:14:25Z

Updated f4cd37e -> b02bc25 (pr18710.02 -> pr18710.03, diff):

picked some ideas from refactor: make checkqueue manage the threads by itself (also removed some boost dependencies) #14464
reduced scope of the PR (some commits has been moved into the refactor: Make CCheckQueue RAII-styled #18731)
OP updated (now every commit passes unit tests)

hebasto · 2020-04-21T22:28:04Z

Benchmark results on my machine:

master (a998c51)

# Benchmark, evals, iterations, total, min, max, median
CCheckQueueSpeedPrevectorJob, 100, 1400, 96.3747, 0.000677139, 0.000695471, 0.000688339

this PR (b02bc25)

# Benchmark, evals, iterations, total, min, max, median
CCheckQueueSpeedPrevectorJob, 100, 1400, 77.4393, 0.000545271, 0.00057892, 0.000552496

hebasto · 2020-04-22T22:52:12Z

Rebased b02bc25 -> f16f7f7 (pr18710.03 -> pr18710.04) due to the conflict with #18575.

maflcko

Approach ACK f16f7f7 🌘

Show signature and timestamp

Signature:

Timestamp of file with hash fd6cc8b78bfb432d1fffbc1706dd13c7f3563a0d2caef6be9b45688ccd1b15ad -

src/checkqueue.h

src/test/checkqueue_tests.cpp

src/checkqueue.h

hebasto · 2020-04-24T13:25:44Z

Updated f16f7f7 -> 7a7e346 (pr18710.04 -> pr18710.05, diff):

addressed all of @MarcoFalke's comments

maflcko · 2020-04-24T13:51:21Z

In the last commit: Could replace the last occurrence of thread.hpp with #include <boost/thread/shared_mutex.hpp> and kill boost thread from the linter?

hebasto · 2020-04-24T14:36:20Z

Updated 7a7e346 -> 6173917 (pr18710.05 -> pr18710.06, diff):

addressed @MarcoFalke's comment:

In the last commit: Could replace the last occurrence of thread.hpp with #include <boost/thread/shared_mutex.hpp> and kill boost thread from the linter?

hebasto · 2020-04-24T16:50:44Z

@MarcoFalke ~~It seems there is a thread issue on windows: https://travis-ci.org/github/bitcoin/bitcoin/jobs/679063276 and https://bitcoinbuilds.org/?job=90df9b0f-d918-45a1-b27d-0ae6d4cec303 ...~~

dc8be12 refactor: remove boost::thread_group usage (fanquake) Pull request description: Post #18710, there isn't much left using `boost::thread_group`, so should just be able to replace it with the standard library. This also removes the last use of `boost::thread_interrupted`. After this change, last piece of Boost Thread we'd be using is `boost::shared_mutex`. See the commentary [here](#16684 (comment)) as to why it may be non-trivial to swap that for `std::shared_mutex` in the near future. Closes #17307 ACKs for top commit: laanwj: Code review re-ACK dc8be12 MarcoFalke: review ACK dc8be12 🔁 jonatack: Non-expert code review ACK dc8be12, also checked range-diff since last review and that local debug build is clean with gcc 10.2.1-6 on Debian Tree-SHA512: 5510e2d760cce824234207dc86b1551ca8f21cbf3a2ce753c0254a0d03ffd83c94e449aec202fb7bd76e6fc64df783a6b70a736b0add9ece3734bb9c8ce8fc2f

partial bitcoin#15842, merge bitcoin#15849, bitcoin#17342, bitcoin#18710: Add local thread pool to CCheckQueue

Backport of bitcoin/bitcoin#18710 PR message: This PR: gets rid of boost::thread_group in the CCheckQueue class allows thread safety annotation usage in the CCheckQueue class is alternative to #14464 (#18710 (comment), #18710 (comment)) Also, with this PR (I hope) it could be easier to resurrect a bunch of brilliant ideas from #9938. Related: #17307 Squashed commits: 0ef938685b5c079a6f5a98daf0e3865d718d817b refactor: Use member initializers in CCheckQueue 01511776acb0c7ec216dc9c8112531067763f1cb Add local thread pool to CCheckQueue dba30695fc42f45828db008e7e5b81cb2b5d8551 test: Use CCheckQueue local thread pool 6784ac471bb32b6bb8e2de60986f123eb4990706 bench: Use CCheckQueue local thread pool bb6fcc75d1ec94b733d1477c816351c50be5faf9 refactor: Drop boost::thread stuff in CCheckQueue Also in this commit: Slight modification to our custom CCheckQueue_32MB benchmark to use the new API. Signed-off-by: Calin Culianu <calin.culianu@gmail.com>

Summary: This is a backport of [[bitcoin/bitcoin#18710 | core#18710]] [1/4] bitcoin/bitcoin@0ef9386 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10985

Summary: This is a backport of [[bitcoin/bitcoin#18710 | [[bitcoin/bitcoin#18710 | core#18710]]]] [2/4] bitcoin/bitcoin@0151177 bitcoin/bitcoin@dba3069 Depends on D10985 Test Plan: With TSAN: `ninja && ninja check check-functional` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10986

Summary: This is a backport of [[bitcoin/bitcoin#18710 | core#18710]] [3/4] bitcoin/bitcoin@6784ac4 Depends on D10986 Test Plan: With TSAN: `ninja bitcoin-bench && TSAN_OPTIONS=suppressions=/home/pierre/dev/bitcoin-abc/test/sanitizer_suppressions/tsan src/bench/bitcoin-bench` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10987

Summary: This is a backport of [[bitcoin/bitcoin#18710 | core#18710]] [4/4] bitcoin/bitcoin@bb6fcc7 Depends on D10987 Test Plan: With TSAN: `ninja && ninja check check-functional` ``` cmake .. -GNinja -DCMAKE_BUILD_TYPE=Debug -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ ninja all check-all ``` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10988

Summary: > Post [[bitcoin/bitcoin#18710 | core#18710]], there isn't much left using boost::thread_group, so should just be able to replace it with the standard library. This also removes the last use of `boost::thread_interrupted`. > > After this change, last piece of Boost Thread we'd be using is `boost::shared_mutex`. See the commentary [[https://github.com/bitcoin/bitcoin/issues/16684#issuecomment-726214696|here]] as to why it may be non-trivial to swap that for `std::shared_mutex` in the near future. This is a backport of [[bitcoin/bitcoin#21016 | core#21016]] and [[bitcoin/bitcoin#22433 | core#22433]] Test Plan: With clang + debug and (separate run) TSAN `ninja && ninja check check-functional` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10992

maflcko added Refactoring Validation labels Apr 20, 2020

This was referenced Apr 20, 2020

Make g_chainman internal to validation #18698

Merged

Prevent consecutive ::cs_main locks #18639

Closed

coins: allow cache resize after init #18637

Merged

bench: Remove requirement that all benches use same testing setup #18575

Merged

hebasto force-pushed the 200419-thread-pool branch from 9764d81 to f4cd37e Compare April 20, 2020 17:45

hebasto force-pushed the 200419-thread-pool branch from f4cd37e to b02bc25 Compare April 21, 2020 21:08

hebasto mentioned this pull request Apr 22, 2020

refactor: Make CCheckQueue RAII-styled #18731

Closed

DrahtBot added the Needs rebase label Apr 22, 2020

hebasto force-pushed the 200419-thread-pool branch from b02bc25 to f16f7f7 Compare April 22, 2020 22:50

DrahtBot removed the Needs rebase label Apr 23, 2020

maflcko approved these changes Apr 23, 2020

View reviewed changes

maflcko added this to the 0.21.0 milestone Apr 24, 2020

hebasto force-pushed the 200419-thread-pool branch from f16f7f7 to 7a7e346 Compare April 24, 2020 13:21

hebasto force-pushed the 200419-thread-pool branch from 7a7e346 to 6173917 Compare April 24, 2020 14:34

fanquake mentioned this pull request Jan 27, 2021

refactor: remove boost::thread_group usage #21016

Merged

kwvg added a commit to kwvg/dash that referenced this pull request Jun 4, 2021

merge bitcoin#18710: Add local thread pool to CCheckQueue

15b2b65

kwvg added a commit to kwvg/dash that referenced this pull request Jun 5, 2021

merge bitcoin#18710: Add local thread pool to CCheckQueue

b0f9dfa

kwvg mentioned this pull request Jun 5, 2021

partial #15842, merge #15849, #17342, #18710: Add local thread pool to CCheckQueue dashpay/dash#4191

Merged

kwvg added a commit to kwvg/dash that referenced this pull request Jun 6, 2021

merge bitcoin#18710: Add local thread pool to CCheckQueue

0261427

kwvg added a commit to kwvg/dash that referenced this pull request Jun 6, 2021

merge bitcoin#18710: Add local thread pool to CCheckQueue

70109da

kwvg added a commit to kwvg/dash that referenced this pull request Jun 7, 2021

merge bitcoin#18710: Add local thread pool to CCheckQueue

25227a7

kwvg added a commit to kwvg/dash that referenced this pull request Jun 8, 2021

merge bitcoin#18710: Add local thread pool to CCheckQueue

a1271b1

kwvg added a commit to kwvg/dash that referenced this pull request Jun 8, 2021

merge bitcoin#18710: Add local thread pool to CCheckQueue

0bdeee6

kwvg added a commit to kwvg/dash that referenced this pull request Jun 9, 2021

merge bitcoin#18710: Add local thread pool to CCheckQueue

2759ad0

kwvg added a commit to kwvg/dash that referenced this pull request Jun 10, 2021

merge bitcoin#18710: Add local thread pool to CCheckQueue

3471350

kwvg added a commit to kwvg/dash that referenced this pull request Jun 11, 2021

merge bitcoin#18710: Add local thread pool to CCheckQueue

7a72b35

kwvg added a commit to kwvg/dash that referenced this pull request Jun 16, 2021

merge bitcoin#18710: Add local thread pool to CCheckQueue

d27e546

kwvg added a commit to kwvg/dash that referenced this pull request Jun 16, 2021

merge bitcoin#18710: Add local thread pool to CCheckQueue

972af32

kwvg added a commit to kwvg/dash that referenced this pull request Jun 16, 2021

merge bitcoin#18710: Add local thread pool to CCheckQueue

076356f

kwvg added a commit to kwvg/dash that referenced this pull request Jun 24, 2021

merge bitcoin#18710: Add local thread pool to CCheckQueue

78e9bd5

kwvg added a commit to kwvg/dash that referenced this pull request Jun 25, 2021

merge bitcoin#18710: Add local thread pool to CCheckQueue

c9d0d92

UdjinM6 added a commit to dashpay/dash that referenced this pull request Jun 26, 2021

Merge pull request #4191 from kittywhiskers/checkqueue

c5cc285

partial bitcoin#15842, merge bitcoin#15849, bitcoin#17342, bitcoin#18710: Add local thread pool to CCheckQueue

This was referenced Sep 8, 2021

Boost usage reduction DeFiCh/ain#714

Closed

C++17 and reduce Boost usage DeFiCh/ain#701

Closed

shaavan mentioned this pull request Mar 17, 2022

Suggestion for questions based on architecture of the Bitcoin core chaincodelabs/onboarding-to-bitcoin-core#9

Open

gades pushed a commit to cosanta/cosanta-core that referenced this pull request May 1, 2022

merge bitcoin#18710: Add local thread pool to CCheckQueue

7cd576f

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Add local thread pool to CCheckQueue #18710

Add local thread pool to CCheckQueue #18710

Uh oh!

Conversation

hebasto commented Apr 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Apr 20, 2020

Uh oh!

maflcko commented Apr 20, 2020

Uh oh!

hebasto commented Apr 20, 2020

Uh oh!

DrahtBot commented Apr 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

hebasto commented Apr 20, 2020

Uh oh!

JeremyRubin commented Apr 21, 2020

Uh oh!

hebasto commented Apr 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Apr 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Apr 21, 2020

Uh oh!

hebasto commented Apr 22, 2020

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hebasto commented Apr 24, 2020

Uh oh!

maflcko commented Apr 24, 2020

Uh oh!

hebasto commented Apr 24, 2020

Uh oh!

hebasto commented Apr 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

hebasto commented Apr 20, 2020 •

edited

Loading

DrahtBot commented Apr 20, 2020 •

edited

Loading

hebasto commented Apr 21, 2020 •

edited

Loading

hebasto commented Apr 21, 2020 •

edited

Loading

hebasto commented Apr 24, 2020 •

edited

Loading