This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Optimize reading ELF objects with many group sections


On Mon, Jan 8, 2018 at 11:23 AM, Nick Clifton <nickc@redhat.com> wrote:
> Hi Jens,
>
>   A couple of comments on the patch:
>
>   * It would be helpful if you could include a changelog
>     entry along with the patch itself.

Will do that in the next revision.

>   * Please can you tell us how you tested the patch, and if
>     you noticed any regressions.  (I do not expect that there
>     were any, but you never know).  In particular a patch like
>     this that affects generic code needs to be tested with a
>     wide range of targets to make sure that it does not break
>     anything.

As I was investigating a performance issue, my manual testing has been
somewhat focused on performance. I've run `make check` (on a 64-bit
Linux) with and without my patch, and the output was identical.

Some pointers to other useful testing I should do would be helpful.
First-time contributor here, so I'm not exactly familiar with the
process.

>   * Also...
>
>> +++ b/bfd/elf-bfd.h
>> @@ -1908,6 +1908,7 @@ struct elf_obj_tdata
>>
>>    Elf_Internal_Shdr **group_sect_ptr;
>>    int num_group;
>> +  unsigned int group_search_offset;
>
>   I think that it would be helpful to include a comment by
>   this field explaining what it is for.

Will add.

>> +       /* Begin search from previous found group. */
>> +       unsigned i = j + search_offset;
>> +
>> +       if (i >= num_group)
>> +         i -= num_group;
>
>   I found this to be a little bit confusing when I was reading
>   the patch for the first time.  Maybe this would be clearer ?
>
>           unsigned int i = (j + search_offset) % num_group;
>
>   Of course since the intent is to optimise this loop for speed
>   you may object to the presence of the modulo operator, so I
>   leave the choice up to you.

That happens to be exactly what I had in the first iteration of my patch. :-)

IIRC my next profiling run showed that division instruction as
"significant", and thought someone might comment on it in the context
of an optimization patch, so changed to the current code. But overall,
I like the code with a modulo operator better, and I doubt the
difference in performance, if there even was one, matters in practice.
I'll change back.

--
Jens


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]