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

[Pedro Alves <pedro@palves.net>]

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.

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.

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 ();
       };