[Spice-devel] [PATCH 4/5] Refactory Glz RedDrawable retention code
Jonathon Jongsma
jjongsma at redhat.com
Tue Jun 21 20:07:09 UTC 2016
On Fri, 2016-06-17 at 14:58 +0100, Frediano Ziglio wrote:
> The code was quite complicated.
> - a GlzEncDictImageContext was bound to a GlzDrawableInstanceItem
> (1-1 relation);
> - multiple GlzDrawableInstanceItem were attached to RedGlzDrawable;
> - multiple RedGlzDrawable (one for RedClient) were attached to Drawable
> using GlzImageRetention;
> - OOM code require all Glz dictionary used by DisplayChannel to be locked
> at the same time;
> - drawable count computation during OOM was not corrent in case of
> multiple clients;
> - not clear why to call glz_retention_free_drawables or
> glz_retention_free_drawables.
Same function mentioned twice here ^
"glz_retention_free_drawables or glz_retention_free_drawables"
>
> Now:
> - a GlzEncDictImageContext is bound to a GlzContext (still 1-1 relation);
> - multiple GlzContext are attached to a Drawable using GlzImageRetention;
> - there is only a glz_retention_free to be called when the structure
> must be destroyed.
>
> Implementation detail:
> - red_drawable_unref returns a gboolean to indicate if the object
> was really freed;
It actually returns FALSE if it was freed, and TRUE if it was not freed. This
comment makes it sound like it is the opposite.
> - glz_drawable_count was removed as there are no more RedGlzDrawable;
> - image_encoders_glz_encode_lock and image_encoders_glz_encode_unlock
> were removed and now the locking is handled entirely by ImageEncoders;
> - instead of marking GlzContext/GlzDrawableInstanceItem changing
> has_drawable flag contexts are moved to a glz_orphans ring;
this sentence is a bit unclear. Will try to suggest alternate wording after I
look at the code and determine what it actually does.
> - code is smaller (100 lines less in image-encoders.c) and I hope easier
> to read.
>
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
> server/display-channel.c | 44 ++++----
> server/image-encoders.c | 270 ++++++++++++++------------------------------
> ---
> server/image-encoders.h | 17 +--
> server/red-worker.c | 11 +-
> server/red-worker.h | 2 +-
> 5 files changed, 117 insertions(+), 227 deletions(-)
>
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 073d45e..e149eb2 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -1167,7 +1167,7 @@ void display_channel_free_glz_drawables(DisplayChannel
> *display)
> }
> }
>
> -static bool free_one_drawable(DisplayChannel *display, int force_glz_free)
> +static bool free_one_drawable(DisplayChannel *display)
> {
> RingItem *ring_item = ring_get_tail(&display->current_list);
> Drawable *drawable;
> @@ -1178,9 +1178,6 @@ static bool free_one_drawable(DisplayChannel *display,
> int force_glz_free)
> }
>
> drawable = SPICE_CONTAINEROF(ring_item, Drawable, list_link);
> - if (force_glz_free) {
> - glz_retention_free_drawables(&drawable->glz_retention);
> - }
> drawable_draw(display, drawable);
> container = drawable->tree_item.base.container;
>
> @@ -1192,33 +1189,35 @@ static bool free_one_drawable(DisplayChannel *display,
> int force_glz_free)
> void display_channel_current_flush(DisplayChannel *display, int surface_id)
> {
> while (!ring_is_empty(&display->surfaces[surface_id].current_list)) {
> - free_one_drawable(display, FALSE);
> + free_one_drawable(display);
> }
> current_remove_all(display, surface_id);
> }
>
> void display_channel_free_some(DisplayChannel *display)
> {
> - int n = 0;
> + int left_to_free = RED_RELEASE_BUNCH_SIZE;
> DisplayChannelClient *dcc;
> GList *link, *next;
>
> - spice_debug("#draw=%d, #glz_draw=%d", display->drawable_count,
> - display->encoder_shared_data.glz_drawable_count);
> + spice_debug("#draw=%d", display->drawable_count);
> FOREACH_CLIENT(display, link, next, dcc) {
> - // encoding using the dictionary is prevented since the following
> operations might
> - // change the dictionary
> - if (image_encoders_glz_encode_lock(&dcc->encoders)) {
> - n = image_encoders_free_some_independent_glz_drawables(&dcc-
> >encoders);
> - }
> + left_to_free =
> image_encoders_free_some_independent_glz_drawables(&dcc->encoders,
> left_to_free);
At first glance It looked like you were over-writing left_to_free here without
using it. Now I see that this function modifies and returns the left_to_free
argument. But I don't really like that. See comment below about this function
signature.
> }
>
> - while (!ring_is_empty(&display->current_list) && n++ <
> RED_RELEASE_BUNCH_SIZE) {
> - free_one_drawable(display, TRUE);
> - }
> + if (left_to_free > 0) {
> + int saved_to_free = left_to_free;
>
> - FOREACH_CLIENT(display, link, next, dcc) {
> - image_encoders_glz_encode_unlock(&dcc->encoders);
> + while (left_to_free > 0 && !ring_is_empty(&display->current_list)) {
> + free_one_drawable(display);
> + --left_to_free;
> + }
> +
> + /* do it again to free drawables added by previous loop */
"added"? the previous loop freed drawables, not added them. right?
> + left_to_free = saved_to_free;
> + FOREACH_CLIENT(display, link, next, dcc) {
> + left_to_free =
> image_encoders_free_some_independent_glz_drawables(&dcc->encoders,
> left_to_free);
> + }
I don't understand the why you restored the saved_to_free here. What is the
purpose exactly?
> }
> }
>
> @@ -1263,7 +1262,7 @@ static Drawable
> *display_channel_drawable_try_new(DisplayChannel *display,
> Drawable *drawable;
>
> while (!(drawable = drawable_try_new(display))) {
> - if (!free_one_drawable(display, FALSE))
> + if (!free_one_drawable(display))
> return NULL;
> }
>
> @@ -1341,7 +1340,7 @@ void drawable_unref(Drawable *drawable)
> drawable_unref_surface_deps(display, drawable);
> display_channel_surface_unref(display, drawable->surface_id);
>
> - glz_retention_detach_drawables(&drawable->glz_retention);
> + glz_retention_free(&drawable->glz_retention);
>
> if (drawable->red_drawable) {
> red_drawable_unref(drawable->red_drawable);
> @@ -1845,9 +1844,8 @@ static void on_disconnect(RedChannelClient *rcc)
> display_channel_compress_stats_print(display);
>
> // this was the last channel client
> - spice_debug("#draw=%d, #glz_draw=%d",
> - display->drawable_count,
> - display->encoder_shared_data.glz_drawable_count);
> + spice_debug("#draw=%d",
> + display->drawable_count);
> }
>
> static int handle_migrate_flush_mark(RedChannelClient *rcc)
> diff --git a/server/image-encoders.c b/server/image-encoders.c
> index 5759230..23c3326 100644
> --- a/server/image-encoders.c
> +++ b/server/image-encoders.c
> @@ -30,11 +30,6 @@
>
> #define ENCODER_MESSAGE_SIZE 512
>
> -#define MAX_GLZ_DRAWABLE_INSTANCES 2
> -
> -typedef struct RedGlzDrawable RedGlzDrawable;
> -typedef struct GlzDrawableInstanceItem GlzDrawableInstanceItem;
> -
> struct GlzSharedDictionary {
> RingItem base;
> GlzEncDictContext *dict;
> @@ -45,37 +40,28 @@ struct GlzSharedDictionary {
> RedClient *client; // channel clients of the same client share the dict
> };
>
> -/* for each qxl drawable, there may be several instances of lz drawables */
> -/* TODO - reuse this stuff for the top level. I just added a second level of
> multiplicity
> - * at the Drawable by keeping a ring, so:
> - * Drawable -> (ring of) RedGlzDrawable -> (up to 2) GlzDrawableInstanceItem
> - * and it should probably (but need to be sure...) be
> - * Drawable -> ring of GlzDrawableInstanceItem.
> +/**
> + * This structure is used to retain RedDrawable objects so they don't
> + * disappear while we are using their memory.
> + * This as Glz will keep using image memory for future compressions.
"This as ..." is a bit unclear. Do you mean to say "This is because ..."?
> */
> -struct GlzDrawableInstanceItem {
> - RingItem glz_link;
> +typedef struct GlzContext {
> + /** linked to ImageEncoders::glz_drawables. Always inserted */
> + RingItem glz_context_link;
> + /** linked to GlzImageRetention::ring or ImageEncoders::orphans. Always
> inserted */
> + RingItem retention_link;
> + /** linked to ImageEncoders::glz_drawables_inst_to_free */
> RingItem free_link;
> + /** context pointer from Glz encoder, used to free the drawable */
> GlzEncDictImageContext *context;
Now that this type is called GlzContext, having a 'context' data member is
potentially a bit confusing. Rename to dict_context, or similar?
> - RedGlzDrawable *glz_drawable;
> -};
> -
> -struct RedGlzDrawable {
> - RingItem link; // ordered by the time it was encoded
> - RingItem drawable_link;
> + /** where the drawable came from */
> + ImageEncoders *enc;
> + /** hold a reference to this object, this to make sure Glz
> + * can continue to use its memory */
> RedDrawable *red_drawable;
> - GlzDrawableInstanceItem instances_pool[MAX_GLZ_DRAWABLE_INSTANCES];
> - Ring instances;
> - uint8_t instances_count;
> - gboolean has_drawable;
> - ImageEncoders *encoders;
> -};
> -
> -#define LINK_TO_GLZ(ptr) SPICE_CONTAINEROF((ptr), RedGlzDrawable, \
> - drawable_link)
> -#define DRAWABLE_FOREACH_GLZ_SAFE(drawable, link, next, glz) \
> - SAFE_FOREACH(link, next, drawable, &(drawable)->glz_retention.ring, glz,
> LINK_TO_GLZ(link))
> +} GlzContext;
>
> -static void glz_drawable_instance_item_free(GlzDrawableInstanceItem
> *instance);
> +static gboolean glz_context_free(GlzContext *glz_context);
> static void encoder_data_init(EncoderData *data);
> static void encoder_data_reset(EncoderData *data);
> static void image_encoders_release_glz(ImageEncoders *enc);
> @@ -382,11 +368,12 @@ static void image_encoders_init_lz(ImageEncoders *enc)
> static void glz_usr_free_image(GlzEncoderUsrContext *usr, GlzUsrImageContext
> *image)
> {
> GlzData *lz_data = (GlzData *)usr;
> - GlzDrawableInstanceItem *glz_drawable_instance = (GlzDrawableInstanceItem
> *)image;
> - ImageEncoders *drawable_enc = glz_drawable_instance->glz_drawable-
> >encoders;
> + GlzContext *glz_context = (GlzContext *)image;
> + ImageEncoders *drawable_enc = glz_context->enc;
> ImageEncoders *this_enc = SPICE_CONTAINEROF(lz_data, ImageEncoders,
> glz_data);
> + glz_context->context = NULL;
> if (this_enc == drawable_enc) {
> - glz_drawable_instance_item_free(glz_drawable_instance);
> + glz_context_free(glz_context);
> } else {
> /* The glz dictionary is shared between all DisplayChannelClient
> * instances that belong to the same client, and glz_usr_free_image
> @@ -396,7 +383,7 @@ static void glz_usr_free_image(GlzEncoderUsrContext *usr,
> GlzUsrImageContext *im
> * this check.
> */
> pthread_mutex_lock(&drawable_enc->glz_drawables_inst_to_free_lock);
> - ring_add_before(&glz_drawable_instance->free_link,
> + ring_add_before(&glz_context->free_link,
> &drawable_enc->glz_drawables_inst_to_free);
> pthread_mutex_unlock(&drawable_enc->glz_drawables_inst_to_free_lock);
> }
> @@ -457,6 +444,7 @@ void image_encoders_init(ImageEncoders *enc,
> ImageEncoderSharedData *shared_data
> spice_assert(shared_data);
> enc->shared_data = shared_data;
>
> + ring_init(&enc->glz_orphans);
> ring_init(&enc->glz_drawables);
> ring_init(&enc->glz_drawables_inst_to_free);
> pthread_mutex_init(&enc->glz_drawables_inst_to_free_lock, NULL);
> @@ -493,118 +481,60 @@ void image_encoders_free(ImageEncoders *enc)
> }
>
> /* Remove from the to_free list and the instances_list.
> - When no instance is left - the RedGlzDrawable is released too. (and the
> qxl drawable too, if
> - it is not used by Drawable).
> + Returns FALSE if a RedDrawable was freed.
I think I would expect that a _free() function would return TRUE if something
was fully freed, rather than FALSE.
> NOTE - 1) can be called only by the display channel that created the
> drawable
> 2) it is assumed that the instance was already removed from the
> dictionary*/
> -static void glz_drawable_instance_item_free(GlzDrawableInstanceItem
> *instance)
> +static gboolean glz_context_free(GlzContext *glz_context)
> {
> - RedGlzDrawable *glz_drawable;
> -
> - spice_assert(instance);
> - spice_assert(instance->glz_drawable);
> + gboolean ret;
>
> - glz_drawable = instance->glz_drawable;
> + spice_assert(glz_context);
>
> - spice_assert(glz_drawable->instances_count > 0);
> -
> - ring_remove(&instance->glz_link);
> - glz_drawable->instances_count--;
> + ring_remove(&glz_context->glz_context_link);
> + ring_remove(&glz_context->retention_link);
>
> // when the remove callback is performed from the channel that the
> // drawable belongs to, the instance is not added to the 'to_free' list
> - if (ring_item_is_linked(&instance->free_link)) {
> - ring_remove(&instance->free_link);
> - }
> -
> - if (ring_is_empty(&glz_drawable->instances)) {
> - spice_assert(glz_drawable->instances_count == 0);
> -
> - if (glz_drawable->has_drawable) {
> - ring_remove(&glz_drawable->drawable_link);
> - }
> - red_drawable_unref(glz_drawable->red_drawable);
> - glz_drawable->encoders->shared_data->glz_drawable_count--;
> - if (ring_item_is_linked(&glz_drawable->link)) {
> - ring_remove(&glz_drawable->link);
> - }
> - free(glz_drawable);
> - }
> -}
> -
> -/*
> - * Releases all the instances of the drawable from the dictionary and the
> display channel client.
> - * The release of the last instance will also release the drawable itself and
> the qxl drawable
> - * if possible.
> - * NOTE - the caller should prevent encoding using the dictionary during this
> operation
> - */
> -static void red_glz_drawable_free(RedGlzDrawable *glz_drawable)
> -{
> - ImageEncoders *enc = glz_drawable->encoders;
> - RingItem *head_instance = ring_get_head(&glz_drawable->instances);
> - int cont = (head_instance != NULL);
> -
> - while (cont) {
> - if (glz_drawable->instances_count == 1) {
> - /* Last instance: glz_drawable_instance_item_free will free the
> glz_drawable */
> - cont = FALSE;
> - }
> - GlzDrawableInstanceItem *instance = SPICE_CONTAINEROF(head_instance,
> - GlzDrawableInstanceIt
> em,
> - glz_link);
> - if (!ring_item_is_linked(&instance->free_link)) {
> - // the instance didn't get out from window yet
> - glz_enc_dictionary_remove_image(enc->glz_dict->dict,
> - instance->context,
> - &enc->glz_data.usr);
> - }
> - glz_drawable_instance_item_free(instance);
> -
> - if (cont) {
> - head_instance = ring_get_head(&glz_drawable->instances);
> - }
> + if (ring_item_is_linked(&glz_context->free_link)) {
> + ring_remove(&glz_context->free_link);
> + } else if (glz_context->context) {
I don't understand why this is an "else if"
> + ImageEncoders *enc = glz_context->enc;
> +
> + // the instance didn't get out from window yet
> + glz_enc_dictionary_remove_image(enc->glz_dict->dict,
> + glz_context->context,
> + &enc->glz_data.usr);
> }
> -}
>
> -gboolean image_encoders_glz_encode_lock(ImageEncoders *enc)
> -{
> - if (enc->glz_dict) {
> - pthread_rwlock_wrlock(&enc->glz_dict->encode_lock);
> - return TRUE;
> - }
> - return FALSE;
> -}
> + ret = red_drawable_unref(glz_context->red_drawable);
> + free(glz_context);
>
> -void image_encoders_glz_encode_unlock(ImageEncoders *enc)
> -{
> - if (enc->glz_dict) {
> - pthread_rwlock_unlock(&enc->glz_dict->encode_lock);
> - }
> + return ret;
> }
>
> /*
> * Remove from the global lz dictionary some glz_drawables that have no
> reference to
> * Drawable (their qxl drawables are released too).
> - * NOTE - the caller should prevent encoding using the dictionary during the
> operation
> */
Comment should also describe the return value so that you don't have to examine
implementation to know how to interpet it. But See below.
> -int image_encoders_free_some_independent_glz_drawables(ImageEncoders *enc)
> +int image_encoders_free_some_independent_glz_drawables(ImageEncoders *enc,
> int left_to_free)
I personally feel that this interface is a bit awkward to use, as alluded to
above. Previously, it returned the number of drawables that were actually freed.
Now it returns the number of drawables that still need to be freed. I think it
would be much more self-contained and generic if it kept returning the number of
items that were freed. We could then rename the "left_to_free" argument to
something like "max_to_free". Then usage would be something more like:
left_to_free -= image_encoders_free_some_independent_glz_drawables(enc,
left_to_free)
I prefer that to the usage above where you simply assign the return value to
left_to_free.
> {
> RingItem *ring_link;
> - int n = 0;
>
> - if (!enc) {
> - return 0;
> + if (!enc || !enc->glz_dict || left_to_free <= 0) {
> + return left_to_free;
> }
> - ring_link = ring_get_head(&enc->glz_drawables);
> - while ((n < RED_RELEASE_BUNCH_SIZE) && (ring_link != NULL)) {
> - RedGlzDrawable *glz_drawable = SPICE_CONTAINEROF(ring_link,
> RedGlzDrawable, link);
> - ring_link = ring_next(&enc->glz_drawables, ring_link);
> - if (!glz_drawable->has_drawable) {
> - red_glz_drawable_free(glz_drawable);
> - n++;
> +
> + // prevent encoding using the dictionary during the operation
> + pthread_rwlock_wrlock(&enc->glz_dict->encode_lock);
> + while (left_to_free > 0
> + && (ring_link = ring_get_head(&enc->glz_orphans)) != NULL) {
> + GlzContext *glz_context = SPICE_CONTAINEROF(ring_link, GlzContext,
> retention_link);
> + if (!glz_context_free(glz_context)) {
> + --left_to_free;
> }
> }
> - return n;
> + pthread_rwlock_unlock(&enc->glz_dict->encode_lock);
> + return left_to_free;
> }
>
> void image_encoders_free_glz_drawables_to_free(ImageEncoders* enc)
> @@ -616,10 +546,8 @@ void
> image_encoders_free_glz_drawables_to_free(ImageEncoders* enc)
> }
> pthread_mutex_lock(&enc->glz_drawables_inst_to_free_lock);
> while ((ring_link = ring_get_head(&enc->glz_drawables_inst_to_free))) {
> - GlzDrawableInstanceItem *drawable_instance =
> SPICE_CONTAINEROF(ring_link,
> - GlzDrawableI
> nstanceItem,
> - free_link);
> - glz_drawable_instance_item_free(drawable_instance);
> + GlzContext * glz_context = SPICE_CONTAINEROF(ring_link, GlzContext,
> free_link);
> + glz_context_free(glz_context);
> }
> pthread_mutex_unlock(&enc->glz_drawables_inst_to_free_lock);
> }
> @@ -638,30 +566,24 @@ void image_encoders_free_glz_drawables(ImageEncoders
> *enc)
> // assure no display channel is during global lz encoding
> pthread_rwlock_wrlock(&glz_dict->encode_lock);
> while ((ring_link = ring_get_head(&enc->glz_drawables))) {
> - RedGlzDrawable *drawable = SPICE_CONTAINEROF(ring_link,
> RedGlzDrawable, link);
> + GlzContext *glz_context = SPICE_CONTAINEROF(ring_link, GlzContext,
> glz_context_link);
> // no need to lock the to_free list, since we assured no other thread
> is encoding and
> // thus not other thread access the to_free list of the channel
> - red_glz_drawable_free(drawable);
> + glz_context_free(glz_context);
> }
> pthread_rwlock_unlock(&glz_dict->encode_lock);
> }
>
> -void glz_retention_free_drawables(GlzImageRetention *ret)
> +void glz_retention_free(GlzImageRetention *ret)
The function name gives the impression that it is actually freeing the memory
associated with 'ret', but it's not. It's simply clearing all of the images in
the ring. So I think a name such as glz_retention_clear() might be more
appropriate.
Also, 'ret' is often used as short-hand terminology for a variable that stores a
return value. So I'd prefer to use the full name here to make it a bit more
readable: "retention"
> {
> - RingItem *glz_item, *next_item;
> - RedGlzDrawable *glz;
> - SAFE_FOREACH(glz_item, next_item, TRUE, &ret->ring, glz,
> LINK_TO_GLZ(glz_item)) {
> - red_glz_drawable_free(glz);
> - }
> -}
> + RingItem *head;
>
> -void glz_retention_detach_drawables(GlzImageRetention *ret)
> -{
> - RingItem *item, *next;
> + /* append all ring to orphans one */
> + while ((head = ring_get_head(&ret->ring)) != NULL) {
> + GlzContext *glz_context = SPICE_CONTAINEROF(head, GlzContext,
> retention_link);
>
> - RING_FOREACH_SAFE(item, next, &ret->ring) {
> - SPICE_CONTAINEROF(item, RedGlzDrawable, drawable_link)->has_drawable
> = FALSE;
> - ring_remove(item);
> + ring_remove(head);
> + ring_add_before(head, &glz_context->enc->glz_orphans);
> }
> }
>
> @@ -1156,52 +1078,22 @@ int image_encoders_compress_lz4(ImageEncoders *enc,
> SpiceImage *dest,
>
> /* if already exists, returns it. Otherwise allocates and adds it (1) to the
> ring tail
> in the channel (2) to the Drawable*/
> -static RedGlzDrawable *get_glz_drawable(ImageEncoders *enc, RedDrawable
> *red_drawable,
> - GlzImageRetention *glz_retention)
> +static GlzContext *get_glz_context(ImageEncoders *enc, RedDrawable
> *red_drawable,
> + GlzImageRetention *glz_retention)
> {
> - RedGlzDrawable *ret;
> - RingItem *item, *next;
> -
> - // TODO - I don't really understand what's going on here, so doing the
> technical equivalent
> - // now that we have multiple glz_dicts, so the only way to go from dcc to
> drawable glz is to go
> - // over the glz_ring (unless adding some better data structure then a
> ring)
> - SAFE_FOREACH(item, next, TRUE, &glz_retention->ring, ret,
> LINK_TO_GLZ(item)) {
> - if (ret->encoders == enc) {
> - return ret;
> - }
> - }
> -
> - ret = spice_new(RedGlzDrawable, 1);
> -
> - ret->encoders = enc;
> - ret->red_drawable = red_drawable_ref(red_drawable);
> - ret->has_drawable = TRUE;
> - ret->instances_count = 0;
> - ring_init(&ret->instances);
> -
> - ring_item_init(&ret->link);
> - ring_item_init(&ret->drawable_link);
> - ring_add_before(&ret->link, &enc->glz_drawables);
> - ring_add(&glz_retention->ring, &ret->drawable_link);
> - enc->shared_data->glz_drawable_count++;
> - return ret;
> -}
> + GlzContext *ret;
>
> -/* allocates new instance and adds it to instances in the given drawable.
> - NOTE - the caller should set the glz_instance returned by the encoder by
> itself.*/
> -static GlzDrawableInstanceItem *add_glz_drawable_instance(RedGlzDrawable
> *glz_drawable)
> -{
> - spice_assert(glz_drawable->instances_count < MAX_GLZ_DRAWABLE_INSTANCES);
> - // NOTE: We assume the additions are performed consecutively, without
> removals in the middle
> - GlzDrawableInstanceItem *ret = glz_drawable->instances_pool +
> glz_drawable->instances_count;
> - glz_drawable->instances_count++;
> + ret = spice_new(GlzContext, 1);
>
> + ring_item_init(&ret->glz_context_link);
> + ring_item_init(&ret->retention_link);
> ring_item_init(&ret->free_link);
> - ring_item_init(&ret->glz_link);
> - ring_add(&glz_drawable->instances, &ret->glz_link);
> - ret->context = NULL;
> - ret->glz_drawable = glz_drawable;
>
> + ret->enc = enc;
> + ret->red_drawable = red_drawable_ref(red_drawable);
> +
> + ring_add_before(&ret->glz_context_link, &enc->glz_drawables);
> + ring_add(&glz_retention->ring, &ret->retention_link);
> return ret;
> }
>
> @@ -1220,8 +1112,7 @@ int image_encoders_compress_glz(ImageEncoders *enc,
> GlzData *glz_data = &enc->glz_data;
> ZlibData *zlib_data;
> LzImageType type = bitmap_fmt_to_lz_image_type[src->format];
> - RedGlzDrawable *glz_drawable;
> - GlzDrawableInstanceItem *glz_drawable_instance;
> + GlzContext *glz_context;
> int glz_size;
> int zlib_size;
>
> @@ -1242,8 +1133,7 @@ int image_encoders_compress_glz(ImageEncoders *enc,
>
> encoder_data_init(&glz_data->data);
>
> - glz_drawable = get_glz_drawable(enc, red_drawable, glz_retention);
> - glz_drawable_instance = add_glz_drawable_instance(glz_drawable);
> + glz_context = get_glz_context(enc, red_drawable, glz_retention);
>
> glz_data->data.u.lines_data.chunks = src->data;
> glz_data->data.u.lines_data.stride = src->stride;
> @@ -1254,8 +1144,8 @@ int image_encoders_compress_glz(ImageEncoders *enc,
> (src->flags & SPICE_BITMAP_FLAGS_TOP_DOWN), NULL,
> 0,
> src->stride, glz_data->data.bufs_head->buf.bytes,
> sizeof(glz_data->data.bufs_head->buf),
> - glz_drawable_instance,
> - &glz_drawable_instance->context);
> + glz_context,
> + &glz_context->context);
>
> stat_compress_add(&enc->shared_data->glz_stat, start_time, src->stride *
> src->y, glz_size);
>
> diff --git a/server/image-encoders.h b/server/image-encoders.h
> index 9b34f92..b10868a 100644
> --- a/server/image-encoders.h
> +++ b/server/image-encoders.h
> @@ -45,16 +45,18 @@ void image_encoder_shared_stat_print(const
> ImageEncoderSharedData *shared_data);
>
> void image_encoders_init(ImageEncoders *enc, ImageEncoderSharedData
> *shared_data);
> void image_encoders_free(ImageEncoders *enc);
> -int image_encoders_free_some_independent_glz_drawables(ImageEncoders *enc);
> +int image_encoders_free_some_independent_glz_drawables(ImageEncoders *enc,
> int left_to_free);
> void image_encoders_free_glz_drawables(ImageEncoders *enc);
> void image_encoders_free_glz_drawables_to_free(ImageEncoders* enc);
> gboolean image_encoders_glz_create(ImageEncoders *enc, uint8_t id);
> void image_encoders_glz_get_restore_data(ImageEncoders *enc,
> uint8_t *out_id,
> GlzEncDictRestoreData *out_data);
> -gboolean image_encoders_glz_encode_lock(ImageEncoders *enc);
> -void image_encoders_glz_encode_unlock(ImageEncoders *enc);
> -void glz_retention_free_drawables(GlzImageRetention *ret);
> -void glz_retention_detach_drawables(GlzImageRetention *ret);
> +
> +/**
> + * Free retention structure so Glz code that drawables
> + * are no more owned by something else.
> + */
> +void glz_retention_free(GlzImageRetention *ret);
>
> #define RED_COMPRESS_BUF_SIZE (1024 * 64)
> struct RedCompressBuf {
> @@ -142,8 +144,6 @@ static inline void glz_retention_init(GlzImageRetention
> *ret)
> }
>
> struct ImageEncoderSharedData {
> - uint32_t glz_drawable_count;
> -
> stat_info_t off_stat;
> stat_info_t lz_stat;
> stat_info_t glz_stat;
> @@ -183,6 +183,9 @@ struct ImageEncoders {
> GlzEncoderContext *glz;
> GlzData glz_data;
>
> + /** list to all Glz contexts that are owned only by Glz and could
> + * be freed in case of OOM */
> + Ring glz_orphans;
> Ring glz_drawables; // all the living lz drawable, ordered
> by encoding time
If I understand correctly, this Ring now holds GlzContext, so glz_drawables is
not really an appropriate name anymore and probably introduces confusion.
> Ring glz_drawables_inst_to_free; // list of instances to be
> freed
Same: glz_drawables_inst refers to GlzDrawableInstanceItem, which no longer
exists. So it should probably be renamed.
> pthread_mutex_t glz_drawables_inst_to_free_lock;
same with this variable
> diff --git a/server/red-worker.c b/server/red-worker.c
> index 9238632..0579dec 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -123,14 +123,15 @@ static void common_release_recv_buf(RedChannelClient
> *rcc, uint16_t type, uint32
> }
> }
>
> -void red_drawable_unref(RedDrawable *red_drawable)
> +gboolean red_drawable_unref(RedDrawable *red_drawable)
> {
> if (--red_drawable->refs) {
> - return;
> + return TRUE;
> }
> red_qxl_release_resource(red_drawable->qxl, red_drawable-
> >release_info_ext);
> red_put_drawable(red_drawable);
> free(red_drawable);
> + return FALSE;
> }
>
> static int red_process_cursor(RedWorker *worker, int *ring_is_empty)
> @@ -821,9 +822,8 @@ static void handle_dev_oom(void *opaque, void *payload)
>
> spice_return_if_fail(worker->running);
> // streams? but without streams also leak
> - spice_debug("OOM1 #draw=%u, #glz_draw=%u current %u pipes %u",
> + spice_debug("OOM1 #draw=%u, current %u pipes %u",
> display->drawable_count,
> - display->encoder_shared_data.glz_drawable_count,
> display->current_size,
> red_channel_sum_pipes_size(display_red_channel));
> while (red_process_display(worker, &ring_is_empty)) {
> @@ -833,9 +833,8 @@ static void handle_dev_oom(void *opaque, void *payload)
> display_channel_free_some(worker->display_channel);
> red_qxl_flush_resources(worker->qxl);
> }
> - spice_debug("OOM2 #draw=%u, #glz_draw=%u current %u pipes %u",
> + spice_debug("OOM2 #draw=%u, current %u pipes %u",
> display->drawable_count,
> - display->encoder_shared_data.glz_drawable_count,
> display->current_size,
> red_channel_sum_pipes_size(display_red_channel));
> red_qxl_clear_pending(worker->qxl->st, RED_DISPATCHER_PENDING_OOM);
> diff --git a/server/red-worker.h b/server/red-worker.h
> index 63be8b5..1639df8 100644
> --- a/server/red-worker.h
> +++ b/server/red-worker.h
> @@ -92,7 +92,7 @@ RedWorker* red_worker_new(QXLInstance *qxl,
> const ClientCbs *client_display_cbs);
> bool red_worker_run(RedWorker *worker);
>
> -void red_drawable_unref(RedDrawable *red_drawable);
> +gboolean red_drawable_unref(RedDrawable *red_drawable);
>
> CommonGraphicsChannel *red_worker_new_channel(RedWorker *worker, int size,
> const char *name,
I must say, this was quite difficult to review since it was a pretty significant
refactoring. The resulting code looks better and is easier to understand than
the original, but I'm not entirely sure that I've convinced myself that the
behavior is exactly the same. In particular the many-to-one relationships
between GlzDrawableInstanceItem / RedGlzDrawable that were collapsed into a
single GlzContext type are a bit challenging to wrap my head around...
Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>
More information about the Spice-devel
mailing list