Re: [PATCH 4/7] Class-fy partial_die_info

[Simon Marchi <simark@simark.ca>]

On 2018-01-30 06:39, Pedro Alves wrote:
> On 01/29/2018 01:15 AM, Simon Marchi wrote:
> 
>> From what I understand, the only reason to have that private 
>> constructor is
>> to construct a temporary partial_die_info object used to search in the 
>> htab,
>> is that right?  If so, converting that htab_t to an std::unordered_map 
>> would
>> remove the need for all this, since you don't need to construct an 
>> object
>> to search it.  See the diff below that applies on top of this patch.
>> 
>> It's not thoroughly tested and I am not sure of the validity of the
>> per_cu->cu->partial_dies.empty () call in find_partial_die, but I 
>> think it
>> should work.  Plus, it adds some type-safety, which I am a big fan of.
>> 
>> But otherwise, the patch is fine with me.
> 
> Careful here.  This could do with some benchmarking.  The DWARF reading 
> code
> is performance (both timing and memory) sensitive.  This is trading an 
> open
> addressing hash table (htab_t), for a node-based closed addressing hash 
> table.
> The keys/elements in the map are small so I'd expect this to make
> a difference.  Also, this is trading a in-principle cache-friendly
> obstack allocation scheme for the standard new allocator.

Ah, indeed.  I thought that unordered_map would be implemented the same 
way as htab_t, but I see it's not the case.  Doing some quick tests on a 
big binary, it increases the time reading the symbols from an average of 
37 seconds to an average of 42 seconds.

I understand the different hash table implementation having an impact, 
but I don't really understand how the allocation scheme can have a 
meaningful impact.  The partial_die_info objects are still allocated on 
the obstack, aren't they?  So it's just the space for the table itself 
that isn't on the objstack, but I don't see why that would make a 
difference.

If we want to have a data structure with the same kind of performance as 
htab_t but with type-safety in the future, is your vision that we'll 
have to implement it ourselves?  Should we make a wrapper around htab_t?

Simon