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

[Noah Shutty via Phabricator via llvm-commits]

noajshu marked an inline comment as done.
noajshu added inline comments.


================
Comment at: llvm/lib/Debuginfod/Debuginfod.cpp:367-373
+  }
+  {
+    std::unique_lock<std::mutex> Guard(IteratorMutex);
+    // Wait on condition variable indicating directory traversal is complete.
+    IteratorCondition.wait(Guard,
+                           [&] { return (I == E || EC) && !NumActiveWorkers; });
+  }
----------------
mysterymath wrote:
> Since each async call exits as soon as `I == E || EC`, this block can be replaced with just `ThreadPool.wait()`, and there's no need to maintain a separate condition variable. (ThreadPool maintains one internally for this purpose.)
> It also looks like the decrements of NumActiveWorkers and the outer loop are interleaved in a way that adds some complexity to the situation; this would remove the need for NumActiveWorkers, which clears that up too.
great idea! 

Since the first draft of this revision, [[ https://llvm.org/doxygen/classllvm_1_1ThreadPoolTaskGroup.html | ThreadPoolTaskGroup ]] was merged into LLVM. It nicely fits our use case, here we just use `IteratorGroup.wait();`.

(now that I am aware of the new Task Group feature, I propose to close D126815 altogether, as it does appear quite ad-hoc in light of the clean alternative of using a task group)


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

https://reviews.llvm.org/D114845