[PATCH] D78408: [llvm-cov] Prevent llvm-cov from using too many threads

[Alexandre Ganea via Phabricator via llvm-commits]

aganea marked 4 inline comments as done.
aganea added inline comments.


================
Comment at: llvm/lib/Support/Threading.cpp:88
   int MaxThreadCount = UseHyperThreads ? computeHostNumHardwareThreads()
                                        : sys::getHostNumPhysicalCores();
   if (MaxThreadCount <= 0)
----------------
MaskRay wrote:
> Note that `sys::getHostNumPhysicalCores` can return -1 in some cases (on some OS/arch). In such cases we will return `1` if `ThreadsRequested == 0`.
> 
> I still think that if the user asks for more threads than the CPU supports, we should respect that. For one thing, not every task can fully leverage 100% of the computing power of a core.
Sorry I'm a bit slow. I completely agree with "//if the user asks for more threads than the CPU supports, we should respect that//", but I don't understand in which case this isn't satisfied as it stands?

`ThreadsRequested == 0` means 'use the default', which is `MaxThreadCount`. Should we return `std::thread::hardware_concurrency()` if `MaxThreadCount == 1`?

If the user asks anything > 0, currently we will satisfy that requirement. Except when we want don't want to automatically spawn more than `MaxThreadCount` (thus the new `Limit` variable). Note, `Limit = false` by default.


================
Comment at: llvm/tools/llvm-cov/CodeCoverage.cpp:950
+    // the number of hardware cores.
+    S = heavyweight_hardware_concurrency(SourceFiles.size());
+    S.Limit = true;
----------------
I can also do `S.ThreadsRequested = std::min(SourceFiles.size(), heavyweight_hardware_concurreny().compute_thread_count());` here instead of the `.Limit` logic. But then the code becomes more difficult to read I find.


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

https://reviews.llvm.org/D78408