[Spice-devel] [PATCH 2/2] Optimise client pipe passing pipe position instead of data
Frediano Ziglio
fziglio at redhat.com
Wed Sep 14 10:30:42 UTC 2016
>
> 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.
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?
> > Drawable *drawable;
> > RedDrawablePipeItem *dpi = NULL;
> > int depend_found = FALSE;
> > + RedPipeItem *item = l->data;
> >
> > + item_pos = l;
> > l = l->next;
> > - item = cur->data;
> > if (item->type == RED_PIPE_ITEM_TYPE_DRAW) {
> > dpi = SPICE_CONTAINEROF(item, RedDrawablePipeItem,
> > dpi_pipe_item);
> > drawable = dpi->drawable;
> > @@ -97,7 +95,7 @@ int
> > dcc_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc, int
> > surface
> > }
> >
> > if (drawable->surface_id == surface_id) {
> > - red_channel_client_pipe_remove_and_release(rcc, item);
> > + red_channel_client_pipe_remove_and_release_pos(rcc, item_pos);
> > continue;
> > }
> >
> > @@ -122,8 +120,8 @@ int
> > dcc_clear_surface_drawables_from_pipe(DisplayChannelClient *dcc, int
> > surface
> > return TRUE;
> > }
> >
> > - if (item) {
> > - return
> > red_channel_client_wait_pipe_item_sent(RED_CHANNEL_CLIENT(dcc), item,
> > + if (item_pos) {
> > + return
> > red_channel_client_wait_pipe_item_sent(RED_CHANNEL_CLIENT(dcc), item_pos,
> > COMMON_CLIENT_TIMEOUT);
> > } else {
> > /*
> > @@ -168,7 +166,7 @@ void dcc_create_surface(DisplayChannelClient *dcc, int
> > surface_id)
> > RedImageItem *dcc_add_surface_area_image(DisplayChannelClient *dcc,
> > int surface_id,
> > SpiceRect *area,
> > - RedPipeItem *pos,
> > + GList *pos,
> > int can_lossy)
>
> Here we totally lost any information on what 'GList *pos' is.
> GList *pipe_item_pos maybe?
>
Make sense.
> > {
> > DisplayChannel *display = DCC_TO_DC(dcc);
> > @@ -219,7 +217,7 @@ RedImageItem
> > *dcc_add_surface_area_image(DisplayChannelClient *dcc,
> > }
> >
> > if (pos) {
> > - red_channel_client_pipe_add_after(RED_CHANNEL_CLIENT(dcc),
> > &item->base, pos);
> > + red_channel_client_pipe_add_after_pos(RED_CHANNEL_CLIENT(dcc),
> > &item->base, pos);
> > } else {
> > red_channel_client_pipe_add(RED_CHANNEL_CLIENT(dcc), &item->base);
> > }
> > diff --git a/server/dcc.h b/server/dcc.h
> > index d08e413..7f93186 100644
> > --- a/server/dcc.h
> > +++ b/server/dcc.h
> > @@ -129,7 +129,7 @@ void dcc_push_surface_image
> > (DisplayCha
> > RedImageItem * dcc_add_surface_area_image
> > (DisplayChannelClient *dcc,
> > int
> > surface_id,
> > SpiceRect
> > *area,
> > -
> > RedPipeItem
> > *pos,
> > +
> > GList
> > *pos,
> > int
> > can_lossy);
> > void dcc_palette_cache_reset
> > (DisplayChannelClient *dcc);
> > void dcc_palette_cache_palette
> > (DisplayChannelClient *dcc,
> > diff --git a/server/red-channel-client.c b/server/red-channel-client.c
> > index 89ab679..563d888 100644
> > --- a/server/red-channel-client.c
> > +++ b/server/red-channel-client.c
> > @@ -1311,20 +1311,29 @@ void
> > red_channel_client_pipe_add_push(RedChannelClient *rcc, RedPipeItem *item)
> > red_channel_client_push(rcc);
> > }
> >
> > +void red_channel_client_pipe_add_after_pos(RedChannelClient *rcc,
> > + RedPipeItem *item,
> > + GList *pos)
>
> Same comment here, though this probably can be guessed from the method
> name.
>
Not much different from red_channel_client_pipe_add_after, maybe renaming
the argument (pos -> pipe_item_pos) would be enough even here.
> Christophe
>
Frediano
More information about the Spice-devel
mailing list