[Spice-devel] [PATCH spice-gtk 07/10] channel: do not reenter the mainloop at every iteration

[Yonit Halperin]

Hi,

I think that iterate_write & iterate_read should be symmetrical: in the 
same way iterate_write handles all outgoing messages, iterate_read 
should flush all incoming messages (i.e., the loop in 
spice_channel_iterate can be moved to iterate_read).

But I'll ack it even if you don't change it :)

Cheers,
Yonit.

On 09/08/2013 02:59 PM, Marc-André Lureau wrote:
> From: Marc-André Lureau <marcandre.lureau at gmail.com>
>
> The current coroutine channel_iterate() creates a GSource for the socket
> reenters the mainloop waiting for IO condition. This is really heavy
> usage of mainloop showing up in system time and user space
> profiling (10% of CPU time spent in mainloop overhead). Instead flush
> all pending input messages before switching context and reentering main
> loop.
> ---
>   gtk/spice-channel.c | 35 +++++++++++++++++------------------
>   1 file changed, 17 insertions(+), 18 deletions(-)
>
> diff --git a/gtk/spice-channel.c b/gtk/spice-channel.c
> index 778a90b..9fb3b0c 100644
> --- a/gtk/spice-channel.c
> +++ b/gtk/spice-channel.c
> @@ -2107,35 +2107,34 @@ static gboolean spice_channel_iterate(SpiceChannel *channel)
>       SpiceChannelPrivate *c = channel->priv;
>       GIOCondition ret;
>
> -    do {
> -        if (c->state == SPICE_CHANNEL_STATE_MIGRATING &&
> -            !g_coroutine_condition_wait(&c->coroutine, wait_migration, channel))
> -                CHANNEL_DEBUG(channel, "migration wait cancelled");
> +    if (c->state == SPICE_CHANNEL_STATE_MIGRATING &&
> +        !g_coroutine_condition_wait(&c->coroutine, wait_migration, channel))
> +        CHANNEL_DEBUG(channel, "migration wait cancelled");
>
> -        if (c->has_error) {
> -            CHANNEL_DEBUG(channel, "channel has error, breaking loop");
> -            return FALSE;
> -        }
> -
> -        SPICE_CHANNEL_GET_CLASS(channel)->iterate_write(channel);
> +    if (c->has_error) {
> +        CHANNEL_DEBUG(channel, "channel has error, breaking loop");
> +        return FALSE;
> +    }
>
> -        ret = g_coroutine_socket_wait(&c->coroutine, c->sock,
> -            c->state != SPICE_CHANNEL_STATE_MIGRATING ? G_IO_IN : 0);
> +    /* flush any pending write */
> +    SPICE_CHANNEL_GET_CLASS(channel)->iterate_write(channel);
> +    /* go back to main loop, wait for an event or incoming data */
> +    ret = g_coroutine_socket_wait(&c->coroutine, c->sock, G_IO_IN);
>
> -#ifdef WIN32
> -        /* FIXME: windows gsocket is buggy, it doesn't return correct condition... */
> -        ret = g_socket_condition_check(c->sock, G_IO_IN);
> -#endif
> -    } while (ret == 0); /* ret == 0 means no IO condition, but woken up */
> +    /* treat all incoming data (block on message completion) */
> +    while (!c->has_error && (ret & G_IO_IN)) {
> +        if (c->state == SPICE_CHANNEL_STATE_MIGRATING)
> +            break;
>
> -    if (ret & G_IO_IN) {
>           do
>               SPICE_CHANNEL_GET_CLASS(channel)->iterate_read(channel);
>   #if HAVE_SASL
> +        /* flush the sasl buffer too */
>           while (c->sasl_decoded != NULL);
>   #else
>           while (FALSE);
>   #endif
> +        ret = g_socket_condition_check(c->sock, G_IO_IN | G_IO_ERR | G_IO_HUP);
>       }
>
>       if (c->state > SPICE_CHANNEL_STATE_CONNECTING &&
>