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

[Bryce Harrington]

On Fri, Oct 02, 2015 at 01:50:09PM -0700, Bryce Harrington wrote:
> 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>
> 
> In this series, patches 1,2,3,5 are mainly doc cleanup and look fine to
> land.  If no one comments to the contrary by EOD today I'll just go
> ahead and land them.

Pushed:

remote: I: patch #60925 updated using rev e74cde739eee62be15d85ad62ce8b7c8bbd90d74
remote: I: patch #60924 updated using rev f755dbde26d817b5937d8cebc8aa55191bddaaf3
remote: I: patch #60926 updated using rev 5e0ed917744f2f881956e91ebe971073fd0a7c51
remote: I: patch #60928 updated using rev 99c34e58dbc9357527d374abf3bb5ca952d91942
remote: I: 4 patch(es) updated to state Accepted.
To ssh://git.freedesktop.org/git/wayland/wayland
   e0b2166..99c34e5  master -> master


>  On 4 I spotted a grammar issue but otherwise also
> looks good to go.
> 
> Patches 6-8 look to me like they deserve a bit deeper review.
> 
> Thanks,
> Bryce
> 
> 
> > > 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?
> > 
> > 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
> > _______________________________________________
> > wayland-devel mailing list
> > wayland-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel