[Spice-devel] [spice-common v2 12/13] quic: Remove undocumented 'no-inline' hack

[Christophe Fergeau]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170802/e6f7e178/attachment.sig>