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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
