[Spice-devel] [spice-gtk v3] vmcstream, gtask: Do idle ourself instead of leaving it to GTask's heuristic

[Victor Toso]

Hi,

On Tue, Mar 22, 2016 at 10:33:56PM +0100, Fabiano Fidêncio wrote:
> Seems that GTask heuristic only makes sense in a non-coroutine case.
> After opening a bug[0] on spice-gtk and a few discussions in the mailing

[0] ?

> list, seems that is completely fine for coroutine code to deal with the
> idle explicitly.
>
> Signed-off-by: Fabiano Fidêncio <fidencio at redhat.com>

"vmcstream, gtask: Do idle ourself instead of leaving it to GTask's heuristic"

I would go with "vmcstream: must return in idle from coroutine context"
but feel free to ignore it.

Acked-by: Victor Toso <victortoso at redhat.com>

Tested and working. Thanks for this fix.

I wonder how we could improve this to avoid the complete-in-idle in
the future. It would be nice.

> ---
> Changes since v2, as per Marc-André review:
> - Change the commit log
> - Change the comment in the code
> - No need for a _free() function, just do the unref and free inside _idle_cb()
> 
> Changes since v1, as per Marc-André review:
> - Use g_idle_add() instead of GSource ... which worries me a bit about the
>   context used when using g_idle_add(), as previously the context used was
>   the one from the GTask.
> ---
> 
>  src/vmcstream.c | 29 +++++++++++++++++++++++++++--
>  1 file changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/src/vmcstream.c b/src/vmcstream.c
> index 5ebf15a..5dd2799 100644
> --- a/src/vmcstream.c
> +++ b/src/vmcstream.c
> @@ -97,6 +97,24 @@ spice_vmc_input_stream_new(void)
>      return self;
>  }
>  
> +typedef struct _complete_in_idle_cb_data {
> +    GTask *task;
> +    gssize pos;
> +} complete_in_idle_cb_data;
> +
> +static gboolean
> +complete_in_idle_cb(gpointer user_data)
> +{
> +    complete_in_idle_cb_data *data = user_data;
> +
> +    g_task_return_int(data->task, data->pos);
> +
> +    g_object_unref (data->task);
> +    g_free (data);
> +
> +    return FALSE;
> +}
> +
>  /* coroutine */
>  /**
>   * Feed a SpiceVmc stream with new data from a coroutine
> @@ -116,6 +134,8 @@ spice_vmc_input_stream_co_data(SpiceVmcInputStream *self,
>      self->coroutine = coroutine_self();
>  
>      while (size > 0) {
> +        complete_in_idle_cb_data *cb_data;
> +
>          SPICE_DEBUG("spicevmc co_data %p", self->task);
>          if (!self->task)
>              coroutine_yield(NULL);
> @@ -137,10 +157,15 @@ spice_vmc_input_stream_co_data(SpiceVmcInputStream *self,
>          if (self->all && min > 0 && self->pos != self->count)
>              continue;
>  
> -        g_task_return_int(self->task, self->pos);
> +        /* Let's deal with the task complete in idle by ourselves, as GTask
> +         * heuristic only makes sense in a non-coroutine case.
> +         */
> +        cb_data = g_new(complete_in_idle_cb_data , 1);
> +        cb_data->task = g_object_ref(self->task);
> +        cb_data->pos = self->pos;
> +        g_idle_add(complete_in_idle_cb, cb_data);
>  
>          g_cancellable_disconnect(g_task_get_cancellable(self->task), self->cancel_id);
> -
>          g_clear_object(&self->task);
>      }
>  
> -- 
> 2.5.0
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel