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

[Yonit Halperin]

ACK

The rest of the patches that haven't been acked in this series, are the 
same as in the previous series, right? If so, ack series.

Cheers,
Yonit.
On 09/12/2013 08:09 AM, 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.
>
> This is the kind of results I get with a replay:
>
> before:
>
>         2179,440455 task-clock                #    0,629 CPUs utilized
>               3 580 context-switches          #    0,002 M/sec
>                 207 cpu-migrations            #    0,095 K/sec
>              48 909 page-faults               #    0,022 M/sec
>       7 362 442 301 cycles                    #    3,378 GHz
>       5 256 957 520 stalled-cycles-frontend   #   71,40% frontend cycles
>               idle
>     <not supported> stalled-cycles-backend
>       5 460 266 981 instructions              #    0,74  insns per cycle
>                                               #    0,96  stalled cycles
>               per insn
>         982 749 523 branches                  #  450,918 M/sec
>          20 745 453 branch-misses             #    2,11% of all branches
>
>         3,463422087 seconds time elapsed
>
> after:
>
>         1741,651383 task-clock                #    0,522 CPUs utilized
>               5 093 context-switches          #    0,003 M/sec
>                 137 cpu-migrations            #    0,079 K/sec
>              49 033 page-faults               #    0,028 M/sec
>       5 874 567 974 cycles                    #    3,373 GHz
>       4 247 059 288 stalled-cycles-frontend   #   72,30% frontend cycles
>               idle
>     <not supported> stalled-cycles-backend
>       4 419 666 346 instructions              #    0,75  insns per cycle
>                                               #    0,96  stalled cycles
>               per insn
>         776 972 366 branches                  #  446,112 M/sec
>          17 862 170 branch-misses             #    2,30% of all branches
>
>         3,337472053 seconds time elapsed
> ---
>   gtk/spice-channel.c | 59 +++++++++++++++++++++++++----------------------------
>   1 file changed, 28 insertions(+), 31 deletions(-)
>
> diff --git a/gtk/spice-channel.c b/gtk/spice-channel.c
> index 55c8325..f6a691e 100644
> --- a/gtk/spice-channel.c
> +++ b/gtk/spice-channel.c
> @@ -2080,10 +2080,24 @@ static void spice_channel_iterate_read(SpiceChannel *channel)
>   {
>       SpiceChannelPrivate *c = channel->priv;
>
> -    g_return_if_fail(c->state != SPICE_CHANNEL_STATE_MIGRATING);
> +    g_coroutine_socket_wait(&c->coroutine, c->sock, G_IO_IN);
> +
> +    /* treat all incoming data (block on message completion) */
> +    while (!c->has_error &&
> +           c->state != SPICE_CHANNEL_STATE_MIGRATING &&
> +           g_socket_condition_check(c->sock, G_IO_IN) & G_IO_IN) {
> +
> +        do
> +            spice_channel_recv_msg(channel,
> +                                   (handler_msg_in)SPICE_CHANNEL_GET_CLASS(channel)->handle_msg, NULL);
> +#if HAVE_SASL
> +            /* flush the sasl buffer too */
> +        while (c->sasl_decoded != NULL);
> +#else
> +        while (FALSE);
> +#endif
> +    }
>
> -    spice_channel_recv_msg(channel,
> -        (handler_msg_in)SPICE_CHANNEL_GET_CLASS(channel)->handle_msg, NULL);
>   }
>
>   static gboolean wait_migration(gpointer data)
> @@ -2105,37 +2119,20 @@ 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->has_error) {
> -            CHANNEL_DEBUG(channel, "channel has error, breaking loop");
> -            return FALSE;
> -        }
> -
> -        SPICE_CHANNEL_GET_CLASS(channel)->iterate_write(channel);
> -
> -        ret = g_coroutine_socket_wait(&c->coroutine, c->sock,
> -            c->state != SPICE_CHANNEL_STATE_MIGRATING ? G_IO_IN : 0);
> -
> -#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 */
> +    if (c->state == SPICE_CHANNEL_STATE_MIGRATING &&
> +        !g_coroutine_condition_wait(&c->coroutine, wait_migration, channel))
> +        CHANNEL_DEBUG(channel, "migration wait cancelled");
>
> -    if (ret & G_IO_IN) {
> -        do
> -            SPICE_CHANNEL_GET_CLASS(channel)->iterate_read(channel);
> -#if HAVE_SASL
> -        while (c->sasl_decoded != NULL);
> -#else
> -        while (FALSE);
> -#endif
> +    if (c->has_error) {
> +        CHANNEL_DEBUG(channel, "channel has error, breaking loop");
> +        return FALSE;
>       }
>
> +    /* flush any pending write and read */
> +    SPICE_CHANNEL_GET_CLASS(channel)->iterate_write(channel);
> +    SPICE_CHANNEL_GET_CLASS(channel)->iterate_read(channel);
> +
> +    ret = g_socket_condition_check(c->sock, G_IO_IN | G_IO_ERR | G_IO_HUP);
>       if (c->state > SPICE_CHANNEL_STATE_CONNECTING &&
>           ret & (G_IO_ERR|G_IO_HUP)) {
>           SPICE_DEBUG("got socket error: %d", ret);
>