This PR:
boost::thread_group in the CCheckQueue classCCheckQueue classAlso, with this PR (I hope) it could be easier to resurrect a bunch of brilliant ideas from #9938.
Related: #17307
Also, with this PR (I hope) it could be easier to resurrect a bunch of brilliant ideas from #9938.
cc @JeremyRubin
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
No conflicts as of last run.
Updated 9764d81e1f000d1bd3c03ac6f6f4361c4aa84ee9 -> f4cd37e93a522d5ab2bc872d129a0142be773a7d (pr18710.01 -> pr18710.02, diff):
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.
Concept ACK on getting rid of boost in consensus code. This pull looks similar to #14464?
The differences from #14464 are:
Interrupt() functionm_destruction_in_progress that replaces the boost “interrupt” feature: https://github.com/bitcoin/bitcoin/blob/f4cd37e93a522d5ab2bc872d129a0142be773a7d/src/checkqueue.h#L96-L97 and https://github.com/bitcoin/bitcoin/blob/f4cd37e93a522d5ab2bc872d129a0142be773a7d/src/checkqueue.h#L172-L173
Updated f4cd37e93a522d5ab2bc872d129a0142be773a7d -> b02bc2583e421dbca35a5652c650ae861230f480 (pr18710.02 -> pr18710.03, diff):
Benchmark results on my machine:
0# Benchmark, evals, iterations, total, min, max, median
1CCheckQueueSpeedPrevectorJob, 100, 1400, 96.3747, 0.000677139, 0.000695471, 0.000688339
0# Benchmark, evals, iterations, total, min, max, median
1CCheckQueueSpeedPrevectorJob, 100, 1400, 77.4393, 0.000545271, 0.00057892, 0.000552496
144+ {
145+ if (threads_num <= 0) return;
146+ assert(m_worker_threads.empty());
147+ m_stopped = false;
148+ m_worker_threads.reserve(threads_num);
149+ for (auto n = 0; n < threads_num; ++n) {
0 for (int n = 0; n < threads_num; ++n) {
is shorter and more precise
143+ void StartWorkerThreads(const int threads_num)
144+ {
145+ if (threads_num <= 0) return;
146+ assert(m_worker_threads.empty());
147+ m_stopped = false;
148+ m_worker_threads.reserve(threads_num);
138@@ -132,16 +139,31 @@ class CCheckQueue
139 {
140 }
141
142+ //! Create a pool of new worker threads.
143+ void StartWorkerThreads(const int threads_num)
144+ {
145+ if (threads_num <= 0) return;
m_stopped = false;
100@@ -95,6 +101,7 @@ class CCheckQueue
101 return fRet;
102 }
103 nIdle++;
104+ if (m_stopped) return true; // TODO: This return value is unused.
if (m_stopped) be after the condition variable wait? Otherwise notifying it will first empty the queue.
Finally, m_stopped could be renamed to m_request_stop and only briefly set in StopWorkerThreads to true and then immediately after to false. No other member functions (like StartWorkerThreads) touch it
Otherwise notifying it will first empty the queue.
Correct. This is done intentionally as I think that worker threads should exit with a clean check queue, no?
Also, shouldn’t the
if (m_stopped)be after the condition variablewait?
This wouldn’t work for the master thread that could never return.
Finally,
m_stoppedcould be renamed tom_request_stopand only briefly set inStopWorkerThreadsto true and then immediately after to false.
How this approach could work when StopWorkerThreads() is called during check queue being processed? I think m_stopped should keep its state in that case.
8@@ -9,7 +9,7 @@
9 #include <util/time.h>
10
11 #include <boost/test/unit_test.hpp>
12-#include <boost/thread.hpp>
13+#include <boost/thread/thread.hpp>
In that case in the commit “bench: Use CCheckQueue local thread pool” the lint-includes.sh linter will produce:
0Good job! The Boost dependency "boost/thread/thread.hpp" is no longer used.
1Please remove it from EXPECTED_BOOST_INCLUDES in ./test/lint/lint-includes.sh
2to make sure this dependency is not accidentally reintroduced.
which seems a bit falsely.
354-
355-
356-/** Test that CCheckQueueControl is threadsafe */
357-BOOST_AUTO_TEST_CASE(test_CheckQueueControl_Locks)
358-{
359- auto queue = MakeUnique<Standard_Queue>(QUEUE_BATCH_SIZE);
CCheckQueue::StartWorkerThreads(int).
Usually you want to use a callable template parameter…
0template<typename Callable>
1function(int x, Callable&& function) {
2 function(x);
3}
4function(1, [](int x) { return x});
5function(1, [](int x) { });
@JeremyRubin Thank you for your suggestion :)
Sorry for my broken English, but I meant do we really need a unit test if the CCheckQueue class has no interface to test?
Sorry to just be responding on this:
I think if there isn’t an interface for something, but you can expose one to make testing more robust, it’s worthwhile.
E.g., the CheckQueue does not need to be templated as we know a concrete single type for it, but templating it allows us to mock out the job type and probe the internal execution, which is a good thing.
What’s the missing interface here? I don’t see one being used in the code for this test.
34- boost::mutex mutex;
35-
36- //! Worker threads block on this when out of work
37- boost::condition_variable condWorker;
38-
39- //! Master thread blocks on this when out of work
Approach ACK f16f7f79dcd5d6c1b55966317b8762abeb5561c3 🌘
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3Approach ACK f16f7f79dcd5d6c1b55966317b8762abeb5561c3 🌘
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUhhpwv+NPSS8v5sdXJqBUVegg4/8o1tsvoNpU9cE80q4qYqkmrP2V55QWbkG2VI
8NE6+E0XSgTx8UuESI+9YEJ9+8gH4jCgNIqDMS4dqG+uPeeTXJUKn39Jn59YuaJGQ
9yWCCO4BZXqW/DWCLVpcDINXGtlZgFR3sv53RiFPWw+NNU5aJYTFssv9KJ8nBQVko
10SXNKSNzFxqx2oafuIaF+Ikrp+HbPzwE7Xrk2byS9Od1sA8d/LGkbhxBB4j32L4NT
118NNUBKdata4zjzkQCXHIrBYNxJxy8/qK45lMO4WjiApxyA5kbbOZ20v9eyh8oF5I
124hsvQN1Bpvdm8hK3oqxcLzZ2/QO6SjkNnVslNlFeYubasFMoTGS7n0SMqgmqgIbW
13qi9sMpsuMEBov6wq5kCIpWTBwHIuDMeRbjo/dOkJcTNqJHu8+s83aqD8P3MbTa0y
14ykeCbWwB8k0v5SPGhKMRx5sUMOGHQBWboDRzDJmNF909W+Yd3DL+C0qONk6vvMSy
1503UYQCgR
16=16Sm
17-----END PGP SIGNATURE-----
Timestamp of file with hash fd6cc8b78bfb432d1fffbc1706dd13c7f3563a0d2caef6be9b45688ccd1b15ad -
Updated f16f7f79dcd5d6c1b55966317b8762abeb5561c3 -> 7a7e346a1d153fc7367bca5a1926881af65279ec (pr18710.04 -> pr18710.05, diff):
thread.hpp with #include <boost/thread/shared_mutex.hpp> and kill boost thread from the linter?
Updated 7a7e346a1d153fc7367bca5a1926881af65279ec -> 61739174c67a773d124734ab6ec3385657fe3ef8 (pr18710.05 -> pr18710.06, diff):
In the last commit: Could replace the last occurrence of
thread.hppwith#include <boost/thread/shared_mutex.hpp>and kill boost thread from the linter?
Updated 61739174c67a773d124734ab6ec3385657fe3ef8 -> b3ac0c3d15a729cdce5f15b918947037905b59dc (pr18710.06 -> pr18710.07, diff):
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 …
Reworked dac216cee9e2e8fece089f67cf84f80f6890541f:
Code review ACK dac216cee9e2e8fece089f67cf84f80f6890541f.
Is f9c3bf45c13c147cb8d66d156519af021d66218e really a refactor due to changes between boost and std threads?
Updated dac216cee9e2e8fece089f67cf84f80f6890541f -> 0b5e2a5528fae627e7be98ac231bfb9ae24cd504 (pr18710.08 -> pr18710.09, diff):
GUARDED_BY thread-safety annotation
utack eee80a1
Failure is a build pause, job needs restart. I’m still not sure why the test needed to be removed, maybe you can explain that more clearly.
Updated eee80a1c36d886583e387934abce6dac9b3117b5 -> ec4401480246f60c6b22d98d205a509a181010d7 (pr18710.14 -> pr18710.15, diff):
test_CheckQueueControl_Locks unit test that was removed by mistake
utACK ec44014
Annoying note:
As noted in #18710 (comment), Would be good if someone with access to some of the different platforms/arches we support can do benching/real network profiling to just to confirm there’s no perf regression on the condition variable and mutex behavior. Otherwise seems ready to merge.
see https://codesequoia.wordpress.com/2013/03/27/condition-variables-performance-of-boost-win32-and-the-c11-standard-library/ for some context, some std synchronization primitives are worse on some platforms and can have more variance.
But to be clear, these potential regressions would probably be small compared to other improvements realizable in the checkqueue design, so I don’t think it’s a blocker for accepting these changes but we do want to know if merging this means we should be prioritizing some optimizations to neutralize any degradation.
To summarize the state of review for this PR:
Concept ACK (laanwj), Approach ACK (MarcoFalke), Code review ACK (Promag), utACK (JeremyRubin) with no NACKs or blocking concerns raised. @JeremyRubin asked if someone “with access to some of the different platforms/arches we support can do benching/real network profiling to just to confirm there’s no perf regression on the condition variable and mutex behavior.”
Benchmarks from the author (Linux Mint): master (a998c51)
0# Benchmark, evals, iterations, total, min, max, median
1CCheckQueueSpeedPrevectorJob, 100, 1400, 96.3747, 0.000677139, 0.000695471, 0.000688339
this PR (b02bc25 - pre-rebase)
0# Benchmark, evals, iterations, total, min, max, median
1CCheckQueueSpeedPrevectorJob, 100, 1400, 77.4393, 0.000545271, 0.00057892, 0.000552496
Updated benchmarks (Linux Mint 20, x86_64):
0# Benchmark, evals, iterations, total, min, max, median
1CCheckQueueSpeedPrevectorJob, 5, 1400, 4.31038, 0.000612868, 0.000620909, 0.000614335
0# Benchmark, evals, iterations, total, min, max, median
1CCheckQueueSpeedPrevectorJob, 5, 1400, 3.67213, 0.000517622, 0.000531811, 0.000523883
Rebased e7893d2e522477374ef9bf01997e9776c86b9813 -> a57a0e526c94b35b1b018fde5a8a2ea46fca8899 (pr18710.16 -> pr18710.17) due to the conflicts with #18011 and #19604.
Updated benchmarks (Linux Mint 20, x86_64):
0| ns/job | job/s | err% | total | benchmark
1|--------------------:|--------------------:|--------:|----------:|:----------
2| 189.96 | 5,264,153.75 | 4.1% | 0.01 | `CCheckQueueSpeedPrevectorJob`
0| ns/job | job/s | err% | total | benchmark
1|--------------------:|--------------------:|--------:|----------:|:----------
2| 157.27 | 6,358,598.51 | 1.9% | 0.01 | `CCheckQueueSpeedPrevectorJob
150+ nTotal = 0;
151+ fAllOk = true;
152+ }
153+ assert(m_worker_threads.empty());
154+ for (int n = 0; n < threads_num; ++n) {
155+ m_worker_threads.emplace_back([this, n]() {
e6eb62464cbfcc9f27a9e4d1e7377df30af77a86
nit, drop ().
63@@ -62,8 +64,11 @@ class CCheckQueue
64 //! The maximum number of elements to be processed in one batch
65 const unsigned int nBatchSize;
66
67+ std::vector<std::thread> m_worker_threads;
e6eb62464cbfcc9f27a9e4d1e7377df30af77a86
These could be guarded by mutex too? At least m_request_stop already is.
m_worker_threads is accessed from one thread only.
148+ boost::unique_lock<boost::mutex> lock(mutex);
149+ nIdle = 0;
150+ nTotal = 0;
151+ fAllOk = true;
152+ }
153+ assert(m_worker_threads.empty());
e6eb62464cbfcc9f27a9e4d1e7377df30af77a86
nit, && ! m_request_stop
Hi! I only have access to two different computers, an Intel i7-8700, and an Intel Celeron N3050. Compiled with g++ 10.1.0.
master f7c73b03d975a72f609ded2bbe250c1c8a76a944, i7-8700:
| ns/job | job/s | err% | ins/job | cyc/job | IPC | bra/job | miss% | total | benchmark |
|---|---|---|---|---|---|---|---|---|---|
| 582.25 | 1,717,469.22 | 0.5% | 267.01 | 347.19 | 0.769 | 50.65 | 3.7% | 0.02 | CCheckQueueSpeedPrevectorJob |
PR a57a0e526c94b35b1b018fde5a8a2ea46fca8899, i7-8700:
| ns/job | job/s | err% | ins/job | cyc/job | IPC | bra/job | miss% | total | benchmark |
|---|---|---|---|---|---|---|---|---|---|
| 371.47 | 2,692,026.21 | 0.6% | 260.57 | 334.12 | 0.780 | 49.78 | 3.7% | 0.01 | CCheckQueueSpeedPrevectorJob |
PR is much faster here, quite an improvement here! Results are also relatively stable, at least stable enough to see that the PR is faster on average.
Here the runtime fluctuates so much that it doesn’t make much sense to show the raw numbers. Especially the PR seems much more prone to fluctuations. To get a better understanding of the fluctuations, I’ve set epoch(100), and generated boxplots with
0std::ofstream html("pr.html");
1bench.render(ankerl::nanobench::templates::htmlBoxplot(), html);
Then I get two html’s for each run, which I’ve merged by hand:
The PR seems to be slower here, but this might also be an artifact of the benchmark. I don’t think this benchmark is very good.
I ran the benchmark again on the Intel Celeron N3050, but with BATCHES=5001 instead of 101. The benchmark takes a long time, and is a bit more stable. It still fluctuates by a factor of 4. On average the PR is slower than master on this machine.
Updated a57a0e526c94b35b1b018fde5a8a2ea46fca8899 -> e084c3b6051fc596d0b9a38ae7a5da58e3637a31 (pr18710.17 -> pr18710.18, diff):
addressed @promag’s nits
added commit b8e267134431db97e853cddd32fdc41716d3414e “bench: Prevent thread oversubscription”
Benchmarks on my machine (Linux Mint 20, x86_64):
0| ns/job | job/s | err% | total | benchmark
1|--------------------:|--------------------:|--------:|----------:|:----------
2| 191.58 | 5,219,878.03 | 2.1% | 0.01 | `CCheckQueueSpeedPrevectorJob`
0| ns/job | job/s | err% | total | benchmark
1|--------------------:|--------------------:|--------:|----------:|:----------
2| 182.46 | 5,480,666.18 | 2.0% | 0.01 | `CCheckQueueSpeedPrevectorJob`
0| ns/job | job/s | err% | total | benchmark
1|--------------------:|--------------------:|--------:|----------:|:----------
2| 150.84 | 6,629,325.36 | 1.1% | 0.01 | `CCheckQueueSpeedPrevectorJob`
Sure, I ran it again. i7-8700 is faster too. I also tested it with queue.StartWorkerThreads(1);, which is by far the fastest for that benchmark. It seems that the work units are far too small to be able to reasonably distribute the work. So I am not sure if this benchmark is much useful.
| ns/job | job/s | err% | ins/job | cyc/job | IPC | bra/job | miss% | total | benchmark |
|---|---|---|---|---|---|---|---|---|---|
| 582.25 | 1,717,469.22 | 0.5% | 267.01 | 347.19 | 0.769 | 50.65 | 3.7% | 0.02 | master f7c73b03d975a72f609ded2bbe250c1c8a76a944 |
| 371.47 | 2,692,026.21 | 0.6% | 260.57 | 334.12 | 0.780 | 49.78 | 3.7% | 0.01 | PR a57a0e526c94b35b1b018fde5a8a2ea46fca8899 |
| 351.57 | 2,844,406.84 | 1.7% | 261.09 | 332.71 | 0.785 | 49.90 | 3.6% | 0.11 | PR e084c3b6051fc596d0b9a38ae7a5da58e3637a31 |
| 131.95 | 7,578,398.22 | 1.1% | 262.19 | 252.28 | 1.039 | 50.15 | 3.8% | 0.04 | queue.StartWorkerThreads(1); |
Updated graph with BATCHES = 5001 on the Celeron N3050:
If I understand it correcly, the benchmark queues PrevectorJob as as the job to simulate. But the jobs itself do nothing, they simply return true:
0bool operator()() {
1 return true;
2}
I’d say the job should do some reasonable amount of work so using multiple threads actually make any sense.
Anyways, here is an update with b8e2671 included:
i7-8700:
| ns/job | job/s | err% | ins/job | cyc/job | IPC | bra/job | miss% | total | benchmark |
|---|---|---|---|---|---|---|---|---|---|
| 582.25 | 1,717,469.22 | 0.5% | 267.01 | 347.19 | 0.769 | 50.65 | 3.7% | 0.02 | master f7c73b03d975a72f609ded2bbe250c1c8a76a944 |
| 371.47 | 2,692,026.21 | 0.6% | 260.57 | 334.12 | 0.780 | 49.78 | 3.7% | 0.01 | PR a57a0e526c94b35b1b018fde5a8a2ea46fca8899 |
| 351.57 | 2,844,406.84 | 1.7% | 261.09 | 332.71 | 0.785 | 49.90 | 3.6% | 0.11 | PR e084c3b6051fc596d0b9a38ae7a5da58e3637a31 |
| 131.95 | 7,578,398.22 | 1.1% | 262.19 | 252.28 | 1.039 | 50.15 | 3.8% | 0.04 | queue.StartWorkerThreads(1); |
| 548.98 | 1,821,565.90 | 2.9% | 267.48 | 334.32 | 0.800 | 50.76 | 3.6% | 0.02 | b8e267134 |
Celeron N3050:
They behave very similar, e084c3b6051fc596d0b9a38ae7a5da58e3637a31 is a bit faster than b8e267134.
This PR consist of two distinct changes now:
boost::thread_groupWhat did I miss?
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 bb6fcc75d1ec94b733d1477c816351c50be5faf9
nit: remove the
#include <boost/thread/thread.hpp>from checkqueue_tests.cpp.
boost::thread_group is still used in the test_CheckQueueControl_Locks.
boost::thread_groupis still used in thetest_CheckQueueControl_Locks
It builds for me without that include. Oh, I see, it’s included indirectly via test/util/setup_common.h, thanks.
Thanks for getting rid of the one-to-last use of boost::thread_group.
Code review ACK bb6fcc75d1ec94b733d1477c816351c50be5faf9
Labels
Refactoring
Validation
Milestone
22.0