[Spice-devel] [PATCH spice-gtk 10/14] gtk: use GHashTable in display_cache

Marc-André Lureau marcandre.lureau at gmail.com
Thu Sep 12 05:09:18 PDT 2013


From: Marc-André Lureau <marcandre.lureau at gmail.com>

The cache code isn't very quick, it shows up in profilers.  Using
GHashTable allows to simplify the code while making it faster.
---
 gtk/channel-cursor.c      |  56 +++++++----------------
 gtk/channel-display.c     |  93 +++++++++++---------------------------
 gtk/spice-channel-cache.h | 111 +++++++++++++++++-----------------------------
 gtk/spice-session-priv.h  |   4 +-
 gtk/spice-session.c       |  58 +++++++-----------------
 5 files changed, 102 insertions(+), 220 deletions(-)

diff --git a/gtk/channel-cursor.c b/gtk/channel-cursor.c
index 5d2db84..0443b9f 100644
--- a/gtk/channel-cursor.c
+++ b/gtk/channel-cursor.c
@@ -51,7 +51,7 @@ struct display_cursor {
 };
 
 struct _SpiceCursorChannelPrivate {
-    display_cache               cursors;
+    display_cache               *cursors;
     gboolean                    init_done;
 };
 
@@ -66,7 +66,6 @@ enum {
 
 static guint signals[SPICE_CURSOR_LAST_SIGNAL];
 
-static void delete_cursor_all(SpiceChannel *channel);
 static display_cursor * display_cursor_ref(display_cursor *cursor);
 static void display_cursor_unref(display_cursor *cursor);
 static void channel_set_handlers(SpiceChannelClass *klass);
@@ -81,12 +80,14 @@ static void spice_cursor_channel_init(SpiceCursorChannel *channel)
 
     c = channel->priv = SPICE_CURSOR_CHANNEL_GET_PRIVATE(channel);
 
-    cache_init(&c->cursors, "cursor");
+    c->cursors = cache_new((GDestroyNotify)display_cursor_unref);
 }
 
 static void spice_cursor_channel_finalize(GObject *obj)
 {
-    delete_cursor_all(SPICE_CHANNEL(obj));
+    SpiceCursorChannelPrivate *c = SPICE_CURSOR_CHANNEL_GET_PRIVATE(obj);
+
+    g_clear_pointer(&c->cursors, cache_unref);
 
     if (G_OBJECT_CLASS(spice_cursor_channel_parent_class)->finalize)
         G_OBJECT_CLASS(spice_cursor_channel_parent_class)->finalize(obj);
@@ -97,7 +98,7 @@ static void spice_cursor_channel_reset(SpiceChannel *channel, gboolean migrating
 {
     SpiceCursorChannelPrivate *c = SPICE_CURSOR_CHANNEL(channel)->priv;
 
-    delete_cursor_all(channel);
+    cache_clear(c->cursors);
     c->init_done = FALSE;
 
     SPICE_CHANNEL_CLASS(spice_cursor_channel_parent_class)->channel_reset(channel, migrating);
@@ -359,7 +360,6 @@ static display_cursor *set_cursor(SpiceChannel *channel, SpiceCursor *scursor)
 {
     SpiceCursorChannelPrivate *c = SPICE_CURSOR_CHANNEL(channel)->priv;
     SpiceCursorHeader *hdr = &scursor->header;
-    display_cache_item *item;
     display_cursor *cursor;
     size_t size;
     gint i, pix_mask, pix;
@@ -378,9 +378,9 @@ static display_cursor *set_cursor(SpiceChannel *channel, SpiceCursor *scursor)
                   hdr->width, hdr->height);
 
     if (scursor->flags & SPICE_CURSOR_FLAGS_FROM_CACHE) {
-        item = cache_find(&c->cursors, hdr->unique);
-        g_return_val_if_fail(item != NULL, NULL);
-        return display_cursor_ref(item->ptr);
+        cursor = cache_find(c->cursors, hdr->unique);
+        g_return_val_if_fail(cursor != NULL, NULL);
+        return display_cursor_ref(cursor);
     }
 
     g_return_val_if_fail(scursor->data_size != 0, NULL);
@@ -453,36 +453,12 @@ static display_cursor *set_cursor(SpiceChannel *channel, SpiceCursor *scursor)
 
 cache_add:
     if (cursor && (scursor->flags & SPICE_CURSOR_FLAGS_CACHE_ME)) {
-        display_cursor_ref(cursor);
-        item = cache_add(&c->cursors, hdr->unique);
-        item->ptr = cursor;
+        cache_add(c->cursors, hdr->unique, display_cursor_ref(cursor));
     }
 
     return cursor;
 }
 
-static void delete_cursor_one(SpiceChannel *channel, display_cache_item *item)
-{
-    SpiceCursorChannelPrivate *c = SPICE_CURSOR_CHANNEL(channel)->priv;
-
-    display_cursor_unref((display_cursor*)item->ptr);
-    cache_del(&c->cursors, item);
-}
-
-static void delete_cursor_all(SpiceChannel *channel)
-{
-    SpiceCursorChannelPrivate *c = SPICE_CURSOR_CHANNEL(channel)->priv;
-    display_cache_item *item;
-
-    for (;;) {
-        item = cache_get_lru(&c->cursors);
-        if (item == NULL) {
-            return;
-        }
-        delete_cursor_one(channel, item);
-    }
-}
-
 /* coroutine context */
 static void emit_cursor_set(SpiceChannel *channel, display_cursor *cursor)
 {
@@ -502,7 +478,7 @@ static void cursor_handle_init(SpiceChannel *channel, SpiceMsgIn *in)
 
     g_return_if_fail(c->init_done == FALSE);
 
-    delete_cursor_all(channel);
+    cache_clear(c->cursors);
     cursor = set_cursor(channel, &init->cursor);
     c->init_done = TRUE;
     if (cursor)
@@ -520,7 +496,7 @@ static void cursor_handle_reset(SpiceChannel *channel, SpiceMsgIn *in)
 
     CHANNEL_DEBUG(channel, "%s, init_done: %d", __FUNCTION__, c->init_done);
 
-    delete_cursor_all(channel);
+    cache_clear(c->cursors);
     emit_main_context(channel, SPICE_CURSOR_RESET);
     c->init_done = FALSE;
 }
@@ -584,18 +560,18 @@ static void cursor_handle_inval_one(SpiceChannel *channel, SpiceMsgIn *in)
 {
     SpiceCursorChannelPrivate *c = SPICE_CURSOR_CHANNEL(channel)->priv;
     SpiceMsgDisplayInvalOne *zap = spice_msg_in_parsed(in);
-    display_cache_item *item;
 
     g_return_if_fail(c->init_done == TRUE);
 
-    item = cache_find(&c->cursors, zap->id);
-    delete_cursor_one(channel, item);
+    cache_remove(c->cursors, zap->id);
 }
 
 /* coroutine context */
 static void cursor_handle_inval_all(SpiceChannel *channel, SpiceMsgIn *in)
 {
-    delete_cursor_all(channel);
+    SpiceCursorChannelPrivate *c = SPICE_CURSOR_CHANNEL(channel)->priv;
+
+    cache_clear(c->cursors);
 }
 
 static void channel_set_handlers(SpiceChannelClass *klass)
diff --git a/gtk/channel-display.c b/gtk/channel-display.c
index c8061ff..45d31c5 100644
--- a/gtk/channel-display.c
+++ b/gtk/channel-display.c
@@ -486,16 +486,8 @@ static void image_put(SpiceImageCache *cache, uint64_t id, pixman_image_t *image
 {
     SpiceDisplayChannelPrivate *c =
         SPICE_CONTAINEROF(cache, SpiceDisplayChannelPrivate, image_cache);
-    display_cache_item *item;
 
-    item = cache_find(c->images, id);
-    if (item) {
-        cache_ref(item);
-        return;
-    }
-
-    item = cache_add(c->images, id);
-    item->ptr = pixman_image_ref(image);
+    cache_add(c->images, id, pixman_image_ref(image));
 }
 
 typedef struct _WaitImageData
@@ -508,18 +500,16 @@ typedef struct _WaitImageData
 
 static gboolean wait_image(gpointer data)
 {
-    display_cache_item *item;
+    gboolean lossy;
     WaitImageData *wait = data;
     SpiceDisplayChannelPrivate *c =
         SPICE_CONTAINEROF(wait->cache, SpiceDisplayChannelPrivate, image_cache);
+    pixman_image_t *image = cache_find_lossy(c->images, wait->id, &lossy);
 
-    item = cache_find(c->images, wait->id);
-    if (item == NULL ||
-        (item->lossy && !wait->lossy))
+    if (!image || (lossy && !wait->lossy))
         return FALSE;
 
-    cache_used(c->images, item);
-    wait->image = pixman_image_ref(item->ptr);
+    wait->image = pixman_image_ref(image);
 
     return TRUE;
 }
@@ -538,58 +528,33 @@ static pixman_image_t *image_get(SpiceImageCache *cache, uint64_t id)
     return wait.image;
 }
 
-static void image_remove(SpiceImageCache *cache, uint64_t id)
-{
-    SpiceDisplayChannelPrivate *c =
-        SPICE_CONTAINEROF(cache, SpiceDisplayChannelPrivate, image_cache);
-    display_cache_item *item;
-
-    item = cache_find(c->images, id);
-    g_return_if_fail(item != NULL);
-    if (cache_unref(item)) {
-        pixman_image_unref(item->ptr);
-        cache_del(c->images, item);
-    }
-}
-
 static void palette_put(SpicePaletteCache *cache, SpicePalette *palette)
 {
     SpiceDisplayChannelPrivate *c =
         SPICE_CONTAINEROF(cache, SpiceDisplayChannelPrivate, palette_cache);
-    display_cache_item *item;
 
-    item = cache_add(c->palettes, palette->unique);
-    item->ptr = g_memdup(palette, sizeof(SpicePalette) +
-                         palette->num_ents * sizeof(palette->ents[0]));
+    cache_add(c->palettes, palette->unique,
+              g_memdup(palette, sizeof(SpicePalette) +
+                       palette->num_ents * sizeof(palette->ents[0])));
 }
 
 static SpicePalette *palette_get(SpicePaletteCache *cache, uint64_t id)
 {
     SpiceDisplayChannelPrivate *c =
         SPICE_CONTAINEROF(cache, SpiceDisplayChannelPrivate, palette_cache);
-    display_cache_item *item;
 
-    item = cache_find(c->palettes, id);
-    if (item) {
-        cache_ref(item);
-        return item->ptr;
-    }
-    return NULL;
+    /* here the returned pointer is weak, no ref given to caller.  it
+     * seems spice canvas usage is exclusively temporary, so it's ok
+     * (for now) */
+    return cache_find(c->palettes, id);
 }
 
 static void palette_remove(SpicePaletteCache *cache, uint32_t id)
 {
     SpiceDisplayChannelPrivate *c =
         SPICE_CONTAINEROF(cache, SpiceDisplayChannelPrivate, palette_cache);
-    display_cache_item *item;
 
-    item = cache_find(c->palettes, id);
-    if (item) {
-        if (cache_unref(item)) {
-            g_free(item->ptr);
-            cache_del(c->palettes, item);
-        }
-    }
+    cache_remove(c->palettes, id);
 }
 
 static void palette_release(SpicePaletteCache *cache, SpicePalette *palette)
@@ -603,30 +568,18 @@ static void image_put_lossy(SpiceImageCache *cache, uint64_t id,
 {
     SpiceDisplayChannelPrivate *c =
         SPICE_CONTAINEROF(cache, SpiceDisplayChannelPrivate, image_cache);
-    display_cache_item *item;
 
 #ifndef NDEBUG
     g_warn_if_fail(cache_find(c->images, id) == NULL);
 #endif
 
-    item = cache_add(c->images, id);
-    item->ptr = pixman_image_ref(surface);
-    item->lossy = TRUE;
+    cache_add_lossy(c->images, id, pixman_image_ref(surface), TRUE);
 }
 
 static void image_replace_lossy(SpiceImageCache *cache, uint64_t id,
                                 pixman_image_t *surface)
 {
-    SpiceDisplayChannelPrivate *c =
-        SPICE_CONTAINEROF(cache, SpiceDisplayChannelPrivate, image_cache);
-    display_cache_item *item;
-
-    item = cache_find(c->images, id);
-    g_return_if_fail(item != NULL);
-
-    pixman_image_unref(item->ptr);
-    item->ptr = pixman_image_ref(surface);
-    item->lossy = FALSE;
+    image_put(cache, id, surface);
 }
 
 static pixman_image_t* image_get_lossless(SpiceImageCache *cache, uint64_t id)
@@ -968,7 +921,7 @@ static void display_handle_reset(SpiceChannel *channel, SpiceMsgIn *in)
     if (surface != NULL)
         surface->canvas->ops->clear(surface->canvas);
 
-    spice_session_palettes_clear(spice_channel_get_session(channel));
+    cache_clear(c->palettes);
 
     c->mark = FALSE;
     emit_main_context(channel, SPICE_DISPLAY_MARK, FALSE);
@@ -997,9 +950,12 @@ static void display_handle_inv_list(SpiceChannel *channel, SpiceMsgIn *in)
     int i;
 
     for (i = 0; i < list->count; i++) {
+        guint64 id = list->resources[i].id;
+
         switch (list->resources[i].type) {
         case SPICE_RES_TYPE_PIXMAP:
-            image_remove(&c->image_cache, list->resources[i].id);
+            if (!cache_remove(c->images, id))
+                SPICE_DEBUG("fail to remove image %" G_GUINT64_FORMAT, id);
             break;
         default:
             g_return_if_reached();
@@ -1011,9 +967,10 @@ static void display_handle_inv_list(SpiceChannel *channel, SpiceMsgIn *in)
 /* coroutine context */
 static void display_handle_inv_pixmap_all(SpiceChannel *channel, SpiceMsgIn *in)
 {
-    spice_channel_handle_wait_for_channels(channel, in);
+    SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
 
-    spice_session_images_clear(spice_channel_get_session(channel));
+    spice_channel_handle_wait_for_channels(channel, in);
+    cache_clear(c->images);
 }
 
 /* coroutine context */
@@ -1028,7 +985,9 @@ static void display_handle_inv_palette(SpiceChannel *channel, SpiceMsgIn *in)
 /* coroutine context */
 static void display_handle_inv_palette_all(SpiceChannel *channel, SpiceMsgIn *in)
 {
-    spice_session_palettes_clear(spice_channel_get_session(channel));
+    SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
+
+    cache_clear(c->palettes);
 }
 
 /* ------------------------------------------------------------------ */
diff --git a/gtk/spice-channel-cache.h b/gtk/spice-channel-cache.h
index 3f39877..17775e6 100644
--- a/gtk/spice-channel-cache.h
+++ b/gtk/spice-channel-cache.h
@@ -25,109 +25,80 @@
 G_BEGIN_DECLS
 
 typedef struct display_cache_item {
-    RingItem                    hash_link;
-    RingItem                    lru_link;
-    uint64_t                    id;
-    uint32_t                    refcount;
-    void                        *ptr;
+    guint64                     id;
     gboolean                    lossy;
 } display_cache_item;
 
-typedef struct display_cache {
-    const char                  *name;
-    Ring                        hash[64];
-    Ring                        lru;
-    int                         nitems;
-} display_cache;
+typedef GHashTable display_cache;
 
-static inline void cache_init(display_cache *cache, const char *name)
+static inline display_cache_item* cache_item_new(guint64 id, gboolean lossy)
 {
-    int i;
+    display_cache_item *self = g_slice_new(display_cache_item);
+    self->id = id;
+    self->lossy = lossy;
+    return self;
+}
 
-    cache->name = name;
-    ring_init(&cache->lru);
-    for (i = 0; i < SPICE_N_ELEMENTS(cache->hash); i++) {
-        ring_init(&cache->hash[i]);
-    }
+static inline void cache_item_free(display_cache_item *self)
+{
+    g_slice_free(display_cache_item, self);
 }
 
-static inline Ring *cache_head(display_cache *cache, uint64_t id)
+static inline display_cache* cache_new(GDestroyNotify value_destroy)
 {
-    return &cache->hash[id % SPICE_N_ELEMENTS(cache->hash)];
+    GHashTable* self;
+
+    self = g_hash_table_new_full(g_int64_hash, g_int64_equal,
+                                 (GDestroyNotify)cache_item_free,
+                                 value_destroy);
+
+    return self;
 }
 
-static inline void cache_used(display_cache *cache, display_cache_item *item)
+static inline gpointer cache_find(display_cache *cache, uint64_t id)
 {
-    ring_remove(&item->lru_link);
-    ring_add(&cache->lru, &item->lru_link);
+    return g_hash_table_lookup(cache, &id);
 }
 
-static inline display_cache_item *cache_get_lru(display_cache *cache)
+static inline gpointer cache_find_lossy(display_cache *cache, uint64_t id, gboolean *lossy)
 {
+    gpointer value;
     display_cache_item *item;
-    RingItem *ring;
 
-    if (ring_is_empty(&cache->lru))
+    if (!g_hash_table_lookup_extended(cache, &id, (gpointer*)&item, &value))
         return NULL;
-    ring = ring_get_tail(&cache->lru);
-    item = SPICE_CONTAINEROF(ring, display_cache_item, lru_link);
-    return item;
-}
 
-static inline display_cache_item *cache_find(display_cache *cache, uint64_t id)
-{
-    display_cache_item *item;
-    RingItem *ring;
-    Ring *head;
-
-    head = cache_head(cache, id);
-    for (ring = ring_get_head(head); ring != NULL; ring = ring_next(head, ring)) {
-        item = SPICE_CONTAINEROF(ring, display_cache_item, hash_link);
-        if (item->id == id) {
-            return item;
-        }
-    }
-
-    SPICE_DEBUG("%s: %s %" PRIx64 " [not found]", __FUNCTION__,
-            cache->name, id);
-    return NULL;
+    *lossy = item->lossy;
+
+    return value;
 }
 
-static inline display_cache_item *cache_add(display_cache *cache, uint64_t id)
+static inline void cache_add_lossy(display_cache *cache, uint64_t id,
+                                   gpointer value, gboolean lossy)
 {
-    display_cache_item *item;
+    display_cache_item *item = cache_item_new(id, lossy);
 
-    item = spice_new0(display_cache_item, 1);
-    item->id = id;
-    item->refcount = 1;
-    ring_add(cache_head(cache, item->id), &item->hash_link);
-    ring_add(&cache->lru, &item->lru_link);
-    cache->nitems++;
+    g_hash_table_replace(cache, item, value);
+}
 
-    SPICE_DEBUG("%s: %s %" PRIx64 " (%d)", __FUNCTION__,
-            cache->name, id, cache->nitems);
-    return item;
+static inline void cache_add(display_cache *cache, uint64_t id, gpointer value)
+{
+    cache_add_lossy(cache, id, value, FALSE);
 }
 
-static inline void cache_del(display_cache *cache, display_cache_item *item)
+static inline gboolean cache_remove(display_cache *cache, uint64_t id)
 {
-    SPICE_DEBUG("%s: %s %" PRIx64, __FUNCTION__,
-            cache->name, item->id);
-    ring_remove(&item->hash_link);
-    ring_remove(&item->lru_link);
-    free(item);
-    cache->nitems--;
+    return g_hash_table_remove(cache, &id);
 }
 
-static inline void cache_ref(display_cache_item *item)
+static inline void cache_clear(display_cache *cache)
 {
-    item->refcount++;
+    g_hash_table_remove_all(cache);
 }
 
-static inline int cache_unref(display_cache_item *item)
+static inline void cache_unref(display_cache *cache)
 {
-    item->refcount--;
-    return item->refcount == 0;
+    g_hash_table_unref(cache);
 }
 
 G_END_DECLS
diff --git a/gtk/spice-session-priv.h b/gtk/spice-session-priv.h
index 5ed48dd..e175281 100644
--- a/gtk/spice-session-priv.h
+++ b/gtk/spice-session-priv.h
@@ -92,8 +92,8 @@ struct _SpiceSessionPrivate {
     guint             after_main_init;
     gboolean          migration_copy;
 
-    display_cache     images;
-    display_cache     palettes;
+    display_cache     *images;
+    display_cache     *palettes;
     SpiceGlzDecoderWindow *glz_window;
     int               images_cache_size;
     int               glz_window_size;
diff --git a/gtk/spice-session.c b/gtk/spice-session.c
index a9435f4..c050266 100644
--- a/gtk/spice-session.c
+++ b/gtk/spice-session.c
@@ -176,8 +176,8 @@ static void spice_session_init(SpiceSession *session)
     g_free(channels);
 
     ring_init(&s->channels);
-    cache_init(&s->images, "image");
-    cache_init(&s->palettes, "palette");
+    s->images = cache_new((GDestroyNotify)pixman_image_unref);
+    s->palettes = cache_new(g_free);
     s->glz_window = glz_decoder_window_new();
     update_proxy(session, NULL);
 }
@@ -219,35 +219,6 @@ spice_session_dispose(GObject *gobject)
         G_OBJECT_CLASS(spice_session_parent_class)->dispose(gobject);
 }
 
-G_GNUC_INTERNAL
-void spice_session_palettes_clear(SpiceSession *session)
-{
-    SpiceSessionPrivate *s = SPICE_SESSION_GET_PRIVATE(session);
-    g_return_if_fail(s != NULL);
-
-    for (;;) {
-        display_cache_item *item = cache_get_lru(&s->palettes);
-        if (item == NULL)
-            break;
-        cache_del(&s->palettes, item);
-    }
-}
-
-G_GNUC_INTERNAL
-void spice_session_images_clear(SpiceSession *session)
-{
-    SpiceSessionPrivate *s = SPICE_SESSION_GET_PRIVATE(session);
-    g_return_if_fail(s != NULL);
-
-    for (;;) {
-        display_cache_item *item = cache_get_lru(&s->images);
-        if (item == NULL)
-            break;
-        pixman_image_unref(item->ptr);
-        cache_del(&s->images, item);
-    }
-}
-
 static void
 spice_session_finalize(GObject *gobject)
 {
@@ -267,8 +238,8 @@ spice_session_finalize(GObject *gobject)
     g_strfreev(s->disable_effects);
     g_strfreev(s->secure_channels);
 
-    spice_session_palettes_clear(session);
-    spice_session_images_clear(session);
+    g_clear_pointer(&s->images, cache_unref);
+    g_clear_pointer(&s->palettes, cache_unref);
     glz_decoder_window_destroy(s->glz_window);
 
     g_clear_pointer(&s->pubkey, g_byte_array_unref);
@@ -1332,6 +1303,15 @@ gboolean spice_session_get_client_provided_socket(SpiceSession *session)
     return s->client_provided_sockets;
 }
 
+static void cache_clear_all(SpiceSession *self)
+{
+    SpiceSessionPrivate *s = SPICE_SESSION_GET_PRIVATE(self);
+
+    cache_clear(s->images);
+    cache_clear(s->palettes);
+    glz_decoder_window_clear(s->glz_window);
+}
+
 G_GNUC_INTERNAL
 void spice_session_switching_disconnect(SpiceSession *self)
 {
@@ -1353,9 +1333,7 @@ void spice_session_switching_disconnect(SpiceSession *self)
 
     g_warn_if_fail(!ring_is_empty(&s->channels)); /* ring_get_length() == 1 */
 
-    spice_session_palettes_clear(self);
-    spice_session_images_clear(self);
-    glz_decoder_window_clear(s->glz_window);
+    cache_clear_all(self);
 }
 
 G_GNUC_INTERNAL
@@ -1561,9 +1539,7 @@ void spice_session_migrate_end(SpiceSession *self)
         }
     }
 
-    spice_session_palettes_clear(self);
-    spice_session_images_clear(self);
-    glz_decoder_window_clear(s->glz_window);
+    cache_clear_all(self);
 
     /* send MIGRATE_END to target */
     out = spice_msg_out_new(s->cmain, SPICE_MSGC_MAIN_MIGRATE_END);
@@ -2119,9 +2095,9 @@ void spice_session_get_caches(SpiceSession *session,
     g_return_if_fail(s != NULL);
 
     if (images)
-        *images = &s->images;
+        *images = s->images;
     if (palettes)
-        *palettes = &s->palettes;
+        *palettes = s->palettes;
     if (glz_window)
         *glz_window = s->glz_window;
 }
-- 
1.8.3.1



More information about the Spice-devel mailing list