[Support][Parallel] ThreadPoolExecutor fixes for Windows

[andrewng]

Summary

• Disable use of MS ConcRT as it can cause crashes on exit, particularly with the MSVC static debug CRT.

• Changed ThreadPoolExecutor to not use detached threads but instead to join threads where possible on destruction. This is to prevent random crashing on Windows when doing a full exit, e.g. via exit().

• Changed ThreadPoolExecutor to be a ManagedStatic so that it can be shutdown on llvm_shutdown(). Otherwise this could only occur when doing a full exit, e.g. via exit(). This is required to avoid a race condition on Windows between the ThreadPoolExecutor still starting up threads and the process doing a fast exit, e.g. via _exit().

[zturner]

I absolutely do not want to disable support for ConcRT under any circumstances unless we find that it is a completely broken library, which I highly doubt. Even then, I would want to ask Microsoft to fix it.

What is the exact problem you're running into?

[andrewng]

In D40366#933070, @zturner wrote:

I absolutely do not want to disable support for ConcRT under any circumstances unless we find that it is a completely broken library, which I highly doubt. Even then, I would want to ask Microsoft to fix it.

What is the exact problem you're running into?

Hi, sorry, I thought that this would be a "private" patch. Don't worry, I do not intend to make any changes without proper review!

I created this patch to see if it might help with a problem being seen on Linux, in the message thread "std::thread::detach() rarely crashes in Parallel.cpp".

The reason I've been looking into this is as mentioned in the summary. The MS Concurrency Run-time can cause exceptions at exit, particularly when used with the MSVC static debug CRT, which in turn causes LLD to intermittently crash on exit. We have already submitted a bug to Microsoft, but no progress has been made. We also discovered whilst researching the issue, that the MSVC STL used to use the MS ConcRT but no longer does...

Locally, we've been running with MS ConcRT disabled, as we need to use the MSVC static CRT's.

[zturner]

In D40366#933119, @andrewng wrote:

In D40366#933070, @zturner wrote:

I absolutely do not want to disable support for ConcRT under any circumstances unless we find that it is a completely broken library, which I highly doubt. Even then, I would want to ask Microsoft to fix it.

What is the exact problem you're running into?

Hi, sorry, I thought that this would be a "private" patch. Don't worry, I do not intend to make any changes without proper review!

Ahh, sorry about that. One of the Phabricator triggers must have added me automatically.

I created this patch to see if it might help with a problem being seen on Linux, in the message thread "std::thread::detach() rarely crashes in Parallel.cpp".

The reason I've been looking into this is as mentioned in the summary. The MS Concurrency Run-time can cause exceptions at exit, particularly when used with the MSVC static debug CRT, which in turn causes LLD to intermittently crash on exit. We have already submitted a bug to Microsoft, but no progress has been made.

Can you link me to the connect issue?

We also discovered whilst researching the issue, that the MSVC STL used to use the MS ConcRT but no longer does...

At least on my local MSVC 2017 installation, it's still used in <future> and <mutex>. We don't have an implementation of Parallel TS yet from Microsoft, but I would imagine that's going to use it behind the scenes. If their plan is to deprecate it, they're definitely being quiet about it :)

[labath]

In D40366#933119, @andrewng wrote:

In D40366#933070, @zturner wrote:

I absolutely do not want to disable support for ConcRT under any circumstances unless we find that it is a completely broken library, which I highly doubt. Even then, I would want to ask Microsoft to fix it.

What is the exact problem you're running into?

Hi, sorry, I thought that this would be a "private" patch. Don't worry, I do not intend to make any changes without proper review!

I created this patch to see if it might help with a problem being seen on Linux, in the message thread "std::thread::detach() rarely crashes in Parallel.cpp".

I have no idea about windows, but for the linux issue, you should look into whether you're not running into https://sourceware.org/bugzilla/show_bug.cgi?id=19951. We've ran into that in LLDB, where we also spawn a ton of detached threads. If you can send me some core dumps or steps to reproduce I may be able to help you with diagnosing that. If this is indeed the same bug it is fairly easy to work around it without ditching detached threads. If you can send me a core dump or steps to reproduce I may be able to help you with diagnosing that.

[andrewng]

Latest version of this patch is now in D70447.

include/llvm/Support/Parallel.h

 #include "llvm/Config/llvm-config.h"
 #include "llvm/Support/MathExtras.h"
 
 #include <algorithm>
 #include <condition_variable>
 #include <functional>
 #include <mutex>
 
