[PATCH] D114845: [llvm] [Debuginfod] DebuginfodCollection and DebuginfodServer for tracking local debuginfo.

[Daniel Thornburgh via Phabricator via llvm-commits]

mysterymath added inline comments.
Herald added a project: All.


================
Comment at: llvm/include/llvm/Debuginfod/Debuginfod.h:85
+  ThreadPool &Pool;
+  size_t Concurrency = 1;
+
----------------
I don't think the `= 1` does anything, since providing a constructor inhibits generation of a default one.


================
Comment at: llvm/include/llvm/Debuginfod/Debuginfod.h:96
+
+class DebuginfodServer {
+public:
----------------
Since all members are public, this should be a `struct`. This also fits with how it's used, post-construction.


================
Comment at: llvm/lib/Debuginfod/Debuginfod.cpp:217-218
+    return Err;
+  outs() << "finished first update\n";
+  outs().flush();
+  std::this_thread::sleep_for(Interval);
----------------
This seems a bit odd to unconditionally put to stdout; is this necessary?

Otherwise, the whole routine just becomes
```
while (true) {
  if (Error Err = update())
    return Err;
  std::this_thread::sleep_for(Interval);
}
return Error::success();
```


================
Comment at: llvm/lib/Debuginfod/Debuginfod.cpp:225
+  }
+  return Error::success();
+}
----------------
llvm_unreachable()?


================
Comment at: llvm/lib/Debuginfod/Debuginfod.cpp:315-319
+    // Wait for the number of concurrent jobs to go down
+    while (NumTasksRemaining > Concurrency) {
+      LLVM_DEBUG(dbgs() << NumTasksRemaining << " tasks remaining\n";);
+      std::this_thread::sleep_for(std::chrono::milliseconds(50));
+    }
----------------
Is there an advantage for manually managing the concurrency here, over passing it as an argument to ThreadPool, `std::min`-ed with the hardware concurrency?
>From a cursor look at ThreadPool's API, each async call after the thread pool is full should just more-or-less push a std::function<void()> onto a vector.


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

https://reviews.llvm.org/D114845