Re: [PATCH v2 11/18] Do more DWARF reading in the background

[John Baldwin <jhb@FreeBSD.org>]

On 11/13/23 11:39 AM, Tom de Vries wrote:
> On 11/13/23 18:56, John Baldwin wrote:
>> On 11/12/23 11:15 PM, Tom de Vries wrote:
>>> On 11/12/23 21:25, Tom Tromey wrote:
>>>> + /* How much effort should be put into each worker. */ + const size_t
>>>> size_per_thread = total_size / n_worker_threads;
>>>
>>> I've tested the patch series on x86_64-linux, and ran into trouble in
>>> one test-case:
>>> ...
>>> (gdb) file
>>> /data/vries/gdb/leap-15-4/build/gdb/testsuite/outputs/gdb.dwarf2/dw2-dummy-cu/dw2-dummy-cu^M
>>> Reading symbols from
>>> /data/vries/gdb/leap-15-4/build/gdb/testsuite/outputs/gdb.dwarf2/dw2-dummy-cu/dw2-dummy-cu...^M
>>> /data/vries/gdb/src/gdb/dwarf2/read.c:5000: internal-error: do_reading:
>>> Assertion `iter != last' failed.^M
>>> A problem internal to GDB has been detected,^M
>>> further debugging may prove unreliable.^M
>>> ----- Backtrace -----^M
>>> ERROR: Couldn't load dw2-dummy-cu into GDB (GDB internal error).
>>> Resyncing due to internal error.
>>> (gdb) maint expand-symtab^M
>>> 0x5ac476 gdb_internal_backtrace_1^M
>>>            /data/vries/gdb/src/gdb/bt-utils.c:122^M
>>> 0x5ac519 _Z22gdb_internal_backtracev^M
>>>            /data/vries/gdb/src/gdb/bt-utils.c:168^M
>>> 0xd11a89 internal_vproblem^M
>>>            /data/vries/gdb/src/gdb/utils.c:396^M
>>> 0xd11e58 _Z15internal_verrorPKciS0_P13__va_list_tag^M
>>>            /data/vries/gdb/src/gdb/utils.c:476^M
>>> 0x14ca6f0 _Z18internal_error_locPKciS0_z^M
>>>            /data/vries/gdb/src/gdbsupport/errors.cc:58^M
>>> 0x72b01b _ZN19cooked_index_worker10do_readingEv^M
>>>            /data/vries/gdb/src/gdb/dwarf2/read.c:5000^M
>>> ...
>>>
>>> This fixes the test-case for me:
>>> ...
>>> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
>>> index 385160de0fd..191c2ad7f26 100644
>>> --- a/gdb/dwarf2/read.c
>>> +++ b/gdb/dwarf2/read.c
>>> @@ -4974,7 +4974,8 @@ cooked_index_worker::do_reading ()
>>>         = std::max (gdb::thread_pool::g_thread_pool->thread_count (),
>>> (size_t) 1);
>>>
>>>       /* How much effort should be put into each worker.  */
>>> -  const size_t size_per_thread = total_size / n_worker_threads;
>>> +  const size_t size_per_thread
>>> +    = std::max (total_size / n_worker_threads, (size_t)1);
>>
>> Presumably total_size was zero in that case?
> 
> No, it was 11.
> 
>> Should there be a shortcut
>> to bother queueing a task in that case instead perhaps?  Or alternatively
>> is the issue that total_size is < n_worker_threads?
> 
> Yes, n_worker_threads was 12.
> 
>> If the latter, then
>> a min size of 1 does seem sensible.
>>
> 
> Thanks for the review.

Is it possible that you want size_per_thread to always round up?  That is,
if you had total_size == 23 with n_worker_threads == 12, is there a reason
to prefer size_per_thread being 2 in that case vs 1?  (E.g. if the threads
each do "size_per_thread" items with the last one handling any remainder,
then you'd probably prefer 2 so that 11 threads do 2 and the last thread
does 1 rather than 11 threads doing 1 and the last thread doing 11.)

Looking at the patch, I do think it's the case I described, so perhaps you
want to always round up via:

const size_t size_per_thread
   = (total_size + n_worker_threads - 1) / n_worker_threads;

-- 
John Baldwin