[clang][deps] NFC: Simplify worker loop

[jansvoboda11]

Summary

This patch simplifies the loop inside each worker by extracting index retrieval into a lambda function.

[benlangmuir]

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

804

Why are we second-guessing the ThreadPool at all? I would think we should do

for (unsigned Index = 0; E = Inputs.size(); Index + E; ++Index) {
  Pool.async([Index, &]{ ... });
}

Then the thread pool is responsible for dispatching the tasks when it has available resources instead of us manually looping inside the threads.

[jansvoboda11]

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

804

I guess it was originally done this way because we have a number of DependencyScanning{Tool,Worker} instances (the same number as there are threads in the pool) we want to reuse to get the advantage of local caching in DependencyScanningWorkerFilesystem. I guess having one Worker instance "pinned" to one thread might get us better memory access patterns? (I'm just guessing here.)

We could implement your simplification and keep the "pinning" if the thread pool gave us index of the thread the async task is running on. But it doesn't seem like we have that API.

That means we'd probably need to implement some Worker queue each async task would take from. I'm not sure that's better/simpler than monotonically increasing input index.

Maybe @Bigcheese can explain this in more detail?

[benlangmuir]

I guess it was originally done this way because we have a number of DependencyScanning{Tool,Worker} instances

Oh of course, this makes sense. Yeah we could maybe find a different way to do it if we care, but this is a sufficiently good reason to keep doing what we're doing and shouldn't hold you up.

Thanks for explaining; LGTM.

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

for (unsigned I = 0; I < Pool.getThreadCount(); ++I)
     WorkerTools.push_back(std::make_unique<DependencyScanningTool>(Service));
 
   std::vector<tooling::CompileCommand> Inputs =
       AdjustingCompilations->getAllCompileCommands();
 
   std::atomic<bool> HadErrors(false);
   std::optional<FullDeps> FD;
   P1689Deps PD;
+
   std::mutex Lock;
   size_t Index = 0;
+  auto GetNextInputIndex = [&]() -> std::optional<size_t> {
+    std::unique_lock<std::mutex> LockGuard(Lock);
+    if (Index < Inputs.size())
+      return Index++;
+    return {};
+  };
 
   if (Format == ScanningOutputFormat::Full)
     FD.emplace(ModuleName.empty() ? Inputs.size() : 0);
 
   if (Verbose) {
     llvm::outs() << "Running clang-scan-deps on " << Inputs.size()
                  << " files using " << Pool.getThreadCount() << " workers\n";
   }
   for (unsigned I = 0; I < Pool.getThreadCount(); ++I) {
-    Pool.async([I, &Lock, &Index, &Inputs, &HadErrors, &FD, &PD, &WorkerTools,
-                &DependencyOS, &Errs]() {
+    Pool.async([&, I]() {
       llvm::StringSet<> AlreadySeenModules;
-      while (true) {
-        const tooling::CompileCommand *Input;
-        std::string Filename;
-        std::string CWD;
-        size_t LocalIndex;
-        // Take the next input.
-        {
-          std::unique_lock<std::mutex> LockGuard(Lock);
-          if (Index >= Inputs.size())
-            return;
-          LocalIndex = Index;
-          Input = &Inputs[Index++];
-          Filename = std::move(Input->Filename);
-          CWD = std::move(Input->Directory);
-        }
+      while (auto MaybeInputIndex = GetNextInputIndex()) {
+        size_t LocalIndex = *MaybeInputIndex;
+        const tooling::CompileCommand *Input = &Inputs[LocalIndex];
+        std::string Filename = std::move(Input->Filename);
+        std::string CWD = std::move(Input->Directory);
+
         std::optional<StringRef> MaybeModuleName;
         if (!ModuleName.empty())
           MaybeModuleName = ModuleName;
 
         std::string OutputDir(ModuleFilesDir);
         if (OutputDir.empty())
           OutputDir = getModuleCachePath(Input->CommandLine);
         auto LookupOutput = [&](const ModuleID &MID, ModuleOutputKind MOK) {