[PATCH] D123225: [ThreadPool] add ability to group tasks into separate groups

[Alexandre Ganea via Phabricator via llvm-commits]

aganea added inline comments.


================
Comment at: llvm/include/llvm/Support/ThreadPool.h:117
+  typedef std::deque<std::pair<std::function<void()>, ThreadPoolTaskGroup *>>
+      TaskQueue;
+
----------------
This is a bit confusing, we already have a `llvm::TaskQueue`, can we change this to something else? Just `Queue` maybe?


================
Comment at: llvm/include/llvm/Support/ThreadPool.h:233
+  /// by calling ThreadPool::wait().
+  ~ThreadPoolTaskGroup() { wait(); }
+  /// Calls ThreadPool::async() for this group.
----------------
Would you mind please inserting a line break here, and after each other function below, just to match the style in this file?


================
Comment at: llvm/lib/Support/ThreadPool.cpp:54
+// The group of the task run by the current thread.
+static LLVM_THREAD_LOCAL ThreadPoolTaskGroup *CurrentThreadTaskGroup = nullptr;
+#endif
----------------
Shouldn't this be a stack since we're re-entering `processTasks()`? The following might not assert:
```
int main() {
  ThreadPool TP{hardware_concurrency(1)};
  ThreadPoolTaskGroup G1(TP);
  ThreadPoolTaskGroup G2(TP);
  G1.async([]{
    G2.wait(); // commenting this line would assert below.
    G1.wait(); // will deadlock without assert if the line above is there.
  });
  G2.async([]{
    // nop
  });
  return 0;
}
```


================
Comment at: llvm/lib/Support/ThreadPool.cpp:159
 bool ThreadPool::isWorkerThread() const {
   std::unique_lock<std::mutex> LockGuard(ThreadsLock);
   llvm::thread::id CurrentThreadId = llvm::this_thread::get_id();
----------------
I think there's a bug here, it was there before but now we're using `isWorkerThread()` a lot more. If the `ThreadPool` is shutting down, thus `ThreadLock` is locked in `ThreadPool::~ThreadPool()`, then if anyone calls `.wait()` in a managed thread, `isWorkerThread()` would deadlock. Probably better replacing with a `llvm::sys::RWMutex ThreadLock`, and use a `std::shared_lock<>` here instead.


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

https://reviews.llvm.org/D123225