<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Dec 1, 2017 at 2:54 PM, Rafael Avila de Espindola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="m_3930766701756646844m_3784248082045176491HOEnZb"><div class="m_3930766701756646844m_3784248082045176491h5">Rui Ueyama <<a href="mailto:ruiu@google.com" target="_blank">ruiu@google.com</a>> writes:<br>
<br>
> On Fri, Dec 1, 2017 at 2:10 PM, Rafael Avila de Espindola <<br>
> <a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>> wrote:<br>
><br>
>> Rui Ueyama <<a href="mailto:ruiu@google.com" target="_blank">ruiu@google.com</a>> writes:<br>
>><br>
>> > On Fri, Dec 1, 2017 at 1:41 PM, Rafael Avila de Espindola <<br>
>> > <a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>> wrote:<br>
>> ><br>
>> >> Rui Ueyama via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>> writes:<br>
>> >><br>
>> >> > ruiu updated this revision to Diff 125067.<br>
>> >> > ruiu added a comment.<br>
>> >> ><br>
>> >> > - clang-format<br>
>> >> ><br>
>> >> > It seems this patch actually makes that test case 2% faster, which<br>
>> >> > isn't a negligible improvement.<br>
>> >><br>
>> >> Please benchmark more than clang. There is a set of different benchmarks<br>
>> >> in <a href="https://s3-us-west-2.amazonaws.com/linker-tests/" rel="noreferrer" target="_blank">https://s3-us-west-2.amazonaws<wbr>.com/linker-tests/</a><br>
>> lld-speed-test.tar.xz.<br>
>> ><br>
>> ><br>
>> > We don't need to run all these benchmarks as this patch is a micro<br>
>> > optimization. More precisely, in order to create a hash table for 22212<br>
>> > symbols,<br>
>> ><br>
>> >  - GnuHashTableSection::addSymbol<wbr>s took 60 ms<br>
>> >  - GnuHashTableSection::writeHash<wbr>Table took 8 ms.<br>
>> ><br>
>> > With this patch it now takes 37 ms and 7 ms, respectively.<br>
>><br>
>> I am uncomfortable LGTMing an optimization without a full<br>
>> benchmark. Specially one that trades memory for speed.<br>
>><br>
><br>
> There's no memory-to-CPU tradeoff. There was a 32-bit padding at end of<br>
> `Entry` struct.<br>
<br>
</div></div>OK, that makes it more likely that this is useful, but I still think<br>
we should fully benchmark every single optimization.<br></blockquote><div><br></div><div>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.</div><div><br></div><div>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.</div></div></div></div>