From: Marc-André Lureau <marcandre.lureau@xxxxxxxxx>
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 +++++++-----------------
spice-common | 2 +-
6 files changed, 103 insertions(+), 221 deletions(-)
diff --git a/gtk/channel-cursor.c b/gtk/channel-cursor.c
index e4a996b..eb53843 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;
};
@@ -67,7 +67,6 @@ enum {
static guint signals[SPICE_CURSOR_LAST_SIGNAL];
static void spice_cursor_handle_msg(SpiceChannel *channel, SpiceMsgIn *msg);
-static void delete_cursor_all(SpiceChannel *channel);
static display_cursor * display_cursor_ref(display_cursor *cursor);
static void display_cursor_unref(display_cursor *cursor);
@@ -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 const spice_msg_handler cursor_handlers[] = {
diff --git a/gtk/channel-display.c b/gtk/channel-display.c
index 03ae535..a69f14f 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;
}
diff --git a/spice-common b/spice-common
index fc27fb2..7aef065 160000
--- a/spice-common
+++ b/spice-common
@@ -1 +1 @@
-Subproject commit fc27fb20b8ecd3e07809aec884f99f2121712bc9
+Subproject commit 7aef06577e47965bbe93e0a054857a562d2fde5a