[PATCH] D115019: ThreadPool: grow the pool only as needed

[Benoit Jacob via Phabricator via llvm-commits]

Benoit added inline comments.


================
Comment at: llvm/include/llvm/Support/ThreadPool.h:68-70
+  // Returns the overall number of threads, which is 1 (main thread) plus the
+  // number of worker Threads.
+  unsigned getThreadCount() const { return Threads.size() + 1; }
----------------
Benoit wrote:
> Please decide what we want here? The immediate problem that led me to add +1 here was that when Threads was empty, I was now returning 0, and the caller was clearly expecting at least 1:
> 
> At mlir/include/mlir/IR/Threading.h:75
> 
> ```
>   size_t numActions = std::min(numElements, threadPool.getThreadCount());
>   SmallVector<std::shared_future<void>> threadFutures;
>   threadFutures.reserve(numActions - 1);
>   for (unsigned i = 1; i < numActions; ++i)
>     threadFutures.emplace_back(threadPool.async(processFn));
> ```
> 
Self-replying: this call site really shows that `getThreadCount` was expected to return the potential max number of threads, which incidentally was equal to the current number of threads before this diff, but that's what is changing here.  To avoid breaking existing users, I reverted `getThreadCount` to this behavior  (I believe that the earlier state of this diff would have effectively kept the number of threads to 1. Now  I've checked in GDB that we do create a dozen threads for a simple lit test).


================
Comment at: llvm/lib/Support/ThreadPool.cpp:24
+void ThreadPool::grow() {
+  if (getThreadCount() >= Strategy.compute_thread_count()) {
+    // Already hit the max thread pool size.
----------------
Benoit wrote:
> Here actually I'm really not sure: what should be the max number of threads in `Threads`:  should it be `Strategy.compute_thread_count()` or should it be that minus one to account for the main thread?  it looks like the existing code was doing the former but (as actually confirmed by GDB) that meant it had one more thread running than the detected hardware concurrency.  Which could be fine if we expect that the main thread would be idle most of the time? What was the intent here?
Got it, nevermind, updated this diff (see reply to the other comment thread).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115019/new/

https://reviews.llvm.org/D115019