Re: [PATCH][gdbsupport] Use task size in parallel_for_each

[Tom de Vries via Gdb-patches <gdb-patches@sourceware.org>]

On 7/22/22 02:03, Pedro Alves wrote:
> On 2022-07-21 9:23 p.m., Tom de Vries wrote:
>> On 7/21/22 19:35, Pedro Alves wrote:
>>>> diff --git a/gdbsupport/parallel-for.h b/gdbsupport/parallel-for.h
>>>> index bf40f125f0f..3c9269574df 100644
>>>> --- a/gdbsupport/parallel-for.h
>>>> +++ b/gdbsupport/parallel-for.h
>>>> @@ -134,7 +134,9 @@ typename gdb::detail::par_for_accumulator<
>>>>        typename std::result_of<RangeFunction (RandomIt, RandomIt)>::type
>>>>      >::result_type
>>>>    parallel_for_each (unsigned n, RandomIt first, RandomIt last,
>>>> -           RangeFunction callback)
>>>> +           RangeFunction callback,
>>>> +           std::function<unsigned int(RandomIt)> *task_size_ptr
>>>> +             = (std::function<unsigned int(RandomIt)> *)nullptr)
>>>
>>> That use of a std::function pointer looks odd.  AFAICT, TASK_SIZE_PTR is only ever called
>>> as a callback by parallel_for_each, for setup, in the calling thread, and isn't stored
>>> anywhere, right?  If so, gdb::function_view instead should work, is lightweight, and is
>>> nullable, meaning you don't need a pointer.
>>>
>>> And then, at the caller, just using a lambda instead of a std::function should work too:
>>>
>>>       auto task_size = [=] (iter_type iter)
>>>         {
>>>      dwarf2_per_cu_data *per_cu = iter->get ();
>>>      return per_cu->length ();
>>>         };
>>
>> I've tried that (attached) but ran into the usual template error mess, not sure how I could solve that yet.
> 
> I see.  The problem is that here:
> 
>   @@ -134,7 +135,9 @@ typename gdb::detail::par_for_accumulator<
>        typename std::result_of<RangeFunction (RandomIt, RandomIt)>::type
>      >::result_type
>    parallel_for_each (unsigned n, RandomIt first, RandomIt last,
>   -		   RangeFunction callback)
>   +		   RangeFunction callback,
>   +		   gdb::function_view<unsigned int(RandomIt)> task_size
>   +		     = nullptr)
>   
> parallel_for_each is a template, and the function_view parameter's type depends
> on a template parameter (RandomIt), so we can't rely on implicit conversions, such
> as when passing a lambda  (lambda -> function_view).  We need to pass a function_view of
> the right type already.  That's not special about function_view, it's just how templates
> and overload resolution works.
> 	

Ack, I usually manage if there's a specific error mesage, but it was too 
generic in this case.  Anyway, I just lack experience writing templated 
code, something that needs fixing on my side.

> So this would fix it:
> 
> diff --git c/gdb/dwarf2/read.c w/gdb/dwarf2/read.c
> index 23c3873cba6..06df773f1e0 100644
> --- c/gdb/dwarf2/read.c
> +++ w/gdb/dwarf2/read.c
> @@ -7067,11 +7067,12 @@ dwarf2_build_psymtabs_hard (dwarf2_per_objfile *per_objfile)
>   
>       using iter_type = decltype (per_bfd->all_comp_units.begin ());
>   
> -    auto task_size = [=] (iter_type iter)
> +    auto task_size_ = [=] (iter_type iter)
>         {
>          dwarf2_per_cu_data *per_cu = iter->get ();
>          return per_cu->length ();
>         };
> +    gdb::function_view<unsigned (iter_type)> task_size = task_size_;
> 
> Though it's annoying to have to write the function_view type.
> 
> Note that this instead, is a bad pattern with function_view (similarly
> to other view types, like string_view), as it immediately dangles the lambda
> temporary:
> 
> -    auto task_size = [=] (iter_type iter)
> +    gdb::function_view<unsigned (iter_type)> task_size = [=] (iter_type iter)
>         {
>          dwarf2_per_cu_data *per_cu = iter->get ();
>          return per_cu->length ();
>         };
> 
> I think the best is to introduce a gdb::make_function_view function,
> so that you can do this:
> 
> diff --git c/gdb/dwarf2/read.c w/gdb/dwarf2/read.c
> index 23c3873cba6..255b955a54c 100644
> --- c/gdb/dwarf2/read.c
> +++ w/gdb/dwarf2/read.c
> @@ -7101,7 +7101,7 @@ dwarf2_build_psymtabs_hard (dwarf2_per_objfile *per_objfile)
>                }
>            }
>          return result_type (thread_storage.release (), std::move (errors));
> -      }, task_size);
> +      }, gdb::make_function_view (task_size));
>   
>       /* Only show a given exception a single time.  */
>       std::unordered_set<gdb_exception> seen_exceptions;
> 
> I've got that working here.  I'll post it tomorrow.
> 


Ack, used in updated patch, submitted in this ( 
https://sourceware.org/pipermail/gdb-patches/2022-July/191004.html ) series.


> Note: the 'task_size' lambda doesn't actually need to capture anything:
> 
> @@ -7067,11 +7067,12 @@ dwarf2_build_psymtabs_hard (dwarf2_per_objfile *per_objfile)
>   
>       using iter_type = decltype (per_bfd->all_comp_units.begin ());
>   
> -    auto task_size = [=] (iter_type iter)
> +    auto task_size = [] (iter_type iter)
>         {
>          dwarf2_per_cu_data *per_cu = iter->get ();
>          return per_cu->length ();
>         };
> 

Fixed.

Thanks for the review and help.

- Tom