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

[Christophe Fergeau]

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.

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

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

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/20160913/f800e994/attachment.sig>