>
> 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"
Yes, one should be glz_retention_detach_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.
>
Yes, could be confusing, was inspired as some unref returns the pointer or
NULL. But for boolean you are right.
> > - 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@xxxxxxxxxx>
> > ---
> > 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);
I would add a comment before the next loop:
"As we are trying to release some memory as requested by guest driver
loop througth Glz freeing drawables which have no corresponding Drawable
as this will cause guest memory to be freed."
> > 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.
>
Agreed with signature change.
>
> > }
> >
> > - 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?
>
mumble:
"We freed Drawables in the loop above but the underlaying RedDrawables
could still be referenced by Glz so scan again the Glz for independent
drawables. Note that there is a 1-to-1 relatioship between Drawable
and RedDrawable so in theory there will be a maximum of saved_to_free
RedDrawables to free. Use this limit in any case, this should not be
a problem and will make sure we won't free more items that we are
supposed to free."
> > + 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?
>
Let's say that we should free 10 RedDrawables (RED_RELEASE_BUNCH_SIZE) and
that freeing the first independent we freed 6 of them so we still should free
4. We free 4 Drawables but then the associated RedDrawables are still holded
by Glz, we still need to free these 4 so we restore the counter.
> > }
> > }
> >
> > @@ -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 ..."?
>
Yes. To be more detailed Glz dictionary will use the memory pointed by
SpiceImage but we use RedDrawable to retain the pointer as SpiceImage
has no reference counting.
There is actually similar code in the streaming code.
> > */
> > -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?
>
Actually I think that GlzContext is confusing and would be better to rename.
First: context is too general. dict_context for that field is fine but
dict_image_context would be even better. I think at this structure as a
counterpart of another in Glz code. Like two face of a coins. Basically we
store a GlzEncDictImageContext pointer in our structure and on the other place
we tell Glz our GlzContext pointer. Glz code will call our code with our
"context" pointer (to handle cleanup) while we can use Glz pointer to force
image cleanup. I think a better name would be "GlzDictItem".
> > - 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"
>
There are 3 cases:
- context is in the free list which means that Glz code requested a
cleanup but didn't manage to clean directly as was running in another
thread;
- there is not a context (NULL) as Glz code requested a cleanup and
is cleaning up directly as running in the same thread;
- there is still a context (not NULL) as the image is still used
by Glz, we need to clean the image from Glz.
Probably these notes should be added to a code comment.
> > + 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.
>
I think "destroy" would even better as this suggests that structure should
not be used anymore
> 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"
>
Yes.
> > {
> > - 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.
>
As I suggestd GlzDictItem could be glz_dict_items even better?
> > 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.
>
glz_dict_items_to_free.
> > pthread_mutex_t glz_drawables_inst_to_free_lock;
>
> same with this variable
>
glz_dict_items_to_free_lock.
> > 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@xxxxxxxxxx>
>
The behaviour is not exactly the same, actually it fixes something.
The original code for instance assumed that RedDrawables were references
by Glz and Drawable only. This is no more true as now could be referenced
even by streams. So the computation for OOM is now more accurate.
Also the old code assumed that with multiple clients all clients were
aligned that is had the same items on the queues. I don't know if this
is/was true but the current code avoid this assumption.
The old code used a GlzRedDrawable which combined with RedClient was
1-to-1 with RedDrawables. This could have been used to handle
differently GlzInstanceItems in case there was still occurrencies for
the same RedDrawable but this is not likely and also was not used.
There was a comment stating that this double layer was possibly too
complicate and suitale to be removed.
Not taking into account the ugly static allocation of GlzInstanceItem
(why the limit of 2? There were no explanation but just some checks
with some asserts if this was not the case and many possible corner
cases in the code on the allocation order in this static 2 item
pool).
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel