On Wed, Aug 02, 2017 at 05:53:45AM -0400, Frediano Ziglio wrote: > > > > On Wed, Aug 02, 2017 at 05:26:47AM -0400, Frediano Ziglio wrote: > > > > > > > > The quic code goes through a function pointer in two places in order to > > > > try to prevent the compiler from inlining code. This does not say why > > > > we don't want that code to be inlined. Removing this hack even made the > > > > resulting object file slightly smaller (600 bytes) on my fedora 26. > > > > > > > > > > I agree the hack is ugly. Was documented but looking at the code > > > for me is pretty clear. Developers want to make sure that > > > write_io_word is inlined while __write_io_word is not. > > > > Yes, I got that far, but *why* was it desired? Is this just a matter > > of code size? If yes, how bad was it at the time, is it better now? If > > not, what was the problem this was solving? This is what I meant by > > "undocumented" > > > > Christophe > > > > The how bad is something you should provide in the rationale, > remove an optimization because you don't understand it and is > not documented is IMHO a bad rationale. Better to leave the > code as it. You are the one saying this was added for optimization purpose, what I'm saying is that we have *no idea* if this was added for that, or to workaround a compiler bug, or something else. Note that this commit says it's removing a hack, not an optimization. If we want this function not to be inlined, we have better ways of achieving that, so I still want to remove that particular code. Iirc I ran some timing comparisons with/without that hack, and there were no significant differences (sometimes was faster, sometimes slower depending on the run). > You have to consider all the code around. write_io_word is > called only by encode, both are declared inline and encode > is called in many places. The author wanted to prevent compiler > to inline __write_io_word all these time and at the same time > help inlining encode. Beside the old way in which is implemented > it can be a big hint to the compiler. The compiler does some > statistics on code generated to inline and if is too much it > does not inline. Reducing code to inline encode (and write_io_word) > helps to keep the inlining. Considering the time this code was > written (I don't think compilers had the options to do these > declarations) it makes sense. > So the main target was making sure encode and write_io_work are > inlined (same for read path). The bigger object file you got after removing the inline would hint that the inlining is still taking place even without it. Christophe
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel