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

[Nick Clifton <nickc at redhat dot com>]

Hi Jens,

> 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.

No worries - thank you for taking the time to pursue contributing this patch.

As for testing I would recommend building and testing a 32-bit target, in
order to make sure that there are no problems relating to bit sizes.  Eg:

  % <path-to-binutils-sources>/configure --quiet CFLAGS="-m32"
  % make all-gas all-ld all-binutils
  % make check

Another useful test is with a toolchain configured for all targets as this
can often throw up issues with other architectures that do not show up with
your primary architecture:

  % <path-to-binutils-source>/configure --enable-64-bit-bfd --enable-targets=all
  % make all-gas all-ld all-binutils
  % make check

Note - I am not actually expecting you to find any issues with the patch,
it looks perfectly fine to me.  But it is good practice to always test,
even when you think that something looks innocuous.


>>           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. :-)

Great minds ... :-)

> 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.

Fair enough.  I would hope that a sufficiently clever compiler would
be able to replace the modulo operator with a subtraction anyway, so
performance should not suffer.

Cheers
  Nick