Add local thread pool to CCheckQueue by hebasto · Pull Request #18710 · bitcoin/bitcoin

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 ...~~

jamesob · 2020-12-11T05:12:46Z

Concept ACK, will plan to review.

LarryRuane

nit: remove the #include <boost/thread/thread.hpp> from checkqueue_tests.cpp.

Looks good. I ran the affected tests (and single-stepped through most of the new test code), and verified using debugger that a mainnet node correctly reaches the real script checking code.
ACK bb6fcc7

hebasto · 2020-12-16T01:21:08Z

@LarryRuane

nit: remove the #include <boost/thread/thread.hpp> from checkqueue_tests.cpp.

boost::thread_group is still used in the test_CheckQueueControl_Locks.

LarryRuane · 2020-12-16T02:08:13Z

boost::thread_group is still used in the test_CheckQueueControl_Locks

It builds for me without that include. Oh, I see, it's included indirectly via test/util/setup_common.h, thanks.

laanwj · 2021-01-07T17:51:38Z

Thanks for getting rid of the one-to-last use of boost::thread_group.

Code review ACK bb6fcc7

jonatack

Code review ACK bb6fcc7 and verified rebase to master builds cleanly with unit/functional tests green

bb6fcc7 refactor: Drop boost::thread stuff in CCheckQueue (Hennadii Stepanov) 6784ac4 bench: Use CCheckQueue local thread pool (Hennadii Stepanov) dba3069 test: Use CCheckQueue local thread pool (Hennadii Stepanov) 0151177 Add local thread pool to CCheckQueue (Hennadii Stepanov) 0ef9386 refactor: Use member initializers in CCheckQueue (Hennadii Stepanov) Pull request description: This PR: - gets rid of `boost::thread_group` in the `CCheckQueue` class - allows thread safety annotation usage in the `CCheckQueue` class - is alternative to bitcoin#14464 (bitcoin#18710 (comment), bitcoin#18710 (comment)) Also, with this PR (I hope) it could be easier to resurrect a bunch of brilliant ideas from bitcoin#9938. Related: bitcoin#17307 ACKs for top commit: laanwj: Code review ACK bb6fcc7 LarryRuane: ACK bb6fcc7 jonatack: Code review ACK bb6fcc7 and verified rebase to master builds cleanly with unit/functional tests green Tree-SHA512: fddeb720d5a391b48bb4c6fa58ed34ccc3f57862fdb8e641745c021841c8340e35c5126338271446cbd98f40bd5484f27926aa6c3e76fa478ba1efafe72e73c1

After the merge of bitcoin#18710, the linter is warning: ```bash A new Boost dependency in the form of "boost/thread/mutex.hpp" appears to have been introduced: src/sync.cpp:#include <boost/thread/mutex.hpp> src/test/sync_tests.cpp:#include <boost/thread/mutex.hpp> ^---- failure generated from test/lint/lint-includes.sh ``` the interim bitcoin#19337 was merged, which introduced more `boost::mutex` usage. Given we no longer use `boost::mutex`, just remove the double lock test and remaining includes.

f827e15 refactor: remove straggling boost::mutex usage (fanquake) Pull request description: After the merge of #18710, the linter is warning: ```bash A new Boost dependency in the form of "boost/thread/mutex.hpp" appears to have been introduced: src/sync.cpp:#include <boost/thread/mutex.hpp> src/test/sync_tests.cpp:#include <boost/thread/mutex.hpp> ^---- failure generated from test/lint/lint-includes.sh ``` #18710 removed `boost/thread/mutex.hpp` from lint-includes, however in the interim #19337 was merged, which introduced more `boost::mutex` usage. Given we no longer use `boost::mutex`, just remove the double lock test and remaining includes. ACKs for top commit: laanwj: Code review ACK f827e15 hebasto: ACK f827e15 Tree-SHA512: f738b12189fe5b39db3e8f8231e9002714413a962eaf98adc84a6614fa474df5616358cfb1c89b92a2b0564efa9b704a774c49d4a25dca18a0ccc3cd9eabfc0a

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

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

LarryRuane reviewed Dec 16, 2020

View reviewed changes

jonatack reviewed Jan 11, 2021

View reviewed changes

jonatack mentioned this pull request Jan 11, 2021

Lock-Free CheckQueue #9938

Closed

laanwj merged commit b386d37 into bitcoin:master Jan 25, 2021

hebasto deleted the 200419-thread-pool branch January 25, 2021 21:31

fanquake mentioned this pull request Jan 26, 2021

refactor: remove straggling boost::mutex usage #21010

Merged

fanquake mentioned this pull request Jan 27, 2021

refactor: remove boost::thread_group usage #21016

Merged

kwvg mentioned this pull request Jun 5, 2021

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

Merged

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

Closed

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!

jamesob commented Dec 11, 2020

Uh oh!

LarryRuane left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Dec 16, 2020

Uh oh!

LarryRuane commented Dec 16, 2020

Uh oh!

laanwj commented Jan 7, 2021

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

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