[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](); });
----------------
scott.smith wrote:
> 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)).
> 
oh, and I wouldn't be surprised if there's a better home in llvm.  I'm fine with moving it.  I doubt it should go in llvm::parallel, since that seems like it's trying to be similar to std::parallel (though note for_each_n is incompatible, since it should take Iter,Size instead of Type,Type, and it tries to dereference Iter, which means you can't pass in a number like all the callsites do.  I tried fixing that but failed due to the deref assumption, and the LLD dependency).

It's small enough that it does seem like it should fit under something else rather than be standalone; I'm open to suggestions.


Repository:
  rL LLVM

https://reviews.llvm.org/D33246