[lld] r244934 - Template OutputSection only over Is64Bit.

[Sean Silva via llvm-commits]

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/20150814/a3059174/attachment.html>