Re: [PATCH 3/6] gdbsupport: use dynamic partitioning in gdb::parallel_for_each

[Tom Tromey <tom@tromey.com>]

>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:

Simon>  - I want to still be able to use scoped_time_it to measure the time
Simon>    that each worker thread spent working on the task.  I find it really
Simon>    handy when measuring the performance impact of changes.

Simon>    Unfortunately, the current interface of gdb::parallel_for_each, which
Simon>    receives a simple callback, is not well-suited for that, once I
Simon>    introduce the dynamic partitioning.  The callback would get called
Simon>    once for each work item batch (multiple time for each worker thread),
Simon>    so it's not possible to maintain a per-worker thread object for the
Simon>    duration of the parallel for.

Simon>    To allow this, I changed gdb::parallel_for_each to receive a worker
Simon>    type as a template parameter.  Each worker thread creates one local
Simon>    instance of that type, and calls operator() on it for each work item
Simon>    batch.  By having a scoped_time_it object as a field of that worker,
Simon>    we can get the timings per worker thread.

Simon>    The drawbacks of this approach is that we must now define the
Simon>    parallel task in a separate class and manually capture any context we
Simon>    need as fields of that class.

This seems like a real step backward to me.  It's basically replicating
lambdas by hand.

Why can't parallel_for_each just do this itself?  It could take an
argument naming the task to pass to the scoped_time_it constructor.  The
'task' lambda could instantiate the timer.

Simon> +  /* The next item to hand out.  */
Simon> +  std::atomic<RandomIt> next = first;

This is maybe a bit worrying since IIUC std::atomic only works for
trivially copyable types.  But this doesn't have to be a general purpose
facility, it only has to work for gdb's needs.

Tom