-#if defined(_MSC_VER) && LLVM_ENABLE_THREADS
+#if defined(_MSC_VER) && LLVM_ENABLE_THREADS && 0 // Disable use of MS ConcRT.
 #pragma warning(push)
 #pragma warning(disable : 4530)
 #include <concrt.h>
 #include <ppl.h>
 #pragma warning(pop)
 #endif
 
 namespace llvm {
class TaskGroup {
   Latch L;
 
 public:
   void spawn(std::function<void()> f);
 
   void sync() const { L.sync(); }
 };
 
-#if defined(_MSC_VER)
+#if defined(_MSC_VER) && 0 // Disable use of MS ConcRT.
 template <class RandomAccessIterator, class Comparator>
 void parallel_sort(RandomAccessIterator Start, RandomAccessIterator End,
                    const Comparator &Comp) {
   concurrency::parallel_sort(Start, End, Comp);
 }
 template <class IterTy, class FuncTy>
 void parallel_for_each(IterTy Begin, IterTy End, FuncTy Fn) {
   concurrency::parallel_for_each(Begin, End, Fn);

lib/Support/Parallel.cpp

 //===- llvm/Support/Parallel.cpp - Parallel algorithms --------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
+#include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/Parallel.h"
 #include "llvm/Config/llvm-config.h"
 #include "llvm/Support/Threading.h"
 
 #include <atomic>
 #include <stack>
 #include <thread>
+#include <vector>
 
 using namespace llvm;
 
 namespace {
 
 /// \brief An abstract class that takes closures and runs them asynchronously.
 class Executor {
 public:
public:
   virtual void add(std::function<void()> F) { F(); }
 };
 
 Executor *Executor::getDefaultExecutor() {
   static SyncExecutor Exec;
   return &Exec;
 }
 
-#elif defined(_MSC_VER)
+#elif defined(_MSC_VER) && 0 // Disable use of MS ConcRT.
 /// \brief An Executor that runs tasks via ConcRT.
 class ConcRTExecutor : public Executor {
   struct Taskish {
     Taskish(std::function<void()> Task) : Task(Task) {}
 
     std::function<void()> Task;
 
     static void run(void *P) {
Executor *Executor::getDefaultExecutor() {
   return &exec;
 }
 
 #else
 /// \brief An implementation of an Executor that runs closures on a thread pool
 ///   in filo order.
 class ThreadPoolExecutor : public Executor {
 public:
-  explicit ThreadPoolExecutor(unsigned ThreadCount = hardware_concurrency())
-      : Done(ThreadCount) {
+  explicit ThreadPoolExecutor(unsigned ThreadCount = hardware_concurrency()) {
     // Spawn all but one of the threads in another thread as spawning threads
     // can take a while.
-    std::thread([&, ThreadCount] {
-      for (size_t i = 1; i < ThreadCount; ++i) {
-        std::thread([=] { work(); }).detach();
+    Threads.reserve(ThreadCount);
+    Threads.resize(1);
+    Threads[0] = std::thread([&, ThreadCount] {
+      for (unsigned i = 1; i < ThreadCount; ++i) {
+        Threads.emplace_back([=] { work(); });
+        if (Stop)
+          break;
       }
+
+      std::unique_lock<std::mutex> Lock(Mutex);
+      Started = true;
+      Lock.unlock();
+      CondStarted.notify_one();
+
       work();
-    }).detach();
+    });
   }
 
-  ~ThreadPoolExecutor() override {
+  void shutdown() {
     std::unique_lock<std::mutex> Lock(Mutex);
+    if (Stop)
+      return;
+
     Stop = true;
     Lock.unlock();
     Cond.notify_all();
-    // Wait for ~Latch.
+
+    Lock.lock();
+    if (!Started)
+      CondStarted.wait(Lock);
   }
 
+  ~ThreadPoolExecutor() override {
+    shutdown();
+
+    std::thread::id ThreadId = std::this_thread::get_id();
+    for (std::thread &T : Threads) {
+      if (T.get_id() == ThreadId)
+        T.detach();
+      else
+        T.join();
+    }
+  }
+
+  struct Creator {
+    static void *call() {
+      return new ThreadPoolExecutor();
+    }
+  };
+
+  struct Deleter {
+    static void call(void *Ptr) {
+      ((ThreadPoolExecutor *) Ptr)->shutdown();
+    }
+  };
+
   void add(std::function<void()> F) override {
     std::unique_lock<std::mutex> Lock(Mutex);
     WorkStack.push(F);
     Lock.unlock();
     Cond.notify_one();
   }
 
 private:
   void work() {
     while (true) {
       std::unique_lock<std::mutex> Lock(Mutex);
       Cond.wait(Lock, [&] { return Stop || !WorkStack.empty(); });
       if (Stop)
         break;
       auto Task = WorkStack.top();
       WorkStack.pop();
       Lock.unlock();
       Task();
     }
-    Done.dec();
   }
 
   std::atomic<bool> Stop{false};
+  std::atomic<bool> Started{false};
   std::stack<std::function<void()>> WorkStack;
   std::mutex Mutex;
   std::condition_variable Cond;
-  parallel::detail::Latch Done;
+  std::condition_variable CondStarted;
+  std::vector<std::thread> Threads;
 };
 
 Executor *Executor::getDefaultExecutor() {
-  static ThreadPoolExecutor exec;
-  return &exec;
+  // The ManagedStatic enables the ThreadPoolExecutor to be shutdown via
+  // llvm_shutdown() which allows a "clean" fast exit, e.g. via _exit(). The
+  // shutdown tells the thread pool to stop and waits for any worker thread
+  // creation to complete. However, it does not wait for the threads to finish.
+  // The wait for worker thread creation to complete is important as it prevents
+  // intermittent crashes on Windows due to a race condition between thread
+  // creation and process exit.
+  //
+  // The ThreadPoolExecutor itself will only be destroyed when the static
+  // unique_ptr to it is destroyed, i.e. in a normal full exit. The
+  // ThreadPoolExecutor destructor ensures shutdown and also waits for worker
+  // threads to finish. The wait is important as it prevents intermittent
+  // crashes on Windows when the process is doing a full exit.
+  //
+  // The Windows crashes are particularly noticeable when using the static debug
+  // CRT.
+  static ManagedStatic<ThreadPoolExecutor, ThreadPoolExecutor::Creator,
+                       ThreadPoolExecutor::Deleter>
+      ManagedExec;
+  static std::unique_ptr<ThreadPoolExecutor> Exec(&(*ManagedExec));
+  return Exec.get();
 }
 #endif
 }
 
 #if LLVM_ENABLE_THREADS
 void parallel::detail::TaskGroup::spawn(std::function<void()> F) {
   L.inc();
   Executor::getDefaultExecutor()->add([&, F] {
     F();
     L.dec();
   });
 }
 #endif