[Spice-devel] [PATCH spice-gtk 08/14] channel: do not reenter the mainloop at every iteration
Yonit Halperin
yhalperi at redhat.com
Tue Sep 10 12:15:47 PDT 2013
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);
>
More information about the Spice-devel
mailing list