[PATCH] D71775: [ThreadPool] On Windows, extend usage to all CPU sockets and all NUMA groups

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: llvm/lib/Support/Threading.cpp:94
+  // uselessly induce more context-switching and cache eviction.
+  if (!ThreadsRequested || ThreadsRequested > (unsigned)MaxThreadCount)
+    return MaxThreadCount;
----------------
aganea wrote:
> @mehdi_amini You mean this? Testing was showing degraded performance if ThreadsRequested > MaxThreadCount, so I thought it'd maybe better to prevent that situation. More soft threads than hardware threads means you'd pay for context switching, for the cache eviction and for extra memory pressure (even more if the allocator has per-thread pools).
> Do you see a cases where not having this test would be valuable? Providing `--threads=50`, and creating 50-threads ThreadPool when your CPU only supports 12 hardware threads? The only case I can think of is usage of async operations in threads, but then your ThreadPool sounds more like a queue, and maybe it's not the right hammer for the nail? Should we support that case and explicitly tag the ThreadPool constructor in client code with something like ThreadPool(50, AcknowledgeDegradedPerformance)?
>  Testing was showing degraded performance

I don't understand why this is relevant to anything else than the default? This is a library and if the caller override the default then IMO this layer should just let it be!

The current issue with bulldozer is a perfect example of this: the user is *requesting* more threads: why wouldn't you honor this request? If they want to shoot themselves in the foot, I'd let them (in most cases the user would know something you don't, like here).

I don't think we need the extra `AcknowledgeDegradedPerformance` flag, if the client override the default they can be assumed to know what they're doing (or they shouldn't use such an API in the first place), this seems more in line with the rest of the system (and C++ API in general I believe).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71775