This PR significantly reduces lock contention in the CCheckQueue class by releasing a mutex before calling std::condition_variable::notify_one and std::condition_variable::notify_all.
From C++ docs:
The notifying thread does not need to hold the lock on the same mutex as the one held by the waiting thread(s); in fact doing so is a pessimization, since the notified thread would immediately block again, waiting for the notifying thread to release the lock.
Related to:
tACK 5bcfd70 on Ubuntu 20.04
This PR moves LOCK(m_mutex) to a smaller scope, so when code reaches notify_one()or notify_all(), the mutex is already released.
It also changes CCheckQueue ::Add to exit earlier if vChecks is empty.
166@@ -167,15 +167,20 @@ class CCheckQueue
167 //! Add a batch of checks to the queue
168 void Add(std::vector<T>& vChecks)
169 {
170- LOCK(m_mutex);
171- for (T& check : vChecks) {
172- queue.push_back(T());
173- check.swap(queue.back());
174+ unsigned int num_of_new_checks;
unsigned int is typically smaller than the size of a vector. Use size_t or std::vector<T>::size_type perhaps?
I very much doubt we’ll ever have more than 4 billion checks queued up, but it also seems simpler to make the type can just account for everything.
I’ve chosen unsinged int due to https://github.com/bitcoin/bitcoin/blob/5adc5c02800f00d1e6e8812a2b0559b1800e82e9/src/checkqueue.h#L60
Maybe use size_t for nTodo as well?
185- if (vChecks.size() == 1)
186+
187+ if (num_of_new_checks == 1)
188 m_worker_cv.notify_one();
189- else if (vChecks.size() > 1)
190+ else if (num_of_new_checks > 1)
176+ LOCK(m_mutex);
177+ for (T& check : vChecks) {
178+ queue.push_back(T());
179+ check.swap(queue.back());
180+ }
181+ num_of_new_checks = vChecks.size();
const auto num_of_new_checks = vChecks.size(); above, saves one line of code in the lock and doesn’t keep an uninitialized variable around for a few lines.
vChecks doesn’t change size during the adding loop. The old vChecks.size() based checks can be kept.
vChecksdoesn’t change size during the adding loop.
Correct. But vChecks is passed in CCheckQueue::Add by a non-const reference, and that makes possible its modification by a concurrent thread between leaving a critical section and checking num_of_new_checks == 1.
Although, the only caller is located in CChainState::ConnectBlock which is guarded by the cs_main mutex. Therefore, you are right.
The old
vChecks.size()based checks can be kept.
Yes, it will work. But while looking into CCheckQueue::Add code, it is not obvious that vChecks.size() is constant in multi-threading environment. I think that adding a local variable makes code more readable and robust considering any possible changes at the caller site(s) in the future.
OTOH, it seems all could look clearer if we pass vChecks as an rvalue reference using move semantics. What do you think?
I would never think that a reference argument could be changed by another thread while the function is executing. I hope we don’t do that anywhere in the code. Because how would the function avoid races? Which mutex protects the argument? That would require something like (it does not compile, of course):
0void func(T& arg GUARDED_BY(some_mutex), ...)
1{
2 ...
3 LOCK(some_mutex);
4 access arg;
5 ...
So, I think it is safe to assume that vChecks will not change while Add() is executing. Indeed the only caller passes in a local variable that is not visible by other threads (and then throws it away, so the “swap” is actually a “move”).
… it seems all could look clearer if we pass vChecks as an rvalue reference using move semantics …
Yes :) and std::move() each element from vChecks into queue. It is unnecessary to first create a default-constructed object only to swap it with another and throw it away. This is what std::move() is for.
@hebasto That’s not how const works in C++. The lack thereof means you (the called function in this case) are allowed to modify the argument; it says nothing at all about whether another thread might be modifying the same variable. Even if the argument was const, nothing prevents another thread from holding a mutable reference to the same object.
That said, passing an argument (const or not) to a function while it is being modified by other threads would be completely non-idiomatic, dangerous code. It is possible, and sometimes necessary of course, but at least you’d expect the passed object to have internal mutexes or other synchronization mechanisms to protect its state in that case; not something the callee should deal with. Virtually every STL function will exhibit undefined behavior if you pass it references to arguments that are being modified concurrently. Outside of very exceptional cases that should raise big red flags, you can very much assume that arguments passed to a function are under exclusive control of that function for the duration of its execution.
173- check.swap(queue.back());
174+ unsigned int num_of_new_checks;
175+ {
176+ LOCK(m_mutex);
177+ for (T& check : vChecks) {
178+ queue.push_back(T());
queue.emplace_back() instead queue.push_back(T()) would save one move/copy constructor and one destructor because the object could be directly constructed in the queue
queue.push_back(std::move(check))?
I tried that once but then tests fail, I think for the types used here std::swap is implemented but a move constructor isn’t. I didn’t have a closer look why that fails.
I think the nicest c++ solution would be to replace the whole loop with
0queue.insert(queue.end(), std::make_move_iterator(vChecks.begin()), std::make_move_iterator(vChecks.end()));
But this too requires properly implementing move constructor.
166@@ -167,16 +167,26 @@ class CCheckQueue
167 //! Add a batch of checks to the queue
168 void Add(std::vector<T>& vChecks)
169 {
170- LOCK(m_mutex);
171- for (T& check : vChecks) {
172- queue.push_back(T());
173- check.swap(queue.back());
174+ if (vChecks.empty()) {
The code below assumes vChecks may be modified by another thread while this is executing (it cannot actually). I guess either remove that assumption or move this access to vChecks under m_mutex (if that mutex it supposed to protect vChecks!?).
It is a race to call std::vector::empty() while modifying it concurrently.
You are right.
I guess either remove that assumption or move this access to
vChecksunderm_mutex(if that mutex it supposed to protectvChecks!?).
The former will go.
Thank you for your reviews and your suggestions!
Updated 5bcfd70a3b0e008d7a80e37a29fe90887a0f8c66 -> 459e208276a4d1457d37bf41c977e62caf05456d (pr23397.01 -> pr23397.02, diff):
ACK 459e208276a4d1457d37bf41c977e62caf05456d
The CI failure looks unrelated.
ACKish - I haven’t reviewed the code, but it works for the benchmarks and so I guess in some sense I’ve tested it. As expected, no big diff in performance.
| bench name | x | #23397 | $mergebase |
|---|---|---|---|
| ibd.local.range.500000.540000.total_secs | 2 | 4826.3510 (± 0.4951) | 4831.4970 (± 6.8399) |
| ibd.local.range.500000.540000.peak_rss_KiB | 2 | 6410470.0000 (± 1750.0000) | 6406678.0000 (± 2714.0000) |
| ibd.local.range.500000.540000.cpu_kernel_secs | 2 | 345.3800 (± 3.1200) | 348.2850 (± 0.8050) |
| ibd.local.range.500000.540000.cpu_user_secs | 2 | 30102.6800 (± 5.2600) | 30086.9150 (± 24.1150) |
| bench name | x | #23397 | $mergebase |
|---|---|---|---|
| ibd.local.range.500000.540000.total_secs | 2 | 1.000 | 1.001 |
| ibd.local.range.500000.540000.peak_rss_KiB | 2 | 1.001 | 1.000 |
| ibd.local.range.500000.540000.cpu_kernel_secs | 2 | 1.000 | 1.008 |
| ibd.local.range.500000.540000.cpu_user_secs | 2 | 1.001 | 1.000 |
Code-review ACK 459e208276a4d1457d37bf41c977e62caf05456d
Maybe I’m missing something, but IMHO there is currently no possible way that CCheckQueue:Add gets called with an empty vector as argument, as the size always equals the number of inputs of a transaction (see last out parameter of CheckInputScripts). It’d be only empty for coinbase transactions, but for those control.Add(...) doesn’t get called. That said, it doesn’t hurt if the empty check in Add is still there.