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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

I've sent https://reviews.llvm.org/D82352 to clean up some of the logic in clangd.



================
Comment at: clang-tools-extra/clangd/index/Background.cpp:154
   assert(this->IndexStorageFactory && "Storage factory can not be null!");
-  for (unsigned I = 0; I < ThreadPoolSize; ++I) {
+  for (unsigned I = 0; I < Rebuilder.TUsBeforeFirstBuild; ++I) {
     ThreadPool.runAsync("background-worker-" + llvm::Twine(I + 1), [this] {
----------------
aganea wrote:
> sammccall wrote:
> > Hmm, I finally stumbled across this today when editing the rebuild policy.
> > I do wish this hadn't been changed without understanding what this code is doing or getting review from an owner.
> > 
> > After this change, modifying the background-index rebuild frequency (which was initially tuned to be roughly "after each thread built one file") has the side-effect of changing the number of threads used for background indexing!
> > 
> > Less seriously, the use of zero to mean "all the threads" is problematic here because in the other thread roots in this project we use zero to mean "no threads, work synchronously".
> > 
> > I'll look into reverting the clangd parts of this patch.
> Sorry about that Sam. Do you think 'one' could be used in clangd instead? That is the common value used across other parts of LLVM to signify 'no threads', and also when `LLVM_ENABLE_THREADS` is off. 'zero' means to use the default settings for the thread strategy. That is, `llvm::hardware_concurrency(0)` means to use all hardware threads; or `llvm::heavyweight_hardware_concurrency(0)` means to use all hardware cores, but only one `std::thread` per core.
No worries, it happens and nothing came of it except a little head-scratching. Sorry for being grumpy, I shouldn't send email late at night...

> Do you think 'one' could be used in clangd instead? That is the common value used across other parts of LLVM to signify 'no threads', and also when LLVM_ENABLE_THREADS is off

One can't be used here, it means "spawn one background thread". (Clangd has several distinct components that spawn threads).

FWIW, clangd can't be built without LLVM_ENABLE_THREADS - it needs concurrency to be useful, and we designed around threads. Most of the threading can be turned off *at runtime* for certain tests (that's what a threadpool size of zero means) but not the background index - we just disable it instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71775