[clang-scan-deps] Improvements to thread usage

[saudi]

Summary

Switching back to std::thread to simplify the logic, as ThreadPool adds a layer of task management. However, keeping the ThreadPoolStrategy use, to benefit the ThreadPool improvements from https://reviews.llvm.org/D71775

This patch partly reverts https://reviews.llvm.org/D74569

Added a few optimizations:

• • Prevent from spawning more threads than there are files to process;
  • Perform computations on the main thread too, to save one thread spawn. This way, clang-scan-deps -j1 will be really single-threaded, and the code path is the same as when LLVM threads are disabled.
• Call reserve() to std::vectors which size is known when initializing.

[arphaman]

It might be good for @aganea to take a look as well.

[aganea]

In D102633#2769762, @arphaman wrote:

It might be good for @aganea to take a look as well.

Thanks! I actually work with @saudi, I already took a look at the patch before uploading. However I'm stil torn about running one of the workers on the main thread. I fear that we could have random errors because of the stack size of the "main" thread could be different from the stack size of the "satellite" threads. There's 99.99% chance that this won't happen, but I'd prefer that behavior to be explicit.

We could have:

-j0: use all hardware threads
-j1: don't use multi-threading, run all the tasks on the main thread
-jN: use the specified number of threads

The rationale is that we're using clang-scan-deps as-a-DLL in Fastbuild, to extract the dependencies. Since Fastbuild already has its own thread pool management, we call into clang-scan-deps with -j1 from different Fastbuild threads (but keeping the DependencyScanningService alive between calls). It would great if each call to clang-scan-deps wouldn't create a extra new thread.

Perhaps any of you would like to comment? +@mehdi_amini

[dexonsmith]

In D102633#2773578, @aganea wrote:

In D102633#2769762, @arphaman wrote:

It might be good for @aganea to take a look as well.

Thanks! I actually work with @saudi, I already took a look at the patch before uploading. However I'm stil torn about running one of the workers on the main thread. I fear that we could have random errors because of the stack size of the "main" thread could be different from the stack size of the "satellite" threads. There's 99.99% chance that this won't happen, but I'd prefer that behavior to be explicit.

We could have:

-j0: use all hardware threads
-j1: don't use multi-threading, run all the tasks on the main thread
-jN: use the specified number of threads

The rationale is that we're using clang-scan-deps as-a-DLL in Fastbuild, to extract the dependencies. Since Fastbuild already has its own thread pool management, we call into clang-scan-deps with -j1 from different Fastbuild threads (but keeping the DependencyScanningService alive between calls). It would great if each call to clang-scan-deps wouldn't create a extra new thread.

Perhaps any of you would like to comment? +@mehdi_amini

Seems like if you want/need a DLL you should be using a library interface not the tool interface. If you can't / don't want to use the C++ API in clang/lib/Tooling, then we should push out a C API. Maybe you can confirm that https://github.com/apple/llvm-project/blob/apple/main/clang/include/clang-c/Dependencies.h in libclang will work for your needs? It landed on the fork just because it was experimental and libclang has a stable API promise, but I think the names of the functions make that clear enough -- e.g., clang_experimental_DependencyScannerWorker_create_v0 seems well-enough named that no one expects permanent compatibility -- and it seems pretty awkward to be using the tool as a DLL to work around not having this in tree. Then your FastBuild can directly manage the service and the workers. WDYT?

@arphaman / @Bigcheese / @jansvoboda11 , thoughts?

[aganea]

@dexonsmith Yes, using the Clang Tooling C++ API seems like a good option, however some logic could/would be needed from clang/tools/clang-scan-deps/ClangScanDeps.cpp. Using clang-scan-deps as-a-DLL seemed like the best option on the short term, since we're using its main() almost verbatim with only a few minor changes. We still need to solve a contention issue related to the Windows implementation of sys::fs::status(), but then once everything is sorted out we'll re-consider the usage of the DLL.

In the meanwhile, do you have any opinion on this patch and/or on https://reviews.llvm.org/D102633#2773578 ? Do you think there's value for users?

[saudi]

In D102633#2786288, @aganea wrote:

@dexonsmith Yes, using the Clang Tooling C++ API seems like a good option, however some logic could/would be needed from clang/tools/clang-scan-deps/ClangScanDeps.cpp. Using clang-scan-deps as-a-DLL seemed like the best option on the short term, since we're using its main() almost verbatim with only a few minor changes. We still need to solve a contention issue related to the Windows implementation of sys::fs::status(), but then once everything is sorted out we'll re-consider the usage of the DLL.

Abandonning this patch, since we ended up creating a separate tool based on ClangScanDeps, modified and simplified for our needs: builds as a DLL, no -j # support as the main function is itself called from different threads, etc...

clang/test/ClangScanDeps/Inputs/num-workers-1cmd.json

+[
+{
+  "directory": "DIR",
+  "command": "clang -E -fsyntax-only DIR/file.cpp",
+  "file": "DIR/file.cpp"
+}
+]

clang/test/ClangScanDeps/Inputs/num-workers-2cmds.json

+[
+{
+  "directory": "DIR",
+  "command": "clang -E -fsyntax-only DIR/file.cpp",
+  "file": "DIR/file.cpp"
+},
+{
+  "directory": "DIR",
+  "command": "clang -E -fsyntax-only DIR/file2.cpp",
+  "file": "DIR/file2.cpp"
+}
+]

clang/test/ClangScanDeps/num-workers.cpp

