[Spice-devel] [spice-gtk PATCH] This adds reference counting to cached images.

[Sandy Stutsman]

Hello

----- Original Message -----
> From: "Marc-André Lureau" <mlureau at redhat.com>
> To: "Sandy Stutsman" <sstutsma at redhat.com>
> Cc: spice-devel at lists.freedesktop.org
> Sent: Friday, June 19, 2015 6:58:36 PM
> Subject: Re: [Spice-devel] [spice-gtk PATCH] This adds reference counting to cached images.
> 
> Hi
> 
> ----- Original Message -----
> > Windows guests with multi-monitors will often send down the same image
> > via different channels. If these instances are not counted, one channel
> > can delete an image before all the rest of the channels are finished with
> > it. In this case, the client will hang.
> > 
> 
> This looks like a change in the protocol.
> - an image with an existing id will no longer replace it
> - there is extra data being sent by the server
> - this looks like a server bug to me if an image is being removed and later
> referenced
> 
> Could this be addressed on server side to not change the current behaviour
> and avoid unnecessary data being sent to the client?
> 

Actually, when a new image arrives with an id that is currently in the cache,
it will replace the older one. It will just be created with a +1 reference
count.

In the case I've been debugging, the image in question is the Windows wallpaper.
As there is a driver instance for each monitor, each driver is sending 
down it's own copy of the same image, with the same id. As one channel may be 
busier than another there is no way to guarantee the order that the client 
will handle commands from the server.  What I'm seeing is this:
 On the server side
    Monitor 1 sends image A
    Monitor 1 sends mulitple render commands for  image A
    Monitor 1 removes image A
    Monitor 2 sends image A
    Monitor 2 sends render command for image A ...

  What happens on the client end:
    Monitor 1 adds Image A added to cache 
    Monitor 1 starts rendering from image A
    Monitor 2 adds Image A to cache- has the same id, so just replaces it
    Monitor 1 removes image A - same id, so the id is no longer in the cache
    Monitor 2 hangs waiting to render image A - which is no longer in the cache

In the above sequence, I don't think there is a way to prevent the image from 
being sent 2x from the server.  What helps keep this to a minimum is to 
lock the server cache for the whole of the call to fill_bits.  If only the
pixmap_cache_xxx calls are locked it is possible, in the case that two
channels are both executing in the fill_bits call, that the server will end
up sending the same image twice.

I know this seems like a lot of hand waving but I think the basic problem
is handling the Windows Multi-Monitor/multi-channel case where the channels 
share a number of common images.

