>
> Hey,
>
> On Tue, Oct 18, 2016 at 10:28:25AM +0100, Frediano Ziglio wrote:
> > The code was quite complicated.
>
> I tried to look at this one, and it indeed is quite complicated... So
> far, too much entangled stuff that I feel confident I'm going to make a
> meaningful review...
>
> > - 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_detach_drawables.
> >
> > Now:
> > - a GlzEncDictImageContext is bound to a GlzDictItem (still 1-1 relation);
> > - multiple GlzDictItem are attached to a Drawable using GlzImageRetention;
> > - there is only a glz_retention_free to be called when the structure
> > must be destroyed.
>
> By reading/parsing that, executive summary is that
> GlzDrawableInstanceItem/RedGlzDrawable have been merged and everything
> has been renamed.
>
I think this change more than that as changing structures not a kind of
merge/rename.
If you join wheat and eggs to do a cake you don't call the cake wheat-egg.
That is it make no sense to have a rename patch.
When I wrote this patch I draw a diagram of the current situation
than try to check the relationship like into a database then
I wrote a new schema (which really liked more a entity relationship
model than a structure model) and came to this code. To debug and
avoid possible leaks beside the 2/2 patch I have another patch with
some printf and counters.
> >
> > Implementation detail:
> > - red_drawable_unref returns a gboolean to indicate if the object
> > was really freed;
>
> Ok, does it have to be done in this patch rather than before?
>
Yes, probably this can be split.
> > - glz_drawable_count was removed as there are no more RedGlzDrawable;
>
> And this can't be a follow-up?
>
Obviously you cannot count something that is not present anymore.
The point is why these items were counted? Was as the entire structures
were complicated and just for debugging?
I would instead in this case having cleanup code to make sure that
everything is freed.
Was to check for resource allocated? In this case perhaps Glz code
could have a counter instead.
> > - image_encoders_glz_encode_lock and image_encoders_glz_encode_unlock
> > were removed and now the locking is handled entirely by ImageEncoders;
>
> Ok, might be doable after that merging/renaming?
>
I don't get this. Do you means split it?
> > - instead of marking GlzDictItem/GlzDrawableInstanceItem changing
> > has_drawable flag contexts are moved to a glz_orphans ring;
>
> And this change could not be done in a preparatory patch?
>
Maybe, I'll try.
> > - 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 | 57 +++++----
> > server/image-encoders.c | 300
> > +++++++++++++++--------------------------------
> > server/image-encoders.h | 27 +++--
> > server/red-worker.c | 5 +-
> > server/red-worker.h | 2 +-
> > 5 files changed, 145 insertions(+), 246 deletions(-)
> >
> > diff --git a/server/display-channel.c b/server/display-channel.c
> > index 69edd35..9a56080 100644
> > --- a/server/display-channel.c
> > +++ b/server/display-channel.c
> > @@ -1185,7 +1185,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->priv->current_list);
> > Drawable *drawable;
> > @@ -1196,9 +1196,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;
> >
> > @@ -1210,37 +1207,47 @@ 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->priv->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;
> > GListIter iter;
> >
> > - spice_debug("#draw=%d, #glz_draw=%d", display->priv->drawable_count,
> > - display->priv->encoder_shared_data.glz_drawable_count);
> > + /* 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. */
> > + spice_debug("#draw=%d", display->priv->drawable_count);
> > FOREACH_DCC(display, iter, dcc) {
> > ImageEncoders *encoders = dcc_get_encoders(dcc);
> >
> > - // encoding using the dictionary is prevented since the following
> > operations might
> > - // change the dictionary
> > - if (image_encoders_glz_encode_lock(encoders)) {
> > - n =
> > image_encoders_free_some_independent_glz_drawables(encoders);
> > - }
> > + left_to_free -=
> > image_encoders_free_some_independent_glz_drawables(encoders,
> > left_to_free);
> > }
> >
> > - while (!ring_is_empty(&display->priv->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_DCC(display, iter, dcc) {
> > - ImageEncoders *encoders = dcc_get_encoders(dcc);
> > + while (left_to_free > 0 &&
> > !ring_is_empty(&display->priv->current_list)) {
> > + free_one_drawable(display);
> > + --left_to_free;
> > + }
> >
> > - image_encoders_glz_encode_unlock(encoders);
> > + /* 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_DCC(display, iter, dcc) {
> > + ImageEncoders *encoders = dcc_get_encoders(dcc);
> > + left_to_free -=
> > image_encoders_free_some_independent_glz_drawables(encoders,
> > left_to_free);
> > + }
> > }
> > }
> >
> > @@ -1285,7 +1292,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;
> > }
> >
> > @@ -1362,7 +1369,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_destroy(&drawable->glz_retention);
> >
> > if (drawable->red_drawable) {
> > red_drawable_unref(drawable->red_drawable);
>
> A lot of the changes up to now seems to be about removing the second
> argument to free_one_drawable(), and renaming "n" to "left_to_free", and
> passing RED_RELEASE_BUNCH_SIZE as a parameter to
> image_encoders_free_some_independent_glz_drawables() rather than
> hardcoding it in that function. This feels not directly related to the
> type simplification you are trying to do, is it?
>
>
It is. These parameter can be removed as the structures are changed.
The change of this part are more complicated that seems as a first
sight. For instance the old counting was just broken.
You ask to delete X drawables but really counting these drawables
was not done correctly. I think one of the reason for the spice_debug
is that the code was so complicated and unclear that who wrote didn't
notice this issue but at least wanted to make sure something was freed.
> > @@ -1866,9 +1873,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->priv->drawable_count,
> > - display->priv->encoder_shared_data.glz_drawable_count);
> > + spice_debug("#draw=%d",
> > + display->priv->drawable_count);
> > }
> >
> > static int handle_migrate_flush_mark(RedChannelClient *rcc)
> > @@ -2117,10 +2123,9 @@ void display_channel_debug_oom(DisplayChannel
> > *display, const char *msg)
> > {
> > RedChannel *channel = RED_CHANNEL(display);
> >
> > - spice_debug("%s #draw=%u, #glz_draw=%u current %u pipes %u",
> > + spice_debug("%s #draw=%u, current %u pipes %u",
> > msg,
> > display->priv->drawable_count,
> > - display->priv->encoder_shared_data.glz_drawable_count,
> > display->priv->current_size,
> > red_channel_sum_pipes_size(channel));
> > }
>
> Ideally this would go in a follow-up commit, no idea if that's possible.
>
yes... see above... maybe another counter related to glz.
Never had to use these messages but as stated above now the code is
much more deterministic.
> > diff --git a/server/image-encoders.c b/server/image-encoders.c
> > index 39aca6c..951fe96 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 {
> > GlzEncDictContext *dict;
> > uint32_t refs;
> > @@ -44,37 +39,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 is because Glz will keep using image memory for future
> > compressions.
> > */
> > -struct GlzDrawableInstanceItem {
> > - RingItem glz_link;
> > +typedef struct GlzDictItem {
> > + /** linked to ImageEncoders::glz_dict_items. Always inserted */
> > + RingItem glz_context_link;
> > + /** linked to GlzImageRetention::ring or ImageEncoders::orphans.
> > Always inserted */
> > + RingItem retention_link;
> > + /** linked to ImageEncoders::glz_dict_items_to_free */
> > RingItem free_link;
>
> For what it's worth, this is the kind of situations where GLists would
> make the code much easier to follow, no need to keep track that when
> "retention_link" is used, we are changing either GlzImageRetention::ring
> or ImageEncoders::orphans, when "glz_context_link" is used, then we are
> using ImageEncoders::glz_dict_items and so on. I'm really having a hard
> time keeping track of this.
>
Would require some memory/cpu analysis which are out of this patch.
Worth mentioning for instance that maybe would be better to allocate
the entire memory and do copies instead of reusing drawable memory,
still this is out of the scope.
>From some profiling most of the time is spent reading the hash table
as the hash table is big compared to CPU cache. And it's so big because
it have to point to drawables instead of using simple offsets.
Maybe having a single block of memory and offsets would make performance
better and I don't know the memory difference (hash would be smaller
but there would be additional memory for the data copy perhaps counter
balanced by less drawables).
> > - GlzEncDictImageContext *context;
> > - RedGlzDrawable *glz_drawable;
> > -};
> > -
> > -struct RedGlzDrawable {
> > - RingItem link; // ordered by the time it was encoded
> > - RingItem drawable_link;
> > + /** context pointer from Glz encoder, used to free the drawable */
> > + GlzEncDictImageContext *dict_image_context;
> > + /** 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))
> > +} GlzDictItem;
> >
> > -static void glz_drawable_instance_item_free(GlzDrawableInstanceItem
> > *instance);
> > +static gboolean glz_context_free(GlzDictItem *glz_item);
> > static void encoder_data_init(EncoderData *data);
> > static void encoder_data_reset(EncoderData *data);
> > static void image_encoders_release_glz(ImageEncoders *enc);
> > @@ -381,23 +367,24 @@ 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;
> > + GlzDictItem *glz_item = (GlzDictItem *)image;
> > + ImageEncoders *drawable_enc = glz_item->enc;
>
> Here we've removed one level of indirection, but at the same time
> glz_drawable_instance was renamed to glz_item, and the 'encoders' member
> to 'enc'. This really is not helping with the review, this is "renaming
> noise", but one has to be careful to figure this out, and to remember
> this through the rest of the changes in that part of the code.
>
glz_drawable_instance and glz_item are completely different, it's not
a rename.
I can rename enc to encoders if it's more clear.
On the other side most of ImageEncoders* are "enc" so "encoders" is even
less coherent.
> > ImageEncoders *this_enc = SPICE_CONTAINEROF(lz_data, ImageEncoders,
> > glz_data);
> > + glz_item->dict_image_context = NULL;
> > if (this_enc == drawable_enc) {
> > - glz_drawable_instance_item_free(glz_drawable_instance);
> > + glz_context_free(glz_item);
> > } else {
> > /* The glz dictionary is shared between all DisplayChannelClient
> > - * instances that belong to the same client, and
> > glz_usr_free_image
> > + * items that belong to the same client, and glz_usr_free_image
> > * can be called by the dictionary code
> > * (glz_dictionary_window_remove_head). Thus this function can be
> > * called from any DisplayChannelClient thread, hence the need for
> > * this check.
> > */
> > -
> > pthread_mutex_lock(&drawable_enc->glz_drawables_inst_to_free_lock);
> > - ring_add_before(&glz_drawable_instance->free_link,
> > - &drawable_enc->glz_drawables_inst_to_free);
> > -
> > pthread_mutex_unlock(&drawable_enc->glz_drawables_inst_to_free_lock);
> > + pthread_mutex_lock(&drawable_enc->glz_dict_items_to_free_lock);
> > + ring_add_before(&glz_item->free_link,
> > + &drawable_enc->glz_dict_items_to_free);
> > + pthread_mutex_unlock(&drawable_enc->glz_dict_items_to_free_lock);
> > }
> > }
> >
> > @@ -456,9 +443,10 @@ void image_encoders_init(ImageEncoders *enc,
> > ImageEncoderSharedData *shared_data
> > spice_assert(shared_data);
> > enc->shared_data = shared_data;
> >
> > - ring_init(&enc->glz_drawables);
> > - ring_init(&enc->glz_drawables_inst_to_free);
> > - pthread_mutex_init(&enc->glz_drawables_inst_to_free_lock, NULL);
> > + ring_init(&enc->glz_orphans);
> > + ring_init(&enc->glz_dict_items);
> > + ring_init(&enc->glz_dict_items_to_free);
> > + pthread_mutex_init(&enc->glz_dict_items_to_free_lock, NULL);
>
> More "renaming noise"...
>
> > /*
> > - * Remove from the global lz dictionary some glz_drawables that have no
> > reference to
> > + * Remove from the global lz dictionary some glz_dict_items that have no
> > reference to
> > * Drawable (their qxl drawables are released too).
> > - * NOTE - the caller should prevent encoding using the dictionary during
> > the operation
> > */
> > -int image_encoders_free_some_independent_glz_drawables(ImageEncoders *enc)
> > +int image_encoders_free_some_independent_glz_drawables(ImageEncoders *enc,
> > int max_to_free)
> > {
> > RingItem *ring_link;
> > int n = 0;
> >
> > - if (!enc) {
> > - return 0;
> > + if (!enc || !enc->glz_dict || max_to_free <= 0) {
> > + return n;
> > }
> > - 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 (max_to_free > n
> > + && (ring_link = ring_get_head(&enc->glz_orphans)) != NULL) {
> > + GlzDictItem *glz_item = SPICE_CONTAINEROF(ring_link, GlzDictItem,
> > retention_link);
> > + if (glz_context_free(glz_item)) {
> > + ++n;
> > }
> > }
> > + pthread_rwlock_unlock(&enc->glz_dict->encode_lock);
> > return n;
> > }
> >
> > @@ -613,14 +543,12 @@ void
> > image_encoders_free_glz_drawables_to_free(ImageEncoders* enc)
> > if (!enc->glz_dict) {
> > return;
> > }
> > - 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,
> > -
> > GlzDrawableInstanceItem,
> > -
> > free_link);
> > - glz_drawable_instance_item_free(drawable_instance);
> > + pthread_mutex_lock(&enc->glz_dict_items_to_free_lock);
> > + while ((ring_link = ring_get_head(&enc->glz_dict_items_to_free))) {
> > + GlzDictItem * glz_item = SPICE_CONTAINEROF(ring_link, GlzDictItem,
> > free_link);
> > + glz_context_free(glz_item);
>
> And here for example, renaming gets even more in the way. Are
> glz_context_free() and glz_drawable_instance_item_free() the same
> method? Are they doing something different? Looking at their code, they
> sure are different. How much is due to renaming, how much is related to
> the merging/some of the design changes you introduced? This really is
> difficult for me to review, any changes that this is possible to split?
>
> Christophe
>
Beside the red_drawable_unref I don't see any other split possible.
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel