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

[Zachary Turner via lldb-commits]

Even though it creates a different TaskGroup for each one, TaskGroup itself
just says getDefaultExecutor, and that returns a global instance.  On
Windows this will work because it all delegates to an OS thing that's sort
of like libdispatch and that does support recursion, but the thread pool
executor just has a fixed number of threads and there's no way to increase
or decrease them.  Can you put this test inside of ParallelTest.cpp and see
what you get?

TEST(Parallel, parallel_for_recursive) {
  uint32_t range[10001];
  std::fill(range, range + 10001, 1);

  for_each_n(parallel::par, 0, 100, [&range](size_t I) {
    for_each_n(parallel::par, 100 * I, 100 * (I+1), [&range](size_t J) {
      ++range[J];
    });
  });
  uint32_t expected[10000];
  std::fill(expected, expected + 10000, 2);
  ASSERT_TRUE(std::equal(range, range + 10000, expected));
  // Check that we don't write past the end of the requested range.
  ASSERT_EQ(range[10000], 1u);
}

This passes me for me on Windows, but I suspect it will deadlock for you.

On Tue, May 16, 2017 at 7:45 PM Scott Smith via Phabricator <
reviews at reviews.llvm.org> wrote:

> 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20170517/9c102fcb/attachment-0001.html>