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

[Frediano Ziglio]

> 
> 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.

I doubt a compiler bug would cause this problem in 2 part of code.
Something else like? Inline is quite bound to optimizations and
the hack was documented to disable inlining which for me is
quite a big hint on the reason.

> Note that this commit says it's removing a hack, not an optimization.

Is an hack to add 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.

Sure, then we should do it in the right way.

> 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).
> 

This is a good rationale to change that code. But you need to add
how you did it and the statistics.

> > 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
> 

I though was clear my explanation but probably not. Is not only
a question of inlining but also of selective inlining. Kind
of more a quality thing.

I would personally use this code instead

static inline void write_io_word(Encoder *encoder)
{
    if (SPICE_UNLIKELY(encoder->io_now == encoder->io_end)) {
        more_io_words(encoder);
    }
    *(encoder->io_now++) = encoder->io_word;
}

and remove __write_io_word (similar for the other hunk).
And do some performance/code check.
As far as I know somebody could argue that SPICE_UNLIKELY is
not available for VS but I don't consider it a priority.

Frediano