[Spice-devel] [PATCH 2/2] Optimise client pipe passing pipe position instead of data

[Jonathon Jongsma]

On Wed, 2016-09-14 at 06:30 -0400, Frediano Ziglio wrote:
> > 
> > 
> > I haven't looked very carefully at the changes, but a few comments
> > about readability:
> > 
> > On Mon, Sep 12, 2016 at 04:32:03PM +0100, Frediano Ziglio wrote:
> > > 
> > > This avoid to have the search the data scanning all the
> > > queue changing the order of these operations from O(n) to O(1).
> > > 
> > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > ---
> > >  server/dcc-send.c           |  9 ++++-----
> > >  server/dcc.c                | 20 +++++++++-----------
> > >  server/dcc.h                |  2 +-
> > >  server/red-channel-client.c | 32 +++++++++++++++++++++++++----
> > > ---
> > >  server/red-channel-client.h |  4 +++-
> > >  5 files changed, 42 insertions(+), 25 deletions(-)
> > > 
> > > diff --git a/server/dcc.c b/server/dcc.c
> > > index 0d672df..ece37d9 100644
> > > --- a/server/dcc.c
> > > +++ b/server/dcc.c
> > > @@ -78,15 +77,14 @@ int
> > > dcc_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc,
> > > int
> > > surface
> > >         no other drawable depends on them */
> > >  
> > >      rcc = RED_CHANNEL_CLIENT(dcc);
> > > -    l = rcc->priv->pipe.head;
> > > -    while (l != NULL) {
> > > -        GList *cur = l;
> > > +    for (l = rcc->priv->pipe.head; l != NULL; item_pos = NULL) {
> > 
> > Please no :) the while() version might be better. The  for()
> > version
> > with the loop index being updated within the loop, but only after
> > doing
> > a bit of work with the previous value of the loop index is really
> > unexpected.
> > 
> 
> What can I say: I give up!

Give up? Why?

> 
> This patch is not related to GQueue nor to optimization.
> This fixes a regression introduced in the first patch.

Which patch was the regression introduced in? Was that patch merged
already?

> I spotted this regression the first time (some months ago) this
> patch was posted on the mailing list but no matter saying that some
> path touched some sensible code that required more attention, no
> matter all my consideration about complexity no one pick up again
> these patches and noted the issue.

I'm afraid that I don't remember a discussion from some months ago. If
there was a regression introduced by a patch that has already been
merged, then you should feel free to ping for review if something was
not getting reviewed. I wasn't aware of any regressions. 

On the other hand, if the regression was introduced in one of the
patches that are still under review, then I don't quite understand your
frustration. Isn't finding and fixing regressions part of what a review
is for?


> 
> Now I could think different causes:
> - we are too tired of these patches and we don't pay much attention;
> - we look more at style than behavior;
> - we are too busy to spend too much time on the review.
> Any of the reason do not seem really a good reason to me.
> Any other reason I don't see?
> 

Do you have suggestions for improving things?

Jonathon