[Spice-devel] [PATCH spice-gtk] If replace me, should not do refcount plus one

[Frediano Ziglio]

> OK, I have modified it.

> > From 87be8732bf36038c9ba9faabb1aae58f7b830d05 Mon Sep 17 00:00:00 2001
> 
> > From: =?UTF-8?q?=E4=B9=90=E4=B9=89=E5=8D=8E?= <yueyihua at os-easy.com>
> 
> > Date: Thu, 12 Jul 2018 13:16:53 +0800
> 
> > Subject: [PATCH] Fix image cache memory usage
> 

> > When image flag is SPICE_IMAGE_FLAGS_CACHE_REPLACE_ME, and if found it
> 
> > in hash table, we will do refcount increase, but this image will only
> 
> > destroy once, so the cache will usage more and more memory. In this
> 
> > circumstance, we should not increase refconut.
> 

Was the removal of refcount copy on purpose or a mistake? 

typo: refconut -> refcount 

Frediano 

> > ---
> 
> > src/channel-display.c | 5 ++++-
> 
> > src/spice-channel-cache.h | 8 ++++++++
> 
> > 2 files changed, 12 insertions(+), 1 deletion(-)
> 

> > diff --git a/src/channel-display.c b/src/channel-display.c
> 
> > index 7c14c76..ebb8695 100755
> 
> > --- a/src/channel-display.c
> 
> > +++ b/src/channel-display.c
> 
> > @@ -659,7 +659,10 @@ static void image_put_lossy(SpiceImageCache *cache,
> > uint64_t id,
> 
> > static void image_replace_lossy(SpiceImageCache *cache, uint64_t id,
> 
> > pixman_image_t *surface)
> 
> > {
> 
> > - image_put(cache, id, surface);
> 
> > + SpiceDisplayChannelPrivate *c =
> 
> > + SPICE_CONTAINEROF(cache, SpiceDisplayChannelPrivate, image_cache);
> 
> > +
> 
> > + cache_replace_lossy(c->images, id, pixman_image_ref(surface), FALSE);
> 
> > }
> 
> > static pixman_image_t* image_get_lossless(SpiceImageCache *cache, uint64_t
> > id)
> 
> > diff --git a/src/spice-channel-cache.h b/src/spice-channel-cache.h
> 
> > index ca1fd5d..da88a39 100755
> 
> > --- a/src/spice-channel-cache.h
> 
> > +++ b/src/spice-channel-cache.h
> 
> > @@ -101,6 +101,14 @@ static inline void cache_add_lossy(display_cache
> > *cache,
> > uint64_t id,
> 
> > g_hash_table_replace(cache->table, item, value);
> 
> > }
> 
> > +static inline void cache_replace_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->table, item, value);
> 
> > +}
> 
> > +
> 
> > static inline void cache_add(display_cache *cache, uint64_t id, gpointer
> > value)
> 
> > {
> 
> > cache_add_lossy(cache, id, value, FALSE);
> 
> > --
> 
> > 2.5.5
> 

> > From: Christophe Fergeau
> 
> > Date: 2018-07-17 18:07
> 
> > To: Frediano Ziglio
> 
> > CC: spice-devel ; 乐义华
> 
> > Subject: Re: [Spice-devel] [PATCH spice-gtk] If replace me, should not do
> > refcount plus one
> 
> > On Tue, Jul 17, 2018 at 08:17:53AM +0100, Frediano Ziglio wrote:
> 
> > > From: 乐义华 <yueyihua at os-easy.com>
> 
> > >
> 
> > > When an image with SPICE_IMAGE_FLAGS_CACHE_REPLACE_ME is received the
> 
> > > reference counter should stay the same to avoid the cache to grow
> 
> > > continously.
> 
> > >
> 
> > > This fixes https://gitlab.freedesktop.org/spice/spice-gtk/issues/72
> 
> > Can you reword the shortlog a bit?
> 
> > Christophe
> 
> > >
> 
> > > Signed-off-by: 乐义华 <yueyihua at os-easy.com>
> 
> > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> 
> > > ---
> 
> > > src/channel-display.c | 5 ++++-
> 
> > > src/spice-channel-cache.h | 17 +++++++++++++++++
> 
> > > 2 files changed, 21 insertions(+), 1 deletion(-)
> 
> > >
> 
> > > diff --git a/src/channel-display.c b/src/channel-display.c
> 
> > > index 44ba0439..138cd8ce 100644
> 
> > > --- a/src/channel-display.c
> 
> > > +++ b/src/channel-display.c
> 
> > > @@ -788,7 +788,10 @@ static void image_put_lossy(SpiceImageCache *cache,
> > > uint64_t id,
> 
> > > static void image_replace_lossy(SpiceImageCache *cache, uint64_t id,
> 
> > > pixman_image_t *surface)
> 
> > > {
> 
> > > - image_put(cache, id, surface);
> 
> > > + SpiceDisplayChannelPrivate *c =
> 
> > > + SPICE_CONTAINEROF(cache, SpiceDisplayChannelPrivate, image_cache);
> 
> > > +
> 
> > > + cache_replace_lossy(c->images, id, pixman_image_ref(surface), FALSE);
> 
> > > }
> 
> > >
> 
> > > static pixman_image_t* image_get_lossless(SpiceImageCache *cache,
> > > uint64_t
> > > id)
> 
> > > diff --git a/src/spice-channel-cache.h b/src/spice-channel-cache.h
> 
> > > index 75cc2cd8..fc25c354 100644
> 
> > > --- a/src/spice-channel-cache.h
> 
> > > +++ b/src/spice-channel-cache.h
> 
> > > @@ -101,6 +101,23 @@ static inline void cache_add_lossy(display_cache
> > > *cache, uint64_t id,
> 
> > > g_hash_table_replace(cache->table, item, value);
> 
> > > }
> 
> > >
> 
> > > +static inline void cache_replace_lossy(display_cache *cache, uint64_t
> > > id,
> 
> > > + gpointer value, gboolean lossy)
> 
> > > +{
> 
> > > + display_cache_item *item = cache_item_new(id, lossy);
> 
> > > +
> 
> > > + // If image is currently in the table consider its reference count
> > > before
> > > replacing it
> 
> > > + if (cache->ref_counted) {
> 
> > > + display_cache_item *current_item;
> 
> > > + gpointer current_image;
> 
> > > + if (g_hash_table_lookup_extended(cache->table, &id, (gpointer*)
> > > &current_item,
> 
> > > + &current_image)) {
> 
> > > + item->ref_count = current_item->ref_count;
> 
> > > + }
> 
> > > + }
> 
> > > + g_hash_table_replace(cache->table, item, value);
> 
> > > +}
> 
> > > +
> 
> > > static inline void cache_add(display_cache *cache, uint64_t id, gpointer
> > > value)
> 
> > > {
> 
> > > cache_add_lossy(cache, id, value, FALSE);
> 
> > > --
> 
> > > 2.17.1
> 
> > >
> 
> > > _______________________________________________
> 
> > > Spice-devel mailing list
> 
> > > Spice-devel at lists.freedesktop.org
> 
> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180718/1fabe55f/attachment.html>