[Lldb-commits] [PATCH] D13662: Make dwarf parsing multi-threaded

[Greg Clayton via lldb-commits]

clayborg added a comment.

See inlined comments.


================
Comment at: lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2087-2088
@@ +2086,4 @@
+            results.emplace_back(TaskPool::AddTask(parser_fn, cu_idx));
+        for (auto& f : results)
+            f.wait();
+
----------------
tberghammer wrote:
> clayborg wrote:
> > So we are still going to serially wait for the each item in the task list to complete? Don't we want to use TaskRunner::WaitForNextCompletedTask() here?
> I don't see any benefit for using TaskRunner::WaitForNextCompletedTask() here because we can't really do anything when only a few task is completed and using TaskRunner ads an extra layer of indirection (to implement WaitForNextCompletedTask) what have a very minor performance hit.
> 
> One possible improvement we can do is to do the merging of the indexes on the main thread while we are waiting for the parsing tasks to complete, but I am not sure if it will have any performance benefit as it would mean that we do it on a single thread instead of 9 threads we are doing it now (with TaskPool::RunTasks).
Seems like you could change the future to just return the cu_idx from parser_fn so we can append all items to the member variables in the main thread:

```
while (uint32_t cu_idx : task_runner. WaitForNextCompletedTask())
{
     m_function_basename_index.Append(function_basename_index[cu_idx));
     m_function_fullname_index.Append(function_fullname_index[cu_idx));
     ...
}
```

Otherwise you are serializing the merging + finalize to be at the end. One nice thing about doing it the way you are doing is that it will be consistent from run to run as the data will always appear in the same order as the debug info file. But these maps should be the same regardless and the oder in which the data comes in shouldn't affect the final content.

================
Comment at: lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2090-2095
@@ +2089,8 @@
+
+        auto merge_fn = [](NameToDIE& target, const std::vector<NameToDIE>& sources)
+        {
+            for (const auto& src : sources)
+                target.Append(src);
+            target.Finalize();
+        };
+
----------------
If you do the Append() calls inside the while loop above, then all we need to do it call Finalize() on each member variable below.


Repository:
  rL LLVM

http://reviews.llvm.org/D13662