[lld] r244934 - Template OutputSection only over Is64Bit.

[Rui Ueyama via llvm-commits]

Sorry to resurrect this old thread, but as I take a look again at the ELF
Writer's code, I found that this patch made the code a bit messy. Some
functions or classes are specialized only for `Is64Bits` and others are for
`ELFT`, although `ELFT` works for both cases. I really prefer code
simplicity over marginal efficiency at this stage of development, so I
agree with Sean. Can I roll it back?

On Fri, Aug 14, 2015 at 6:02 PM, Sean Silva <chisophugis at gmail.com> wrote:

>
>
> On Thu, Aug 13, 2015 at 9:31 PM, Rafael Espíndola <
> rafael.espindola at gmail.com> wrote:
>
>> On 14 August 2015 at 00:19, Rui Ueyama <ruiu at google.com> wrote:
>> > I think I feel the same way as Sean's. This change does not seem
>> > particularly good or bad, but this is indeed a micro-optimization which
>> > might be a premature optimization at this development stage. And the new
>> > code is a little bit harder than before (although very little) because
>> > previously ELFT was everywhere if the ELF type matters, and that was
>> pretty
>> > straightforward. Now we have two cases -- full ELFT is available or only
>> > 64-bit or not is available.
>>
>> Please be sure to take r244969 in context.
>>
>> We now have a native Header structure that is really easy to print in
>> a debugger, is faster and endian independent.
>>
>> It is also now isolated in a base class. All the classes that directly
>> access ELf structures are templated over ELFT.
>>
>> As for premature optimizations: short of an argument why the old code
>> would be faster, it is a good precaution to change it now since a
>> change in the future would be more expensive.
>>
>
> I disagree that adding an unmeasured "optimization" should be done as a
> "precaution", here is a completely legitimate reason why this could be a
> pessimization for the x86 targeting x86 case (which is our primary
> practical use case): now the versions of this code need to be shared across
> the big and little endian copies of the data structures. Previously, the
> code layout was trivial since there was a copy for each ELFT combination
> and it was a normal code layout problem for each individually. Now, a good
> code layout needs to take into account the cost of sharing these routines,
> possibly pessimizing the code layout.
>
> I'm not saying that this will actually result in a measurable
> pessimization, but it has just as much to go on as the "should be faster"
> statements in the commit message of the present patch.
>
> Considering the simplicity benefits of uniformly having everything
> templated on ELFT, except for things that absolutely don' t need it, I
> think we should for consistency and simplicity just follow that pattern.
>
>
>
>>
>> Last but not least, it is ludicrous to say that having to add
>> ::Is64Bits somewhere is a noticeable merge pain. We have spent way
>> more time discussing this that would have taken anyone to just update
>> the change.
>>
>
> Any given change might take not too long, but the problem is that there is
> a continuous stream (just today I heard Michael multiple times be broken by
> your patches (and almost always by "NFC" ones it seems)). It adds up.
>
> On a related note:
>
> ELF2 hasn't reached a steady state where we have a full set of feature
> functionality so that we are pretty confident that anything that is dead
> can just be removed. During this rapid advancement of functionality
> additions (non-NFC), some stuff is dead but will be imminently used, so
> deleting it is counterproductive; the usual refactoring intuitions are
> turned on their head. It wastes both the time of the person deleting the
> code and also the person adding the functionality that needs it.
>
> Feel free to add `// FIXME: dead?` next to things, but please stop
> deleting code at this early stage unless you absolutely know it will never
> be used. Down the road it is a simple grep to remove everything that has
> been marked with a FIXME. Removing dead code is just as easy at any point
> in time: that is what it means to be dead (what is hard is finding it).
> There is no benefit to deleting it right now, and as I described (and have
> observed in practice) deleting a piece of "dead" code may in fact be
> detrimental to ELF2's development speed in its current phase.
>
> -- Sean Silva
>
>
>>
>> Cheers,
>> Rafael
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150910/e88531a5/attachment.html>