[PATCH wayland 8/8] client: Use read preparation API in wl_display_dispatch_queue()

[Jonas Ådahl]

On Fri, Oct 02, 2015 at 01:42:03PM -0700, Bryce Harrington wrote:
> On Fri, Oct 02, 2015 at 05:32:59PM +0800, Jonas Ådahl wrote:
> > Instead of doing things that do the equivalent of using
> > wl_display_prepare_read() and friends, just use the public API. The
> > only semantical difference is that we will now unlock and lock the mutex
> > more times compared to before.
> > 
> > Signed-off-by: Jonas Ådahl <jadahl at gmail.com>
> > ---
> > 
> > There is no real need to merge this patch, it only makes it easier to
> > understand that the _dispatch(_queue)() functions are compatible with the
> > prepare_read() family.
> 
> I'm all for making code more concise and clear.  Is there any concern
> though that the added locking activity impacts performance negatively?

I haven't done any measurements, but since mutex locking is quite fast,
the locking increase is linear, and the assumption that we are not going
to use these types of functions in any hot paths, I'd say its probably
harmless.


Jonas

> 
> Technically I see nothing wrong with the implementation.
> 
> > Jonas
> > 
> > 
> >  src/wayland-client.c | 35 ++++++++---------------------------
> >  1 file changed, 8 insertions(+), 27 deletions(-)
> > 
> > diff --git a/src/wayland-client.c b/src/wayland-client.c
> > index ca2495f..8f53f6f 100644
> > --- a/src/wayland-client.c
> > +++ b/src/wayland-client.c
> > @@ -1507,15 +1507,9 @@ wl_display_dispatch_queue(struct wl_display *display,
> >  	struct pollfd pfd[2];
> >  	int ret;
> >  
> > -	pthread_mutex_lock(&display->mutex);
> > -
> > -	ret = dispatch_queue(display, queue);
> > -	if (ret == -1)
> > -		goto err_unlock;
> > -	if (ret > 0) {
> > -		pthread_mutex_unlock(&display->mutex);
> > +	ret = wl_display_dispatch_queue_pending(display, queue);
> > +	if (ret != 0)
> >  		return ret;
> > -	}
> >  
> >  	/* We ignore EPIPE here, so that we try to read events before
> >  	 * returning an error.  When the compositor sends an error it
> > @@ -1524,12 +1518,11 @@ wl_display_dispatch_queue(struct wl_display *display,
> >  	ret = wl_connection_flush(display->connection);
> >  	if (ret < 0 && errno != EAGAIN && errno != EPIPE) {
> >  		display_fatal_error(display, errno);
> > -		goto err_unlock;
> > +		return -1;
> >  	}
> >  
> > -	display->reader_count++;
> > -
> > -	pthread_mutex_unlock(&display->mutex);
> > +	if (wl_display_prepare_read_queue(display, queue) != 0)
> > +		return -1;
> >  
> >  	pfd[0].fd = display->fd;
> >  	pfd[0].events = POLLIN;
> > @@ -1542,22 +1535,10 @@ wl_display_dispatch_queue(struct wl_display *display,
> >  		return -1;
> >  	}
> >  
> > -	pthread_mutex_lock(&display->mutex);
> > -
> > -	if (read_events(display) == -1)
> > -		goto err_unlock;
> > -
> > -	ret = dispatch_queue(display, queue);
> > -	if (ret == -1)
> > -		goto err_unlock;
> > -
> > -	pthread_mutex_unlock(&display->mutex);
> > -
> > -	return ret;
> > +	if (wl_display_read_events(display) == -1)
> > +		return -1;
> >  
> > - err_unlock:
> > -	pthread_mutex_unlock(&display->mutex);
> > -	return -1;
> > +	return wl_display_dispatch_queue_pending(display, queue);
> >  }
> >  
> >  /** Dispatch pending events in an event queue
> > -- 
> > 2.4.3
> > 
> > _______________________________________________
> > wayland-devel mailing list
> > wayland-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/wayland-devel