> > Addresses bug: https://bugzilla.redhat.com/show_bug.cgi?id=1194354
> > ---
> > src/spice-channel-cache.h | 60
> > ++++++++++++++++++++++++++++++++++++-----------
> > src/spice-session.c | 2 +-
> > 2 files changed, 47 insertions(+), 15 deletions(-)
> > 
> > diff --git a/src/spice-channel-cache.h b/src/spice-channel-cache.h
> > index 17775e6..968e661 100644
> > --- a/src/spice-channel-cache.h
> > +++ b/src/spice-channel-cache.h
> > @@ -27,15 +27,20 @@ G_BEGIN_DECLS
> > typedef struct display_cache_item {
> > guint64 id;
> > gboolean lossy;
> > + uint32_t ref_count;
> > } display_cache_item;
> > 
> > -typedef GHashTable display_cache;
> > +typedef struct display_cache {
> > + GHashTable *table;
> > + gboolean ref_counted;
> > +}display_cache;
> > 
> > static inline display_cache_item* cache_item_new(guint64 id, gboolean
> > lossy)
> > {
> > display_cache_item *self = g_slice_new(display_cache_item);
> > self->id = id;
> > self->lossy = lossy;
> > + self->ref_count = 1;
> > return self;
> > }
> > 
> > @@ -46,18 +51,24 @@ static inline void
> > cache_item_free(display_cache_item *self)
> > 
> > static inline display_cache* cache_new(GDestroyNotify value_destroy)
> > {
> > - GHashTable* self;
> > -
> > - self = g_hash_table_new_full(g_int64_hash, g_int64_equal,
> > - (GDestroyNotify)cache_item_free,
> > - value_destroy);
> > -
> > + display_cache * self = g_slice_new(display_cache);
> > + self->table = g_hash_table_new_full(g_int64_hash, g_int64_equal,
> > + (GDestroyNotify) cache_item_free,
> > + value_destroy);
> > + self->ref_counted = FALSE;
> > return self;
> > }
> > 
> > +static inline display_cache * cache_image_new(GDestroyNotify
> > value_destroy)
> > +{
> > + display_cache * self = cache_new(value_destroy);
> > + self->ref_counted = TRUE;
> > + return self;
> > +};
> > +
> > static inline gpointer cache_find(display_cache *cache, uint64_t id)
> > {
> > - return g_hash_table_lookup(cache, &id);
> > + return g_hash_table_lookup(cache->table, &id);
> > }
> > 
> > static inline gpointer cache_find_lossy(display_cache *cache, uint64_t
> > id, gboolean *lossy)
> > @@ -65,7 +76,7 @@ static inline gpointer cache_find_lossy(display_cache
> > *cache, uint64_t id, gbool
> > gpointer value;
> > display_cache_item *item;
> > 
> > - if (!g_hash_table_lookup_extended(cache, &id, (gpointer*)&item, &value))
> > + if (!g_hash_table_lookup_extended(cache->table, &id, (gpointer*)&item,
> > &value))
> > return NULL;
> > 
> > *lossy = item->lossy;
> > @@ -77,8 +88,17 @@ static inline void cache_add_lossy(display_cache
> > *cache, uint64_t id,
> > gpointer value, gboolean lossy)
> > {
> > display_cache_item *item = cache_item_new(id, lossy);
> > -
> > - g_hash_table_replace(cache, item, value);
> > + display_cache_item *current_item;
> > + gpointer current_image;
> > +
> > + //If image is currently in the table add its reference count before
> > replacing it
> > + if(cache->ref_counted) {
> > + if(g_hash_table_lookup_extended(cache->table, &id, (gpointer*) ¤t_item,
> > + (gpointer*) ¤t_image)) {
> > + item->ref_count = current_item->ref_count + 1;
> > + }
> > + }
> > + g_hash_table_replace(cache->table, item, value);
> > }
> > 
> > static inline void cache_add(display_cache *cache, uint64_t id, gpointer
> > value)
> > @@ -88,17 +108,29 @@ static inline void cache_add(display_cache *cache,
> > uint64_t id, gpointer value)
> > 
> > static inline gboolean cache_remove(display_cache *cache, uint64_t id)
> > {
> > - return g_hash_table_remove(cache, &id);
> > + display_cache_item * item;
> > + gpointer value;
> > +
> > + if( g_hash_table_lookup_extended(cache->table, &id, (gpointer*) &item,
> > &value)) {
> > + --item->ref_count;
> > + if(!cache->ref_counted || !item->ref_count ) {
> > + return g_hash_table_remove(cache->table, &id);
> > + }
> > + }
> > + else {
> > + return FALSE;
> > + }
> > + return TRUE;
> > }
> > 
> > static inline void cache_clear(display_cache *cache)
> > {
> > - g_hash_table_remove_all(cache);
> > + g_hash_table_remove_all(cache->table);
> > }
> > 
> > static inline void cache_unref(display_cache *cache)
> > {
> > - g_hash_table_unref(cache);
> > + g_hash_table_unref(cache->table);
> > }
> > 
> > G_END_DECLS
> > diff --git a/src/spice-session.c b/src/spice-session.c
> > index 778d82a..a252448 100644
> > --- a/src/spice-session.c
> > +++ b/src/spice-session.c
> > @@ -257,7 +257,7 @@ static void spice_session_init(SpiceSession *session)
> > g_free(channels);
> > 
> > ring_init(&s->channels);
> > - s->images = cache_new((GDestroyNotify)pixman_image_unref);
> > + s->images = cache_image_new((GDestroyNotify)pixman_image_unref);
> > s->glz_window = glz_decoder_window_new();
> > update_proxy(session, NULL);
> > }
> > --
> > 1.9.5.msysgit.0
> > 
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> > 
>