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

[Frediano Ziglio]

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

Was introduced by one GList (now fixed, was not integrated).
Specifically was dcc_clear_surface_drawables_from_pipe.
In the initial code (before GList/GQueue) the item and iterator was represented
by the same variable item so being a single variable by definition was
coherent to itself. When GList was introduced there were 2 variable,
l and item_pos. Unfortunately after the loop item was used by not
being coherent with l at that time a regression was introduced.

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

Honestly I really don't understand why this was not spotted just by me.
These "exercises" are usually used in interviews from junior level and
didn't expect to take so much time and effort so I was really upset of
the few attention taken for these patches even when some changes are
touching sensible piece of software but probably my way to see the code
make quite easy for me to spot these issue that I found other people
not finding them a bit of a joke.
Maybe also some other frustration not related specifically to this.

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

I still think that more attention should be done to critical piece
of software.
On the code part I'm trying to introduce back some macro to avoid
manually mistake iterating lists.
On the long run I had different changes and branches to integrate
mostly born extending some changes introduced in different patches.
Just an example of "extension" now that RedPipeItem has no link in
it... it's not more a PipeItem and all classes should probably be
renamed, not counting the changes that can be made on multiple
clients (a subject we should probably discuss about).

Frediano