[Lldb-commits] [PATCH] D32757: Add TaskMap for iterating a function over a set of integers

[Greg Clayton via Phabricator via lldb-commits]

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

In https://reviews.llvm.org/D32757#743567, @scott.smith wrote:

> IMO, this is a simpler interface than TaskRunner.  Also, after this, there are no users of TaskRunner left.  Should I just delete them?


I think you might not have understood TaskRunner's real benefits. It is smart in the way it runs things: it lets you run N things and get each item **as soon as it completes**. The TaskMap will serialize everything. So if you have 100 items to do and item 0 takes 200 seconds to complete, and items 1 - 99 take 1ms to complete, you will need to wait for task 0 to complete before you can start parsing the data. This will slow down the DWARF parser if you switch over to this. TaskRunner should not be deleted as it has a very specific purpose. Your TaskMap works fine for one case, but not in the other.

> I did this change to help parallel symbol demangling (to come in a separate patch).  Rather than have the symbol demangler use batching, etc, I thought it should be a higher level function.

Make sure this is a win before switching demangling over to using threads. I tried to improve performance on demangling before and I got worse performance on MacOS when we tried it because all of the malloc contention and threading overhead.



================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1988
     //----------------------------------------------------------------------
-    while (true) {
-      auto f = task_runner_extract.WaitForNextCompletedTask();
-      if (!f.valid())
-        break;
-      unsigned cu_idx;
-      bool clear;
-      std::tie(cu_idx, clear) = f.get();
-      clear_cu_dies[cu_idx] = clear;
-    }
+    TaskMap(num_compile_units, 1, extract_fn);
 
----------------
This replacement is ok, since no expensive work is being done in the while loop that did the task_runner_extract.WaitForNextCompletedTask();.


================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1994
 
-    TaskRunner<uint32_t> task_runner;
-    for (uint32_t cu_idx = 0; cu_idx < num_compile_units; ++cu_idx)
-      task_runner.AddTask(parser_fn, cu_idx);
-
-    while (true) {
-      std::future<uint32_t> f = task_runner.WaitForNextCompletedTask();
-      if (!f.valid())
-        break;
-      uint32_t cu_idx = f.get();
+    TaskMap(num_compile_units, 1, parser_fn);
 
----------------
Note that we lost performance here. The old code would run:

```
while (true) {
    std::future<uint32_t> f = task_runner.WaitForNextCompletedTask();
    // Do expensive work as soon as possible with any random task that completes as soon as it completes.
```

Your current code says "wait to do all expensive work until I complete everything and then do all of the expensive work".






================
Comment at: source/Utility/TaskPool.cpp:77
+
+void TaskMap(size_t size, size_t batch, std::function<void(size_t)> const & func)
+{
----------------
Is this named correctly? Maybe SerializedTaskMap? Or BatchedTaskMap?


================
Comment at: source/Utility/TaskPool.cpp:100
+  for (size_t i = 0; i < est_batches; i++)
+    futures[i].wait();
+}
----------------
TaskRunner is smart in the way it runs things: it lets you run N things and get each item as it completes. This implementation, if read it correctly, will serialize everything. So if you have 100 items to do and item at index 0 takes 200 seconds to complete, and items 1 - 99 take 1ms to complete, you will need to wait for task 0 to complete before you can start parsing the data. This will slow down the DWARF parser if you switch over to this.


Repository:
  rL LLVM

https://reviews.llvm.org/D32757