coins: avoid redundant prevout preparation allocations #36496

696ddc5 coins: reuse prevout task vector storage:

The vector is cast to an r-value reference. (The template type becoming an l-value is banned through !std::is_lvalue_reference_v<R>). The argument to Submit() is never used to construct a new vector inside the method, which means the storage of the underlying vector is never currently "reaped". My inference is that the ThreadPool was written that way since we move from the individual elements of the range.

It would probably be more honest/correct to allow non-const l-values to be passed into Submit() and remove the move() here. !std::is_const_v<std::remove_reference_t<R>> could be sufficient, and the l-value/r-value nature of R would be up to the caller as was intended when r-value references were introduced.

<details><summary>Compiling diff</summary>

diff --git a/src/coins.cpp b/src/coins.cpp
index e32e7f21d8..bf8c2bd766 100644
--- a/src/coins.cpp
+++ b/src/coins.cpp
@@ -397,7 +397,7 @@ CCoinsViewCache::ResetGuard CoinsViewOverlay::StartFetching(const CBlock& block
             m_tasks.assign(std::min(workers_count, m_inputs.size()), [this] {
                 while (ProcessInput()) {}
             });
-            if (auto futures{m_thread_pool->Submit(std::move(m_tasks))}) {
+            if (auto futures{m_thread_pool->Submit(m_tasks)}) {
                 m_futures = std::move(*futures);
             } else {
                 // Submit can fail if a shared owner of the thread pool outside of this class calls Stop() or
diff --git a/src/util/threadpool.h b/src/util/threadpool.h
index 6c16851525..421f956367 100644
--- a/src/util/threadpool.h
+++ b/src/util/threadpool.h
@@ -216,7 +216,7 @@ public:
      *          uncaught exceptions, as they would otherwise be silently discarded.
      */
     template <std::ranges::sized_range R>
-        requires(!std::is_lvalue_reference_v<R>)
+        requires(!std::is_const_v<std::remove_reference_t<R>>)
     [[nodiscard]] util::Expected<std::vector<RangeFuture<R>>, SubmitError> Submit(R&& fns) noexcept EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
     {
         std::vector<RangeFuture<R>> futures;

</details>

Thanks, added you as coauthor and also handled the empty tasks internally

Upon further consideration, influenced by on Ralph's feedback in https://git.fish.foo/bitcoin/bitcoin/pulls/36496#issuecomment-610179; it may be safer to revert my suggestion and instead opt for adding Assume(m_tasks.capacity() >= std::min(workers_count, m_inputs.size())) with an explanatory comment like so:

diff --git a/src/coins.cpp b/src/coins.cpp
index cdc5a8858d..88bea6c391 100644
--- a/src/coins.cpp
+++ b/src/coins.cpp
@@ -395,7 +395,10 @@ CCoinsViewCache::ResetGuard CoinsViewOverlay::StartFetching(const CBlock& block
         m_tasks.assign(std::min(workers_count, m_inputs.size()), [this] {
             while (ProcessInput()) {}
         });
-        if (auto futures{m_thread_pool->Submit(m_tasks)}) {
+        if (auto futures{m_thread_pool->Submit(std::move(m_tasks))}) {
+            // We expect to be able to reuse the backing array.
+            Assume(m_tasks.capacity() >= std::min(workers_count, m_inputs.size()));
+
             m_futures = std::move(*futures);
         } else {
             // Submit can fail if a shared owner of the thread pool outside of this class calls Stop() or
diff --git a/src/test/threadpool_tests.cpp b/src/test/threadpool_tests.cpp
index 3f802b754f..4811d73215 100644
--- a/src/test/threadpool_tests.cpp
+++ b/src/test/threadpool_tests.cpp
@@ -469,7 +469,6 @@ BOOST_AUTO_TEST_CASE(submit_range_of_tasks_complete_successfully)
     std::array<std::function<int32_t()>, static_cast<size_t>(num_tasks)> array_tasks;
     std::vector<std::function<int32_t()>> vector_tasks;
     vector_tasks.reserve(static_cast<size_t>(num_tasks));
-    BOOST_CHECK(Assert(threadPool.Submit(vector_tasks))->empty());
     for (const auto i : std::views::iota(int32_t{1}, num_tasks + 1)) {
         array_tasks.at(static_cast<size_t>(i - 1)) = [i, square] { return square(i); };
         vector_tasks.emplace_back([i, square] { return square(i); });
@@ -477,7 +476,7 @@ BOOST_AUTO_TEST_CASE(submit_range_of_tasks_complete_successfully)
 
     auto futures{std::move(*Assert(threadPool.Submit(std::move(array_tasks))))};
     BOOST_CHECK_EQUAL(futures.size(), static_cast<size_t>(num_tasks));
-    std::ranges::move(*Assert(threadPool.Submit(vector_tasks)), std::back_inserter(futures));
+    std::ranges::move(*Assert(threadPool.Submit(std::move(vector_tasks))), std::back_inserter(futures));
     BOOST_CHECK_EQUAL(futures.size(), static_cast<size_t>(num_tasks * 2));
 
     auto squares_sum{0};
diff --git a/src/util/threadpool.h b/src/util/threadpool.h
index 80971009ee..c3c11c53ac 100644
--- a/src/util/threadpool.h
+++ b/src/util/threadpool.h
@@ -216,7 +216,7 @@ public:
      *          uncaught exceptions, as they would otherwise be silently discarded.
      */
     template <std::ranges::sized_range R>
-        requires(!std::is_const_v<std::remove_reference_t<R>>)
+        requires(!std::is_lvalue_reference_v<R>)
     [[nodiscard]] util::Expected<std::vector<RangeFuture<R>>, SubmitError> Submit(R&& fns) noexcept EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
     {
         std::vector<RangeFuture<R>> futures;

Ran functional tests to affirm that the Assume() holds.

Changed back to keep task consumption explicit without broadening the API. Skipped the capacity check since Submit preserves the storage and assign() handles any allocation needed.

Still think it's worth adding the Assume() to catch potential changes in behavior of how Submit() uses the argument. Agree the comment was overkill as the reason for m_tasks being a member is already documented in the header. Not going to insist again though.

nit: We will now perform more work when we have blocks with only have a coinbase tx (previously we would not call ThreadPool::Submit(). In the case that bitcoin as a whole succeeds those blocks are outliers, so I agree with that part of the change.

However, the warning in the else-block becomes somewhat weird when m_tasks is empty (failing to submit 0 tasks). How about bringing the m_inputs.size() check back for that?

        } else if (m_inputs.size()) {
            // Submit can fail if a shared owner of the thread pool outside of this class calls Stop() or
            // Interrupt() on a different thread after we call WorkersCount() above. In that case parallel
            // fetching will not make progress, so we clear the inputs to fall back to single threaded fetching.
            LogWarning("Failed to submit prevout fetch tasks (%s); falling back to single-threaded fetching for this block.", SubmitErrorString(futures.error()));
            m_inputs.clear();
            StopFetching(); // Assert nothing changed if we failed to start tasks.
        }