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

[Marc-André Lureau]

argh, I reintroduced it by mistake..

On Tue, Sep 10, 2013 at 9:15 PM, Yonit Halperin <yhalperi at redhat.com> wrote:
> Hi,
>
>
> On 09/10/2013 10:44 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.
>> ---
>>   gtk/spice-channel.c | 57
>> ++++++++++++++++++++++++-----------------------------
>>   1 file changed, 26 insertions(+), 31 deletions(-)
>>
>> diff --git a/gtk/spice-channel.c b/gtk/spice-channel.c
>> index 46caeef..01598d3 100644
>> --- a/gtk/spice-channel.c
>> +++ b/gtk/spice-channel.c
>> @@ -2080,10 +2080,22 @@ static void
>> spice_channel_iterate_read(SpiceChannel *channel)
>>   {
>>       SpiceChannelPrivate *c = channel->priv;
>>
>> -    g_return_if_fail(c->state != SPICE_CHANNEL_STATE_MIGRATING);
>> +    /* treat all incoming data (block on message completion) */
>> +    while (!c->has_error &&
>> +           c->state != SPICE_CHANNEL_STATE_MIGRATING &&
>> +           g_coroutine_socket_wait(&c->coroutine, c->sock, G_IO_IN) &
>> G_IO_IN) {
>> +
>
> This is logically different from the previous version of the patch. Didn't
> you want to use g_socket_condition_check here, and call
> g_coroutine_socket_wait in between the calls to iterate_write and
> iterate_read in order to save some unnecessary switches to the main loop?
> Besides that and and the nitpick for patch #3, the series looks good. Ack.
>
>> +        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 +2117,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);
>> +    if (c->state == SPICE_CHANNEL_STATE_MIGRATING &&
>> +        !g_coroutine_condition_wait(&c->coroutine, wait_migration,
>> channel))
>> +        CHANNEL_DEBUG(channel, "migration wait cancelled");
>>
>> -        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 (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);
>>
>



-- 
Marc-André Lureau