Repository: bitcoin/bitcoin
File: src/checkqueue.h (C/C++)

Code before:
                }
                // logically, the do loop starts here
                while (queue.empty() && !m_request_stop) {
                    if (fMaster && nTodo == 0) {
                        nTotal--;
                        bool fRet = fAllOk;
                        // reset the status for new work later
                        fAllOk = true;
                        // return the current status
                        return fRet;
                    }
                    nIdle++;
                    cond.wait(lock); // wait
                    nIdle--;
                }
                if (m_request_stop) {
                    return false;
                }

                // Decide how many work units to process now.
                // * Do not try to do everything at once, but aim for increasingly smaller batches so
                //   all workers finish approximately simultaneously.
                // * Try to account for idle jobs which will instantly start helping.
                // * Don't do batches smaller than 1 (duh), or larger than nBatchSize.
                nNow = std::max(1U, std::min(nBatchSize, (unsigned int)queue.size() / (nTotal + nIdle + 1)));
                for (unsigned int i = 0; i < nNow; i++) {
                    vChecks.push_back(std::move(queue.back()));
                    queue.pop_back();
                }
                // Check whether we need to do work at all
                fOk = fAllOk;
            }
            // execute work
            for (T& check : vChecks)
                if (fOk)
                    fOk = check();
            vChecks.clear();
        } while (true);
    }

public:
    //! Mutex to ensure only one concurrent CCheckQueueControl
    Mutex m_control_mutex;

    //! Create a new check queue
    explicit CCheckQueue(unsigned int nBatchSizeIn)
        : nBatchSize(nBatchSizeIn)
    {
    }

    //! Create a pool of new worker threads.

Review comment (performance):
Instead of the loop I'd use this bulk method:
```cpp
auto start_it = queue.end() - nNow;
vChecks.assign(std::make_move_iterator(start_it), std::make_move_iterator(queue.end()));
queue.erase(start_it, queue.end());
```

Again that should be faster than lots of calls to `push_back` which need to check the capacity, also I find it more readable. Note that the difference is that the queue items will be added in a non-reversed order, but I'd say that should be irrelevant.

I gave that a try, and to my surprise this interestingly doesn't compile, for these reasons:

The simple `std::move` here is more or less a best-effort, when the object can't be moved, it will be copied. That is not the case when using `std::make_move_iterator`, it has a `static_assert` that the object must be move-assignable. So actually for `DumbCheck`, which is actually copied because it has a `const bool` member. Another issue is that `FrozenCleanupCheck` doesn't implement move-assignment. 

So full change set should be this:

```diff
diff --git a/src/checkqueue.h b/src/checkqueue.h
index cb751d07f0..a309372ac8 100644
--- a/src/checkqueue.h
+++ b/src/checkqueue.h
@@ -111,10 +111,9 @@ private:
                 // * Try to account for idle jobs which will instantly start helping.
                 // * Don't do batches smaller than 1 (duh), or larger than nBatchSize.
                 nNow = std::max(1U, std::min(nBatchSize, (unsigned int)queue.size() / (nTotal + nIdle + 1)));
-                for (unsigned int i = 0; i < nNow; i++) {
-                    vChecks.push_back(std::move(queue.back()));
-                    queue.pop_back();
-                }
+                auto start_it = queue.end() - nNow;
+                vChecks.assign(std::make_move_iterator(start_it), std::make_move_iterator(queue.end()));
+                queue.erase(start_it, queue.end());
                 // Check whether we need to do work at all
                 fOk = fAllOk;
             }
@@ -170,7 +169,7 @@ public:
 
         {
             LOCK(m_mutex);
-            std::move(vChecks.begin(), vChecks.end(), std::back_inserter(queue));
+            queue.insert(queue.end(), std::make_move_iterator(vChecks.begin()), std::make_move_iterator(vChecks.end()));
             nTodo += vChecks.size();
         }
 
diff --git a/src/test/checkqueue_tests.cpp b/src/test/checkqueue_tests.cpp
index 9954780a7a..5c4ec55da3 100644
--- a/src/test/checkqueue_tests.cpp
+++ b/src/test/checkqueue_tests.cpp
@@ -128,6 +128,12 @@ struct FrozenCleanupCheck {
         should_freeze = other.should_freeze;
         other.should_freeze = false;
     }
+    FrozenCleanupCheck& operator=(FrozenCleanupCheck&& other) noexcept
+    {
+        should_freeze = other.should_freeze;
+        other.should_freeze = false;
+        return *this;
+    }
 };
 
 // Static Allocations
diff --git a/src/test/fuzz/checkqueue.cpp b/src/test/fuzz/checkqueue.cpp
index f684e01e36..429570526f 100644
--- a/src/test/fuzz/checkqueue.cpp
+++ b/src/test/fuzz/checkqueue.cpp
@@ -13,7 +13,7 @@
 
 namespace {
 struct DumbCheck {
-    const bool result = false;
+    bool result = false;
 
     explicit DumbCheck(const bool _result) : result(_result)
     {
```

I ran a benchmark `CCheckQueueSpeedPrevectorJob` with merge-base, the PR, and the PR with my changes: 

|              ns/job |               job/s |    err% |         ins/job |         cyc/job |    IPC |        bra/job |   miss% |     total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
|              395.62 |        2,527,646.43 |    0.7% |          225.93 |          400.00 |  0.565 |          45.40 |    7.6% |      5.53 | `merge-base`
|              384.72 |        2,599,320.73 |    0.7% |          244.92 |          402.79 |  0.608 |          49.41 |    7.0% |      5.56 | `PR`
|              364.97 |        2,739,962.15 |    0.7% |          213.05 |          377.85 |  0.564 |          44.54 |    7.7% |      5.48 | `proposed changes`


Code after:
                    nTotal++;
                }
                // logically, the do loop starts here
                while (queue.empty() && !m_request_stop) {
                    if (fMaster && nTodo == 0) {
                        nTotal--;
                        bool fRet = fAllOk;
                        // reset the status for new work later
                        fAllOk = true;
                        // return the current status
                        return fRet;
                    }
                    nIdle++;
                    cond.wait(lock); // wait
                    nIdle--;
                }
                if (m_request_stop) {
                    return false;
                }

                // Decide how many work units to process now.
                // * Do not try to do everything at once, but aim for increasingly smaller batches so
                //   all workers finish approximately simultaneously.
                // * Try to account for idle jobs which will instantly start helping.
                // * Don't do batches smaller than 1 (duh), or larger than nBatchSize.
                nNow = std::max(1U, std::min(nBatchSize, (unsigned int)queue.size() / (nTotal + nIdle + 1)));
                auto start_it = queue.end() - nNow;
                vChecks.assign(std::make_move_iterator(start_it), std::make_move_iterator(queue.end()));
                queue.erase(start_it, queue.end());
                // Check whether we need to do work at all
                fOk = fAllOk;
            }
            // execute work
            for (T& check : vChecks)
                if (fOk)
                    fOk = check();
            vChecks.clear();
        } while (true);
    }

public:
    //! Mutex to ensure only one concurrent CCheckQueueControl
    Mutex m_control_mutex;

    //! Create a new check queue
    explicit CCheckQueue(unsigned int nBatchSizeIn)
        : nBatchSize(nBatchSizeIn)
    {
    }

    //! Create a pool of new worker threads.