<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Aug 13, 2015 at 9:31 PM, Rafael Espíndola <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"><span>On 14 August 2015 at 00:19, Rui Ueyama <<a href="mailto:ruiu@google.com" target="_blank">ruiu@google.com</a>> wrote:<br>
> I think I feel the same way as Sean's. This change does not seem<br>
> particularly good or bad, but this is indeed a micro-optimization which<br>
> might be a premature optimization at this development stage. And the new<br>
> code is a little bit harder than before (although very little) because<br>
> previously ELFT was everywhere if the ELF type matters, and that was pretty<br>
> straightforward. Now we have two cases -- full ELFT is available or only<br>
> 64-bit or not is available.<br>
<br>
</span>Please be sure to take r244969 in context.<br>
<br>
We now have a native Header structure that is really easy to print in<br>
a debugger, is faster and endian independent.<br>
<br>
It is also now isolated in a base class. All the classes that directly<br>
access ELf structures are templated over ELFT.<br>
<br>
As for premature optimizations: short of an argument why the old code<br>
would be faster, it is a good precaution to change it now since a<br>
change in the future would be more expensive.<br></blockquote><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Last but not least, it is ludicrous to say that having to add<br>
::Is64Bits somewhere is a noticeable merge pain. We have spent way<br>
more time discussing this that would have taken anyone to just update<br>
the change.<br></blockquote><div><br></div><div>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.</div><div><br></div><div>On a related note:</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>-- Sean Silva</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Cheers,<br>
Rafael<br>
</blockquote></div><br></div></div>