[PATCH] D40697: Cache modulo values for the .gnu.hash section.

[Rui Ueyama via llvm-commits]

On Fri, Dec 1, 2017 at 2:54 PM, Rafael Avila de Espindola <
rafael.espindola at gmail.com> wrote:

> Rui Ueyama <ruiu at google.com> writes:
>
> > On Fri, Dec 1, 2017 at 2:10 PM, Rafael Avila de Espindola <
> > rafael.espindola at gmail.com> wrote:
> >
> >> Rui Ueyama <ruiu at google.com> writes:
> >>
> >> > On Fri, Dec 1, 2017 at 1:41 PM, Rafael Avila de Espindola <
> >> > rafael.espindola at gmail.com> wrote:
> >> >
> >> >> Rui Ueyama via Phabricator <reviews at reviews.llvm.org> writes:
> >> >>
> >> >> > ruiu updated this revision to Diff 125067.
> >> >> > ruiu added a comment.
> >> >> >
> >> >> > - clang-format
> >> >> >
> >> >> > It seems this patch actually makes that test case 2% faster, which
> >> >> > isn't a negligible improvement.
> >> >>
> >> >> Please benchmark more than clang. There is a set of different
> benchmarks
> >> >> in https://s3-us-west-2.amazonaws.com/linker-tests/
> >> lld-speed-test.tar.xz.
> >> >
> >> >
> >> > We don't need to run all these benchmarks as this patch is a micro
> >> > optimization. More precisely, in order to create a hash table for
> 22212
> >> > symbols,
> >> >
> >> >  - GnuHashTableSection::addSymbols took 60 ms
> >> >  - GnuHashTableSection::writeHashTable took 8 ms.
> >> >
> >> > With this patch it now takes 37 ms and 7 ms, respectively.
> >>
> >> I am uncomfortable LGTMing an optimization without a full
> >> benchmark. Specially one that trades memory for speed.
> >>
> >
> > There's no memory-to-CPU tradeoff. There was a 32-bit padding at end of
> > `Entry` struct.
>
> OK, that makes it more likely that this is useful, but I still think
> we should fully benchmark every single optimization.
>

It depends. Your test cases do not cover all possible use cases, and since
this is an optimization for that particular code path, it actually makes
more sense to only measure the speed of that particular path. The benchmark
numbers would be more noisy if you measure the entire execution time.

It still make some sense to run some benchmarks just in case, because some
seemingly unrelated change could results in a surprising performance
degradation, but for that purpose, running one or two benchmarks should
enough.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171201/41b7de8c/attachment.html>