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

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 <

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

aganea marked an inline comment as done. aganea added inline comments. 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

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

sammccall added inline comments. Herald added subscribers: msifontes, jurahul, Kayjukh, stephenneuendorffer, aartbik. Herald added a project: MLIR. Comment at: clang-tools-extra/clangd/index/Background.cpp:154 assert(this->IndexStorageFactory && "Storage factory can not be

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

john.brawn added a comment. Herald added subscribers: frgossen, grosul1. Herald added a reviewer: MaskRay. With this patch the Threading.PhysicalConcurrency unit test fails when run with an affinity less than the number of physical cpus. I've raised https://bugs.llvm.org/show_bug.cgi?id=45556.

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

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:

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

aganea marked an inline comment as done. aganea 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

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

RobRich999 added a comment. I too can see how SMT might not afford much performance difference for LTO codegen. CMT appears to be more significant. I do not have exact numbers right now as my build box is busy, but the change added like an hour to locally building the Chromium browser with

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

mehdi_amini added a comment. There is something puzzling to me in this patch in the way the ThreadPool was modified: the client of the thread pool cannot request a number of threads anymore, but only a *maximum* number of threads. I don't really understand why? Using the OS reported

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

aganea added a comment. @thakis I think this is a side-effect of implementing `computeHostNumPhysicalCores()` for Windows, which previously returned -1, which in turn made `llvm::heavyweight_hardware_concurrency()` previously behave like `llvm::hardware_concurrency()` (on Windows only). At

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

thakis added a comment. https://bugs.chromium.org/p/chromium/issues/detail?id=1051578#c12 : """ FYI for those building on AMD Bulldozer family of processors and its various iterations after this commit: https://reviews.llvm.org/D71775 Building with ThinLTO on Bulldozer and similar appears to

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

This revision was automatically updated to reflect the committed changes. Closed by commit rG8404aeb56a73: [Support] On Windows, ensure hardware_concurrency() extends to all CPU sockets… (authored by aganea). Changed prior to commit: https://reviews.llvm.org/D71775?vs=237197=244664#toc

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

rriddle accepted this revision. rriddle added a comment. This revision is now accepted and ready to land. (Appeasing herald for MLIR changes). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71775/new/ https://reviews.llvm.org/D71775 ___

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

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. Herald added a reviewer: rriddle. Herald added a subscriber: Joonsoo. This revision now requires review to proceed. lgtm Comment at: llvm/lib/Support/Unix/Threading.inc:273 + +int

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

aganea added a comment. Herald added a reviewer: nicolasvasilache. Herald added a subscriber: liufengdb. Ping! Any further comments? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71775/new/ https://reviews.llvm.org/D71775 ___ cfe-commits

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

aganea added inline comments. Comment at: llvm/lib/Support/Unix/Threading.inc:273 + +int computeHostNumPhysicalThreads() { +#if defined(HAVE_SCHED_GETAFFINITY) && defined(HAVE_CPU_COUNT) rnk wrote: > I'm not sure it makes sense to say "physical threads". I think

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

aganea updated this revision to Diff 237197. aganea marked 13 inline comments as done. aganea added a comment. Herald added subscribers: lucyrfox, mgester, arpith-jacob, nicolasvasilache, antiagainst, shauheen, burmako, jpienaar, rriddle. Updated as suggested by @rnk. I've also removed

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

rnk added subscribers: inglorion, gbiv. rnk added a comment. Herald added a subscriber: george.burgess.iv. +@gbiv @inglorion, other ThinLTO users. > To solve this situation, we capture (and retain) the initial intention until > the point of usage, through a new ThreadPoolStrategy class. The

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

aganea added a comment. In D71775#1793767 , @mehdi_amini wrote: > > Will it make sense to say "I don't want hyper-threads" ? > > Not sure I remember correctly, but I believe one motivation behind avoiding > "hyper-threads" and other virtual cores was

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

mehdi_amini added a comment. > Will it make sense to say "I don't want hyper-threads" ? Not sure I remember correctly, but I believe one motivation behind avoiding "hyper-threads" and other virtual cores was that while they improve slightly the performance, they also increase the peak memory

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

mehdi_amini added a comment. Also: using heavyweight_hardware_concurrency() in the linker but having multiple linker jobs schedules by the build system was another reason (I think LLVM CMake default to 2 parallel link jobs when using ThinLTO for instance). Repository: rG LLVM Github

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

merge_guards_bot added a comment. {icon check-circle color=green} Unit tests: pass. 61057 tests passed, 0 failed and 728 were skipped. {icon times-circle color=red} clang-tidy: fail. Please fix clang-tidy findings

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

aganea created this revision. aganea added reviewers: mehdi_amini, rnk, tejohnson, russell.gallop, dexonsmith. Herald added subscribers: llvm-commits, cfe-commits, usaxena95, dang, jfb, kadircet, arphaman, steven_wu, jkorous, MaskRay, javed.absar, hiraditya, kristof.beyls, arichardson, emaste.