[PATCH] D70447: [Support] ThreadPoolExecutor fixes for Windows/MinGW

[Andrew Ng via Phabricator via llvm-commits]

andrewng marked an inline comment as done.
andrewng added a comment.

In D70447#1792191 <https://reviews.llvm.org/D70447#1792191>, @mehdi_amini wrote:

> I'm not sure I totally agree with this: a global thread pool without preemption and priority inversion seems to be easily leading to deadlock through starvation. It is also breaking composability of library components since you may end-up enqueuing to the same pool from a thread in the pool.


There is already a "primitive" guard to prevent nesting of parallel invocations. Although this guard could also prevent other threads (not belonging to the "parallel" thread pool) from enqueuing parallel tasks.



================
Comment at: llvm/lib/Support/Parallel.cpp:137
+      ManagedExec;
+  static std::unique_ptr<ThreadPoolExecutor> Exec(&(*ManagedExec));
+  return Exec.get();
----------------
andrewng wrote:
> MaskRay wrote:
> > andrewng wrote:
> > > andrewng wrote:
> > > > rnk wrote:
> > > > > andrewng wrote:
> > > > > > rnk wrote:
> > > > > > > andrewng wrote:
> > > > > > > > ruiu wrote:
> > > > > > > > > OK, I had to take a look at ManagedStatic's class definition to understnad that ManagedStatic is a pointer-ish class which defines operator*.
> > > > > > > > > 
> > > > > > > > > ManagedStatic::operator* calls RegisterManagedStatic if it is not registered yet. So I guess you don't actually have to create a unique pointer, but instead you can just do `return &*ManagedExec;`.
> > > > > > > > As mentioned in the comment, the unique_ptr is used to actually destroy the ThreadPoolExecutor via the destructor which "stops" the ThreadPoolExecutor and also waits for the threads to complete.
> > > > > > > > 
> > > > > > > > The "destroy" of the ManagedStatic only "stops" the ThreadPoolExecutor and does not wait for the threads to complete.
> > > > > > > > 
> > > > > > > You don't truly need unique_ptr to set up an atexit call, all you need is an atexit callback that runs after the ManagedStatic is destroyed. I think the idiomatic way to do that is to make your own class with a ctor/dtor pair that saves a pointer to the ThreadPoolExecutor, and then calls the appropriate methods on it when it is destroyed. I think that will make it clearer that no memory deallocation is occuring.
> > > > > > With the unique_ptr, the ThreadPoolExecutor is destructed and its memory is deallocated. That was the intention and hence the use of unique_ptr.
> > > > > > 
> > > > > > Are you saying you would prefer that there is no memory deallocation of the ThreadPoolExecutor?
> > > > > I see, no, I think I just misunderstood.
> > > > > 
> > > > > I guess there is a use case here that we aren't handling, which is multiple LLVM sessions in one process, where LLVM is shutdown multiple times. The way this code is structured, the thread pool will be created, started, "stopped", and then it will remain stopped for all future sessions. Right now LLD is the only client, but this code is in Support, so it should be general. I see that it is not used by the rest of LLVM. Perhaps it should be moved back to LLD Core, so that we can make such assumptions about process lifetime.
> > > > Another solution would be to add support for "restarting" to this patch. It probably wouldn't be super efficient but it should be possible to handle "multiple shutdowns". Although it has to be said that "multiple shutdowns" just doesn't really sound right...
> > > There is potentially another alternative to using `ManagedStatic` / `llvm_shutdown()` by instead using `std::at_quick_exit()` along with `std::quick_exit()`. As far as I can tell, `std::quick_exit()` is equivalent to `_exit()`, except for the calling of the `std::at_quick_exit()` handlers.
> > > 
> > > Could this be a preferred option?
> > Can you elaborate how you'd use quick_exit and at_quick_exit?
> Instead of `ManagedStatic`, I would create a similar "holding" class that would hook into `std::quick_exit()` using `std::at_quick_exit()` to perform the "stop". However, this means that LLD would need to call `std::quick_exit()` rather than `_exit()`.
> 
> In my current prototyping of this alternative, I'm actually running into a new issue with the MSVC static run-time, in that I appear now to be intermittently losing output to `stderr` and thus getting random lit test failures for where LLD is expected to fail and output an error message. Note that in these situations, LLD is exiting in a thread from the `ThreadPoolExecutor`!
Another problem with `std::quick_exit()` / `std::at_quick_exit()` is that MinGW does not support it!


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

https://reviews.llvm.org/D70447