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

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

I raised the question of merging the thread pool implementations if possible because this code is subtle and hard to understand, and I was hoping I could get out of understanding it by reusing some code that already works. Now that I look at the other thread pool, I see it doesn't manage a global thread pool instance, so it doesn't suffer from the shutdown bugs you are running into. I will chalk that up as a win for not using global variables. =P

Given that this code is already written to use a global default executor instance, I think your proposal as I understand it makes sense:

- Create the threadpool lazily on first access with static locals.
- Stop the threads on llvm_shutdown if and when that is called, but do not deallocate any memory.
- Deallocate the memory if and when the image is unloaded via a static unique_ptr.

I see that LLD today calls llvm_shutdown to destroy ManagedStatics:
https://github.com/llvm/llvm-project/blob/master/lld/Common/ErrorHandler.cpp#L59
Please update that comment to indicate that this will also stop any threads that have been spawned, and this is critical for avoiding crashes on shutdown. It's actually a waste of time to deallocate other ManagedStatics, so someone might try to remove this at some point.

However, I wonder if LLD (and any other hypothetical clients of Parallel.h) should explicitly cancel, join, or detach threads during shutdown instead.


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

https://reviews.llvm.org/D70447