+// REQUIRES: thread_support
+// RUN: rm -rf %t.dir
+// RUN: rm -f %t.*.cdb
+// RUN: mkdir -p %t.dir
+// RUN: cp %s %t.dir/file.cpp
+// RUN: cp %s %t.dir/file2.cpp
+// RUN: echo > %t.dir/header.h
+
+// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/num-workers-1cmd.json > %t.1cmd.cdb
+// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/num-workers-2cmds.json > %t.2cmds.cdb
+//
+// RUN: clang-scan-deps -compilation-database %t.1cmd.cdb -j 2 -v | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK1F1W %s
+//
+// RUN: clang-scan-deps -compilation-database %t.2cmds.cdb -j 1 -v | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK2F1W %s
+//
+// RUN: clang-scan-deps -compilation-database %t.2cmds.cdb -j 2 -v | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK2F2W %s
+//
+// RUN: clang-scan-deps -compilation-database %t.2cmds.cdb -j 10 -v | \
+// RUN:   FileCheck --check-prefixes=CHECK,CHECK2F2W %s
+
+#include "header.h"
+
+// CHECK1F1W: Running clang-scan-deps on 1 files using 1 workers
+// CHECK2F1W: Running clang-scan-deps on 2 files using 1 workers
+// CHECK2F2W: Running clang-scan-deps on 2 files using 2 workers
+
+// CHECK: file.cpp
+// CHECK-NEXT: header.h

clang/tools/clang-scan-deps/ClangScanDeps.cpp

 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileUtilities.h"
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/JSON.h"
 #include "llvm/Support/Program.h"
 #include "llvm/Support/Signals.h"
-#include "llvm/Support/ThreadPool.h"
 #include "llvm/Support/Threading.h"
 #include <mutex>
 #include <thread>
 
 using namespace clang;
 using namespace tooling::dependencies;
 
 namespace {
AdjustingCompilations->appendArgumentsAdjuster(
       tooling::getClangStripSerializeDiagnosticAdjuster());
 
   SharedStream Errs(llvm::errs());
   // Print out the dependency results to STDOUT by default.
   SharedStream DependencyOS(llvm::outs());
 
   DependencyScanningService Service(ScanMode, Format, ReuseFileManager,
                                     SkipExcludedPPRanges);
-  llvm::ThreadPool Pool(llvm::hardware_concurrency(NumThreads));
-  std::vector<std::unique_ptr<DependencyScanningTool>> WorkerTools;
-  for (unsigned I = 0; I < Pool.getThreadCount(); ++I)
-    WorkerTools.push_back(std::make_unique<DependencyScanningTool>(Service));
 
   std::vector<SingleCommandCompilationDatabase> Inputs;
-  for (tooling::CompileCommand Cmd :
-       AdjustingCompilations->getAllCompileCommands())
+  {
+    auto AdjustedCommands = AdjustingCompilations->getAllCompileCommands();
+    Inputs.reserve(AdjustedCommands.size());
+    for (tooling::CompileCommand Cmd : AdjustedCommands)
       Inputs.emplace_back(Cmd);
+  }
 
+  unsigned NumWorkers =
+      llvm::hardware_concurrency(NumThreads).compute_thread_count();
+
+  if (NumWorkers > Inputs.size())
+    NumWorkers = static_cast<unsigned>(Inputs.size());
+
+  std::vector<std::unique_ptr<DependencyScanningTool>> WorkerTools;
+  WorkerTools.reserve(NumWorkers);
+  for (unsigned I = 0; I < NumWorkers; ++I)
+    WorkerTools.push_back(std::make_unique<DependencyScanningTool>(Service));
+
+  std::vector<std::thread> WorkerThreads;
+  if (NumWorkers > 1)
+    WorkerThreads.reserve(NumWorkers - 1);
   std::atomic<bool> HadErrors(false);
   FullDeps FD;
   std::mutex Lock;
   size_t Index = 0;
 
   if (Verbose) {
     llvm::outs() << "Running clang-scan-deps on " << Inputs.size()
-                 << " files using " << Pool.getThreadCount() << " workers\n";
+                 << " files using " << NumWorkers << " workers\n";
   }
-  for (unsigned I = 0; I < Pool.getThreadCount(); ++I) {
-    Pool.async([I, &Lock, &Index, &Inputs, &HadErrors, &FD, &WorkerTools,
+  for (unsigned I = 0; I < NumWorkers; ++I) {
+    auto Worker = [I, &Lock, &Index, &Inputs, &HadErrors, &FD, &WorkerTools,
                    &DependencyOS, &Errs]() {
       llvm::StringSet<> AlreadySeenModules;
       while (true) {
         const SingleCommandCompilationDatabase *Input;
         std::string Filename;
         std::string CWD;
         size_t LocalIndex;
         // Take the next input.
         {
auto Worker = [I, &Lock, &Index, &Inputs, &HadErrors, &FD, &WorkerTools,
         } else {
           auto MaybeFullDeps = WorkerTools[I]->getFullDependencies(
               *Input, CWD, AlreadySeenModules);
           if (handleFullDependencyToolResult(Filename, MaybeFullDeps, FD,
                                              LocalIndex, DependencyOS, Errs))
             HadErrors = true;
         }
       }
-    });
+    };
+    // Leave the last worker for the main thread.
+    if (I < NumWorkers - 1) {
+      assert(llvm::llvm_is_multithreaded());
+      WorkerThreads.emplace_back(std::move(Worker));
+    } else {
+      Worker();
+    }
   }
-  Pool.wait();
+  for (auto &W : WorkerThreads)
+    W.join();
 
   if (Format == ScanningOutputFormat::Full)
     FD.printFullOutput(llvm::outs());
 
   return HadErrors;
 }