[Lldb-commits] [PATCH] D78337: [lldb/Host] Remove TaskPool and replace its uses with llvm::ThreadPool

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

While getting rid of this would be great, this (the manual dwarf index) is also very performance sensitive code which has been optimized and re-optimized a number of times. So, I would want to see a benchmark to ensure this does not affect performance adversely. A good benchmark would be to take a release (without asserts) build of lldb and debug build of clang (without accelerator tables) and then run something like `time opt/lldb dbg/clang -o "br set -n nonexisting_function" -b` a couple of times. The test should be run on an elf binary because the mac debug info model is sufficiently different to skew the results, even if you do manage to disable the accelerator tables.



================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp:24-30
+static void TaskMapOverInt(size_t end,
+                           const llvm::function_ref<void(size_t)> &func) {
+  llvm::ThreadPool pool;
+  for (size_t i = 0; i < end; i++)
+    pool.async(func, i);
+  pool.wait();
+}
----------------
This *is* simpler than the previous implementation, but it may have different performance characteristics. The previous implementation was optimized for small workloads where the overhead of enqueuing a task was not negligible compared to the size of the job itself.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp:90-113
+  TaskMapOverInt(units_to_index.size(), extract_fn);
 
   // Now create a task runner that can index each DWARF unit in a
   // separate thread so we can index quickly.
 
-  TaskMapOverInt(0, units_to_index.size(), parser_fn);
+  TaskMapOverInt(units_to_index.size(), parser_fn);
 
----------------
All of this should use the same ThreadPool instance to avoid re-creating the pool threads a couple of times.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D78337