[Lldb-commits] [PATCH] D33246: Remove most of lldb's TaskPool in favor of llvm's parallel functions

[Scott Smith via Phabricator via lldb-commits]

scott.smith added inline comments.


================
Comment at: include/lldb/Utility/TaskPool.h:18-20
+  std::function<void()> cbs[sizeof...(T)]{tasks...};
+  llvm::parallel::for_each_n(llvm::parallel::par, static_cast<size_t>(0),
+                             sizeof...(T), [&cbs](size_t idx) { cbs[idx](); });
----------------
zturner wrote:
> I'm not sure this is the most efficient implementation.  `std::function` has pretty poor performance, and there might be no need to even convert everything to `std::function` to begin with.  You could make this a bit better by using `llvm::function_ref<void()>` instead.
> 
> That said, I wonder if it's worth adding a function like this to `llvm::TaskGroup`?  And you could just enqueue all the tasks, rather than `for_each_n`.  Not sure if there would be a different in practice, what do you think?
I'm not too worried about std::function vs llvm::function_ref; it isn't called often, and we still need allocations for the tasks that get enqueued.  That said, there's no reason *to* use std::function, so I'll cahnge it.

I like using for_each_n mostly to regularize the interface.  For example, for_each_n/for_each can then optimize the type of TaskGroup it creates to ensure that it gets the right # of threads right away, rather than spawning up enough for full hardware concurrency.  Or, if there are a lot of tasks (unlikely, but possible), then for_each can change to a model of enqueueing one task per thread, and having that thread loop using std::atomic to increment the iterator, which reduces allocations in TaskGroup and reduces lock contention (assuming TaskGroup doesn't use a lock free queue).

i.e. the more things funnel through a single interface, the more we benefit from optimizing that one implementation.

Also it means we can have for_each_n manage TaskGroups itself (maybe keeping one around for repeated use, then creating more as needed to support recursion, etc (more on that later)).



================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1995-1996
     //----------------------------------------------------------------------
-    TaskMapOverInt(0, num_compile_units, extract_fn);
+    llvm::parallel::for_each_n(llvm::parallel::par, 0U, num_compile_units,
+                               extract_fn);
 
----------------
zturner wrote:
> What did you decide about the recursive parallelism?  I don't know if that works yet using LLVM's default executor.
1. This code doesn't care.
2. It looks like it works, since (I think) for_each creates a separate TaskGroup for each call.
3. However I got a deadlock when using this for parallelizing the dynamic library loading itself, which used to work.  That could either be due to other code changes, some oversight on my part, or it could be that for_each_n doesn't actually support recursion - which means that I misunderstood for_each_n.  So I have more work to do...


Repository:
  rL LLVM

https://reviews.llvm.org/D33246