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
Concept ACK on getting rid of boost in consensus code. This pull looks similar to #14464?
Also, with this PR (I hope) it could be easier to resurrect a bunch of brilliant ideas from #9938.
cc @JeremyRubin
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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:
# Benchmark, evals, iterations, total, min, max, median
CCheckQueueSpeedPrevectorJob, 100, 1400, 96.3747, 0.000677139, 0.000695471, 0.000688339
# Benchmark, evals, iterations, total, min, max, median
CCheckQueueSpeedPrevectorJob, 100, 1400, 77.4393, 0.000545271, 0.00057892, 0.000552496
Rebased b02bc2583e421dbca35a5652c650ae861230f480 -> https://github.com/bitcoin/bitcoin/commit/f16f7f79dcd5d6c1b55966317b8762abeb5561c3 (pr18710.03 -> pr18710.04) due to the conflict with #18575.
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) {
for (int n = 0; n < threads_num; ++n) {
is shorter and more precise
Agree.
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);
is this useful in any way?
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;
Is this needed? the for loop will never run anyway when this is true.
The idea was to prevent 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.
Also, shouldn't the 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>
1c7fdb71b32bd91f6ae10313169d9eec63d46e3e: This commit should probably go last, because some of the changes are removed earlier anyway
In that case in the commit "bench: Use CCheckQueue local thread pool" the lint-includes.sh linter will produce:
Good job! The Boost dependency "boost/thread/thread.hpp" is no longer used.
Please remove it from EXPECTED_BOOST_INCLUDES in ./test/lint/lint-includes.sh
to make sure this dependency is not accidentally reintroduced.
which seems a bit falsely.
I hope that no one is running the linters on every commit. They are meant to warn authors and reviewers as well as maintainers of code smell in a pull request as a whole. They are not meant as unit tests that absolutely must pass on every commit.
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);
d64dd5ffee064b0b943b7e00b16b0ad28ac65474: Why remove this test?
~I have no idea how to pass a customized callback into the CCheckQueue::StartWorkerThreads(int).~
Usually you want to use a callable template parameter...
template<typename Callable>
function(int x, Callable&& function) {
function(x);
}
function(1, [](int x) { return x});
function(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.
@MarcoFalke @JeremyRubin I was confused about this test (I don't know the exact reason of that confusion now). The test has been restored.
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
629dae24827121412fb24d31ac19a918ade1db3f: why remove the documentation?
Approach ACK f16f7f79dcd5d6c1b55966317b8762abeb5561c3 🌘
<details><summary>Show signature and timestamp</summary>
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
Approach ACK f16f7f79dcd5d6c1b55966317b8762abeb5561c3 🌘
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUhhpwv+NPSS8v5sdXJqBUVegg4/8o1tsvoNpU9cE80q4qYqkmrP2V55QWbkG2VI
NE6+E0XSgTx8UuESI+9YEJ9+8gH4jCgNIqDMS4dqG+uPeeTXJUKn39Jn59YuaJGQ
yWCCO4BZXqW/DWCLVpcDINXGtlZgFR3sv53RiFPWw+NNU5aJYTFssv9KJ8nBQVko
SXNKSNzFxqx2oafuIaF+Ikrp+HbPzwE7Xrk2byS9Od1sA8d/LGkbhxBB4j32L4NT
8NNUBKdata4zjzkQCXHIrBYNxJxy8/qK45lMO4WjiApxyA5kbbOZ20v9eyh8oF5I
4hsvQN1Bpvdm8hK3oqxcLzZ2/QO6SjkNnVslNlFeYubasFMoTGS7n0SMqgmqgIbW
qi9sMpsuMEBov6wq5kCIpWTBwHIuDMeRbjo/dOkJcTNqJHu8+s83aqD8P3MbTa0y
ykeCbWwB8k0v5SPGhKMRx5sUMOGHQBWboDRzDJmNF909W+Yd3DL+C0qONk6vvMSy
03UYQCgR
=16Sm
-----END PGP SIGNATURE-----
Timestamp of file with hash fd6cc8b78bfb432d1fffbc1706dd13c7f3563a0d2caef6be9b45688ccd1b15ad -
</details>
Updated f16f7f79dcd5d6c1b55966317b8762abeb5561c3 -> 7a7e346a1d153fc7367bca5a1926881af65279ec (pr18710.04 -> pr18710.05, diff):
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?
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?
@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 ...~
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
Rebased 0b5e2a5528fae627e7be98ac231bfb9ae24cd504 -> 82d5e650ee2b72d1447cbfc7bf1bb107dfd390b6 (pr18710.09 -> pr18710.10) on top of the #18881.
Rebased 82d5e650ee2b72d1447cbfc7bf1bb107dfd390b6 -> 46838e94da7c54389bce1fb7c0951ab338dcc304 (pr18710.10 -> pr18710.11) on top of the #16127.
Rebased 46838e94da7c54389bce1fb7c0951ab338dcc304 -> 8457827f784e0e2586b19e78785d143b8eeb9513 (pr18710.11 -> pr18710.12) on top of the #18792.
Rebased 8457827f784e0e2586b19e78785d143b8eeb9513 -> b392ef178725795d9e38a4dd000b20ee47c0d60a (pr18710.12 -> pr18710.13) due to the conflict with #19142.
Rebased b392ef178725795d9e38a4dd000b20ee47c0d60a -> eee80a1c36d886583e387934abce6dac9b3117b5 (pr18710.13 -> pr18710.14) due to the conflict with #18758.
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.
Concept ACK, will review
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)
# Benchmark, evals, iterations, total, min, max, median
CCheckQueueSpeedPrevectorJob, 100, 1400, 96.3747, 0.000677139, 0.000695471, 0.000688339
this PR (b02bc25 - pre-rebase)
# Benchmark, evals, iterations, total, min, max, median
CCheckQueueSpeedPrevectorJob, 100, 1400, 77.4393, 0.000545271, 0.00057892, 0.000552496
Rebased ec4401480246f60c6b22d98d205a509a181010d7 -> e7893d2e522477374ef9bf01997e9776c86b9813 (pr18710.15 -> pr18710.16) due to the conflicts with #18637 and #19589.
Updated benchmarks (Linux Mint 20, x86_64):
# Benchmark, evals, iterations, total, min, max, median
CCheckQueueSpeedPrevectorJob, 5, 1400, 4.31038, 0.000612868, 0.000620909, 0.000614335
# Benchmark, evals, iterations, total, min, max, median
CCheckQueueSpeedPrevectorJob, 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):
| ns/job | job/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 189.96 | 5,264,153.75 | 4.1% | 0.01 | `CCheckQueueSpeedPrevectorJob`
| ns/job | job/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 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.
Why? 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
std::ofstream html("pr.html");
bench.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):
| ns/job | job/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 191.58 | 5,219,878.03 | 2.1% | 0.01 | `CCheckQueueSpeedPrevectorJob`
| ns/job | job/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 182.46 | 5,480,666.18 | 2.0% | 0.01 | `CCheckQueueSpeedPrevectorJob`
| ns/job | job/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 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:
bool operator()() {
return true;
}
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?
Rebased e084c3b6051fc596d0b9a38ae7a5da58e3637a31 -> fa815a305108bc8b25ac468abaec7fa60c261f41 (pr18710.18 -> pr18710.19) on top of #19710 to prevent a merge conflict.
Rebased fa815a305108bc8b25ac468abaec7fa60c261f41 -> 89c4f7591e02df4fd36071995740113c1c06db3d (pr18710.19 -> pr18710.20) due to the conflict with #19710.
Rebased 89c4f7591e02df4fd36071995740113c1c06db3d -> bb6fcc75d1ec94b733d1477c816351c50be5faf9 (pr18710.20 -> pr18710.21) due to the conflict with #19927.
Concept ACK, will plan to review.
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
Code review ACK bb6fcc75d1ec94b733d1477c816351c50be5faf9 and verified rebase to master builds cleanly with unit/functional tests green
Milestone
22.0