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

[Christophe Fergeau]

On Wed, Sep 14, 2016 at 06:30:42AM -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!
> 
> This patch is not related to GQueue nor to optimization.
> This fixes a regression introduced in the first patch.

If the regression which is fixed is *not* a
performance/complexity regression, does it belong to that patch, or
could it be moved to a different one?
Note that I'm not saying "don't fix the regression", just saying that
this for loop is far too magic, hopefully it's possible to fix it
differently?

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

I think I just missed your initial comments/patches, feel free to ping
again when important patches are getting missed. Also, if this patch
fixes a non-performance bug, an explanation would be useful to ease
review/get more focused review :)

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/20160914/9a17eae1/attachment-0001.sig>