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

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a subscriber: MaskRay.
rnk added a comment.

+ at maskray



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


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

https://reviews.llvm.org/D70447