This PR removes use of the following object in checkqueue
boost::condition_variableboost::mutexboost::unique_lockInstead of using boost::thread_group::interrupt_all to trigger boost::thread_interrupted exception, it should call InterruptScriptCheck() or scriptcheckqueue.Interrupt() to interrupt the checkqueue thread.
Also, it adds three functions:
StartScriptCheck()InterruptScriptCheck()StopScriptCheck()for clearer logic for script check threads.
67@@ -68,7 +68,6 @@ EXPECTED_BOOST_INCLUDES=(
68 boost/test/unit_test.hpp
69 boost/thread.hpp
70 boost/thread/condition_variable.hpp
boost/thread/condition_variable.hpp should go as well? :-)
Concept ACK
Towards a Boost free future! :-)
61@@ -62,17 +62,19 @@ class CCheckQueue
62 //! The maximum number of elements to be processed in one batch
63 unsigned int nBatchSize;
64
65+ std::atomic<bool> fInterrupted;
interrupted
97@@ -96,7 +98,11 @@ class CCheckQueue
98 return fRet;
99 }
100 nIdle++;
101+ if (interrupted)
102+ return false;
103 cond.wait(lock); // wait
104+ if (interrupted)
105+ return false;
If an
ifonly has a single-statementthen-clause, it can appear on the same line as theif, without braces. In every other case, braces are required, and thethenandelseclauses must appear correctly indented on a new line.
https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c
259@@ -260,8 +260,12 @@ bool LoadBlockIndex(const CChainParams& chainparams) EXCLUSIVE_LOCKS_REQUIRED(cs
260 bool LoadChainTip(const CChainParams& chainparams);
261 /** Unload database information */
262 void UnloadBlockIndex();
263-/** Run an instance of the script checking thread */
264-void ThreadScriptCheck();
265+/** Start script checking thread */
266+void StartScriptCheck();
267+/** Interrupt all script checking thread */
threads
165+ {
166+ {
167+ std::lock_guard<std::mutex> lock(mutex);
168+ interrupted = true;
169+ }
170+ condMaster.notify_all();
condMaster.notify_one() is more consistent. No difference to code though.
147@@ -149,9 +148,9 @@ typedef CCheckQueue<FrozenCleanupCheck> FrozenCleanup_Queue;
148 static void Correct_Queue_range(std::vector<size_t> range)
149 {
150 auto small_queue = MakeUnique<Correct_Queue>(QUEUE_BATCH_SIZE);
151- boost::thread_group tg;
152+ std::vector<std::thread> tg;
nScriptCheckThreads before loop?
461@@ -462,12 +462,13 @@ BOOST_AUTO_TEST_CASE(test_big_witness_transaction) {
462
463 // check all inputs concurrently, with the cache
464 PrecomputedTransactionData txdata(tx);
465- boost::thread_group threadGroup;
466+ std::vector<std::thread> threadGroup;
20? :-)
97@@ -96,7 +98,9 @@ class CCheckQueue
98 return fRet;
99 }
100 nIdle++;
101+ if (interrupted) return false;
102 cond.wait(lock); // wait
Add the condition as a callback to wait.
e.g., cond.wait(lock, [&] { return interrupted || !queue.empty(); })
Weak concept Ack.
Necessary step to get rid of boost, but conflicts with some unmerged work (https://github.com/bitcoin/bitcoin/pull/9938, https://github.com/JeremyRubin/bitcoin/pull/1) which does the same thing.
Overall, this appears to be correct.
Before merge I’d like to see some benchmarks, the checkqueue code is very performance sensitive, sometimes the boost condition variables and mutexs difference from std’s can be substantial. (I’m not kidding – the time to wake-up and condition variable sleep behavior is one of the major performance issues with this code).
1200@@ -1199,12 +1201,6 @@ bool AppInitMain()
1201 InitSignatureCache();
1202 InitScriptExecutionCache();
1203
1204- LogPrintf("Using %u threads for script verification\n", nScriptCheckThreads);
StartScriptCheck(); line; why is that not necessary?
@JeremyRubin benchmark result on my Ubuntu 18.04 virtual machine: master 816fab9ccae568612d5ed90378b4587256925a1e
0# Benchmark, evals, iterations, total, min, max, median
1CCheckQueueSpeedPrevectorJob, 100, 1400, 83.1845, 0.000461328, 0.000692877, 0.000668382
This PR 625c55a70c376af9a94a2bb118c39685d3b82b33
0# Benchmark, evals, iterations, total, min, max, median
1CCheckQueueSpeedPrevectorJob, 100, 1400, 66.268, 0.000451829, 0.000491473, 0.000473479
That’s great, thanks for checking that – much bigger change than I expected it to be.
Looking at the numbers, it seems not only to be faster on the microbenchmark, but more consistent.
This merits running two additional tests, if you have energy for it:
The second test is important because one thing I noticed when working on this code previously is that it was trivially easy to get speedups on this code at the expense of general performance. E.g., getting rid of condition variables (and just spinning until jobs came in) was much faster, but meant that the OS scheduler would be giving our CheckQueue workers run-time when there was nothing to do, impacting other processes on the computer (most likely because of causing more cache invalidation, rather than the impact of losing a few cycles of a process switch). Hence my suspicion: the 20% speedup is because the standard condition variables are not using system sleep (which plays better with the OS). This can actually negatively impact the process too as our main thread might be getting context switched out while we are trying to add jobs or something rather than waiting for notify.
98 // return the current status
99 return fRet;
100 }
101 nIdle++;
102- cond.wait(lock); // wait
103+ cond.wait(lock, [&] { return interrupted || !queue.empty() || fMaster && nTodo == 0; }); // wait
I think you could assert(! (fMaster && nTodo == 0)) here.
If fMaster were ever true here, we would deadlock.
Hence, there is no point to check that…
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
No conflicts as of last run.
98 // return the current status
99 return fRet;
100+ } else {
101+ nIdle++;
102+ while (!(interrupted || !queue.empty() || (fMaster && nTodo == 0))) cond.wait(lock);
103+ if (interrupted) return false;
return false or somthing else? seem to make the node have to reindex next time.
The issue could be that when you interrupt, you don’t want to make fMaster return false causing the block to be marked false (which could be written into cache somehow).
Is interrupt guaranteed to wait until the current job is done? Making it guaranteed to wait probably could help fix that behavior… One way of doing that is to separate out when you wake up if there’s an active job or not, and don’t allow to quit when there is an active job until it’s done.
This version is not correct either if I’m correct about the block getting flushed with validity cached during shutdown.
Now you might incorrectly cache that a block was valid when it may not have been.
You need to finish processing the block, then shut down. Or, if a shutdown is triggered, abandon the block in the master (you could make a trivalent return: false, true, shutdown).
interrupted is true. It would keep looping until nTodo == 0.
@JeremyRubin How about now? I think the logic here would be more clear. It would keep checking until finish. If there are new tasks added into queue after the worker threads ended, it would be only master worker checking the tasks unfortunately. However I think this is better than interrupt the master thread. Alternatively, we can detach these worker threads.
I am going to test this to see if there is anything broken.
I still think it’s best to make control of the checkqueue (as opposed to participation) protected by a mutex.
Easiest to audit/review.
But do let me know if that fixes the corruption issue…
I still think it’s best to make control of the checkqueue (as opposed to participation) protected by a mutex.
There already have ControlMutex to ensure that there is only one CCheckQueueControl to access the CCheckQueue.
Edit: I don’t think it’s necessary to interrupt the master queue though. We can wait it to return the check result.
The second thought, if we detach the worker thread, then that is not necessary to interrupt the checkqueue. So I start to suspect that if the Interrupt is necessary here.
I am saying that a simple change to make is to have Interrupt take the ControlMutex.
That would cause recursive lock in test_bitcoin. Also, I don’t think that it’s necessary to lock ControlMutex. Can you explain why it’s necessary for Interrupt?
But do let me know if that fixes the corruption issue…
Seems the node won’t have to reindex after restarting now.
CCheckQueueControl and call Interrupt in same thread
Interrupt should be called in the destructor of the test which should happen after any queuecontrols are destructed though?
I think it would be better to make the checkqueue to control their worker threads instead of calling Thread() in somewhere.
git rebase -Xignore-all-space df36ddf9ce8a4dcf0d7f324e57a13abb6cf6a57c?
This commit add a Interrupt function for CCheckQueue that it can handle interrupt by itself instead of relying on boost thread interrupt
replace boost::mutex with debuggable Mutex
replace boost::condition_variable with std::condition_variable
add const specifier to fMaster and nBatchSize
add clang thread safety attributes
move init value of member of CCheckQueue from constructor to definition
Manage CCheckQueue workder threads by itself. Do not expose the threads outside the object.