[Lldb-commits] [PATCH] D13727: Add task pool to LLDB

[Pavel Labath via lldb-commits]

labath requested changes to this revision.
labath added a comment.
This revision now requires changes to proceed.

Although it may not seem from the number of my comments, I actually quite like this implementation. However, I think more work needs to be done to make this future-proof.

- we definitely need unit tests for this.
- we need more documentation (especially on the public interfaces, but the trickier internal parts could use some explanation as well).


================
Comment at: include/lldb/Utility/TaskPool.h:23
@@ +22,3 @@
+
+    // Add a new task to the thread pool and return a std::future belongs for the newly created task.
+    // The caller of this function have to wait on the future for this task to complete.
----------------
belonging to

================
Comment at: include/lldb/Utility/TaskPool.h:24
@@ +23,3 @@
+    // Add a new task to the thread pool and return a std::future belongs for the newly created task.
+    // The caller of this function have to wait on the future for this task to complete.
+    template<typename F, typename... Args>
----------------
s/have/has

================
Comment at: include/lldb/Utility/TaskPool.h:33
@@ +32,3 @@
+    static void
+    RunTasks(T&&... t);
+
----------------
Perhahs the entire TaskPool could be an implementation detail and this functionality would be accessed (via a static method on) TaskRunner? What do you think? I have found it confusing it the other commit, why you sometimes use TaskPool and another time TaskRunner...

================
Comment at: include/lldb/Utility/TaskPool.h:80
@@ +79,3 @@
+    void
+    WaitForAllTask();
+
----------------
WaitForAllTasks

================
Comment at: include/lldb/Utility/TaskPool.h:103
@@ +102,3 @@
+
+template<typename H, typename... T>
+struct TaskPool::RunTaskImpl<H, T...>
----------------
Suggestion: name these `Head`, `Tail`. You use `T` for other purposes elsewhere, and I have spent a lot of time trying to figure out why is this `H` and not `F` before I figured out what's going on here.

================
Comment at: include/lldb/Utility/TaskPool.h:126
@@ +125,3 @@
+{
+    auto task = std::make_shared<std::packaged_task<typename std::result_of<F(Args...)>::type()>>(
+        std::bind(std::forward<F>(f), std::forward<Args>(args)...));
----------------
task_sp

================
Comment at: include/lldb/Utility/TaskPool.h:138
@@ +137,3 @@
+
+template <typename T>
+template<typename F, typename... Args>
----------------
You use `T` for both "task" and the "result value of a task". Could you disambiguate the two. Suggestion: `Ret` (or `Return`) and `Task`.

================
Comment at: include/lldb/Utility/TaskPool.h:145
@@ +144,3 @@
+    auto it = m_pending.emplace(m_pending.end());
+    *it = std::move(TaskPool::AddTask(
+        [this, it](F&& f, Args&&... args)
----------------
This construction is quite convoluted. Do you actually need a list of the pending tasks anywhere? As far as I can tell, you only need to check if there are any tasks pending, which can be done with a simple (atomic) integer.

================
Comment at: include/lldb/Utility/TaskPool.h:148
@@ +147,3 @@
+        {
+            T&& r = f(args...);
+
----------------
Is `std::forward` needed here?


http://reviews.llvm.org/D13727