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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D70447#1790286 <https://reviews.llvm.org/D70447#1790286>, @rnk wrote:

> I also think it's kind of reasonable to keep the Parallel.h APIs simple.


+1



================
Comment at: llvm/lib/Support/Parallel.cpp:137
+      ManagedExec;
+  static std::unique_ptr<ThreadPoolExecutor> Exec(&(*ManagedExec));
+  return Exec.get();
----------------
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?


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

https://reviews.llvm.org/D70447