[llvm-cov] Prevent llvm-cov from using too many threads

[aganea]

Summary

As requested here: https://reviews.llvm.org/D75153#1987272

Before, each instance of llvm-cov was creating one thread per hardware core, which wasn't needed probably because the number of inputs were small. This was probably causing a thread rlimit issue on large core count systems.

After this patch, the previous behavior is restored (to what was before rG8404aeb5):

• If --num-threads is not specified, we create one thread per input, up to num.cores.
• When specified, --num-threads indicates any number of threads, with no upper limit.

Petr, could you please confirm that this patch fixes the issue?

[MaskRay]

I think we should respect ThreadsRequested if it is greater than 0.

For the particular llvm-cov issue, we can probably restore the previous behavior:

NumThreads =
    std::max(1U, std::min(llvm::heavyweight_hardware_concurrency(),
                          unsigned(SourceFiles.size())));
NumThreads = SourceFiles.size();

[aganea]

In D78408#1990302, @MaskRay wrote:

I think we should respect ThreadsRequested if it is greater than 0.

Nothing is changed in that regard. Only when .Limit is set.

In D78408#1990302, @MaskRay wrote:

For the particular llvm-cov issue, we can probably restore the previous behavior:

I cannot restore the code exactly how it was before because of the reasons mentioned in rG8404aeb5.
I rewrote it in a way which is simpler to understand. I also fixed the other places that suffered from the same issue.

A better fix could be to lazily create threads in the ThreadPool when jobs are pushed through async(). This issue here is that we are currently creating too many threads by default, whereas before we were limiting to one thread per input, given a small num.inputs .

[aganea]

Typos.

[MaskRay]

llvm/lib/Support/Threading.cpp

88

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.

[aganea]

llvm/lib/Support/Threading.cpp

88

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.

llvm/tools/llvm-cov/CodeCoverage.cpp

953

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.

[MaskRay]

+@friss @aprantl @JDevlieghere for dsymutil changes

[JDevlieghere]

The dsymutil part looks fine, I don't expect there to be many cases where we have more architectures than cores :-)

[aganea]

Ping! This seemed to be a blocker for @phosek.

[phosek]

This LGTM but I haven't yet been able to verify this on our side, but that's because getting a toolchain with a custom patch used by our coverage infrastructure is something we don't support right now and I still haven't managed to get that working (the issue only reproduces on our bots, I haven't managed to reproduce that issue locally on my workstation). I'd be fine landing this and then follow up if there are any issues once we roll this in.

[aganea]

In D78408#1997212, @phosek wrote:

[...] and I still haven't managed to get that working (the issue only reproduces on our bots, I haven't managed to reproduce that issue locally on my workstation)

The issue is clearly related to the number of cores in the system, the amount of instances of LLVM exes that are running at once, and rlimits. Does your workstation have the same hardware config as the bots?

[MaskRay]

LGTM. I was waiting on people's response whether the addition bool Limit = false (it increased the complexity of the interface a bit) is necessary. It seems that people are fine with it.

[phosek]

In D78408#1997273, @aganea wrote:

In D78408#1997212, @phosek wrote:

[...] and I still haven't managed to get that working (the issue only reproduces on our bots, I haven't managed to reproduce that issue locally on my workstation)

The issue is clearly related to the number of cores in the system, the amount of instances of LLVM exes that are running at once, and rlimits. Does your workstation have the same hardware config as the bots?

I have finally manage to do a full run on our bots and I can confirm that this addresses the issue.

[phosek]

Can we land this?

llvm/include/llvm/Support/Threading.h

public:
     // a suggested high bound, the runtime might choose a lower value (not
     // higher).
     unsigned ThreadsRequested = 0;
 
     // If SMT is active, use hyper threads. If false, there will be only one
     // std::thread per core.
     bool UseHyperThreads = true;
 
+    // Sets an upper bound to the number of hardware threads, or hardware cores.
+    bool Limit = false;
+
     /// Retrieves the max available threads for the current strategy. This
     /// accounts for affinity masks and takes advantage of all CPU sockets.
     unsigned compute_thread_count() const;
 
     /// Assign the current thread to an ideal hardware CPU or NUMA node. In a
     /// multi-socket system, this ensures threads are assigned to all CPU
     /// sockets. \p ThreadPoolNum represents a number bounded by [0,
     /// compute_thread_count()).

llvm/lib/Support/Threading.cpp

 }
 #endif
 
 #else
 
 int computeHostNumHardwareThreads();
 
 unsigned llvm::ThreadPoolStrategy::compute_thread_count() const {
-  if (ThreadsRequested > 0)
-    return ThreadsRequested;
-
   int MaxThreadCount = UseHyperThreads ? computeHostNumHardwareThreads()
                                        : sys::getHostNumPhysicalCores();
   if (MaxThreadCount <= 0)
     MaxThreadCount = 1;
+
+  if (ThreadsRequested == 0)
     return MaxThreadCount;
+
+  return Limit ? std::min((unsigned)MaxThreadCount, ThreadsRequested)
+               : ThreadsRequested;
 }
 
 namespace {
 struct SyncThreadInfo {
   void (*UserFn)(void *);
   void *UserData;
 };
 

llvm/tools/llvm-cov/CodeCoverage.cpp

int CodeCoverageTool::doShow(int argc, const char **argv,
   // Show files
   bool ShowFilenames =
       (SourceFiles.size() != 1) || ViewOpts.hasOutputDirectory() ||
       (ViewOpts.Format == CoverageViewOptions::OutputFormat::HTML);
 
   auto NumThreads = ViewOpts.NumThreads;
 
   // If NumThreads is not specified, auto-detect a good default.
-  if (NumThreads == 0)
-    NumThreads = SourceFiles.size();
+  ThreadPoolStrategy S;
+  if (NumThreads > 0)
+    S = hardware_concurrency(NumThreads); // no upper limit on the # of threads
+  else {
+    S = heavyweight_hardware_concurrency(SourceFiles.size());
+    S.Limit = true; // limit to max hardware cores
+  }
 
   if (!ViewOpts.hasOutputDirectory() || NumThreads == 1) {
     for (const std::string &SourceFile : SourceFiles)
       writeSourceFileView(SourceFile, Coverage.get(), Printer.get(),
                           ShowFilenames);
   } else {
     // In -output-dir mode, it's safe to use multiple threads to print files.
-    ThreadPool Pool(heavyweight_hardware_concurrency(NumThreads));
+    ThreadPool Pool(S);
     for (const std::string &SourceFile : SourceFiles)
       Pool.async(&CodeCoverageTool::writeSourceFileView, this, SourceFile,
                  Coverage.get(), Printer.get(), ShowFilenames);
     Pool.wait();
   }
 
   return 0;
 }