[1/4] Avoid submitting empty tasks in parallel_for_each
Commit Message
I found that parallel_for_each would submit empty tasks to the thread
pool. For example, this can happen if the number of tasks is smaller
than the number of available threads. In the DWARF reader, this
resulted in the cooked index containing empty sub-indices. This patch
arranges to instead shrink the result vector and process the trailing
entries in the calling thread.
---
gdbsupport/parallel-for.h | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
Comments
Hi Tom,
On Thu, Dec 15, 2022 at 12:07:56PM -0700, Tom Tromey via Gdb-patches wrote:
> I found that parallel_for_each would submit empty tasks to the thread
> pool. For example, this can happen if the number of tasks is smaller
> than the number of available threads. In the DWARF reader, this
> resulted in the cooked index containing empty sub-indices. This patch
> arranges to instead shrink the result vector and process the trailing
> entries in the calling thread.
This patch looks good to me.
If I'm allowed to nitpick...
> ---
> gdbsupport/parallel-for.h | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/gdbsupport/parallel-for.h b/gdbsupport/parallel-for.h
> index acd9137efbd..ed460f31f63 100644
> --- a/gdbsupport/parallel-for.h
> +++ b/gdbsupport/parallel-for.h
> @@ -70,6 +70,12 @@ struct par_for_accumulator
> return result;
> }
>
> + /* Resize the results to N. */
> + void resize (size_t n)
> + {
> + m_futures.resize (n);
> + }
> +
> private:
>
> /* A vector of futures coming from the tasks run in the
> @@ -108,6 +114,12 @@ struct par_for_accumulator<void>
> }
> }
>
> + /* Resize the results to N. */
> + void resize (size_t n)
> + {
> + m_futures.resize (n);
> + }
> +
> private:
>
> std::vector<gdb::future<void>> m_futures;
> @@ -232,6 +244,17 @@ parallel_for_each (unsigned n, RandomIt first, RandomIt last,
> end = j;
> remaining_size -= chunk_size;
> }
> +
> + if (end == last)
> + {
> + /* We're about to dispatch the last batch of elements, which
> + we normally process in the main thread. So just truncate
> + the result list here. */
... I'd expand the last sentence to explain that this is to avoid
submitting empty tasks to the thread pool.
But I'll leave it up to you do decide whether that's a useful suggestion
or not ;-).
Thanks for that patch!
> + count = i;
> + results.resize (count);
> + break;
> + }
> +
> if (parallel_for_each_debug)
> {
> debug_printf (_("Parallel for: elements on worker thread %i\t: %zu"),
> --
> 2.34.3
>
>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:
>> + /* We're about to dispatch the last batch of elements, which
>> + we normally process in the main thread. So just truncate
>> + the result list here. */
Joel> ... I'd expand the last sentence to explain that this is to avoid
Joel> submitting empty tasks to the thread pool.
Joel> But I'll leave it up to you do decide whether that's a useful suggestion
Joel> or not ;-).
Makes sense to me, I did this.
Tom
@@ -70,6 +70,12 @@ struct par_for_accumulator
return result;
}
+ /* Resize the results to N. */
+ void resize (size_t n)
+ {
+ m_futures.resize (n);
+ }
+
private:
/* A vector of futures coming from the tasks run in the
@@ -108,6 +114,12 @@ struct par_for_accumulator<void>
}
}
+ /* Resize the results to N. */
+ void resize (size_t n)
+ {
+ m_futures.resize (n);
+ }
+
private:
std::vector<gdb::future<void>> m_futures;
@@ -232,6 +244,17 @@ parallel_for_each (unsigned n, RandomIt first, RandomIt last,
end = j;
remaining_size -= chunk_size;
}
+
+ if (end == last)
+ {
+ /* We're about to dispatch the last batch of elements, which
+ we normally process in the main thread. So just truncate
+ the result list here. */
+ count = i;
+ results.resize (count);
+ break;
+ }
+
if (parallel_for_each_debug)
{
debug_printf (_("Parallel for: elements on worker thread %i\t: %zu"),