[PATCH wayland v2 1/2] client: update obsolete comments

[Marek Chalupa]

On 28 November 2014 at 14:30, Pekka Paalanen <ppaalanen at gmail.com> wrote:

> On Fri, 28 Nov 2014 12:18:55 +0100
> Marek Chalupa <mchqwerty at gmail.com> wrote:
>
> > 1) there is nothing like main thread since
> >    3c7e8bfbb4745315b7bcbf69fa746c3d6718c305 anymore, so remove
> >    it from documentation and update the doc accordingly.
> >
> > 2) use calling 'default queue' instead of 'main queue'. In the code
> >    we use display->default_queue, so it'll be easier the understand.
> >
> > 3) update some obsolete or unprecise pieces of documentation
> >
> > Signed-off-by: Marek Chalupa <mchqwerty at gmail.com>
> > ---
> >  src/wayland-client.c | 80
> ++++++++++++++++++++++++++++++++--------------------
> >  src/wayland-client.h | 26 ++++++++---------
> >  2 files changed, 61 insertions(+), 45 deletions(-)
> >
> > diff --git a/src/wayland-client.c b/src/wayland-client.c
> > index 36380fe..0ab94e9 100644
> > --- a/src/wayland-client.c
> > +++ b/src/wayland-client.c
> > @@ -1333,18 +1333,19 @@ wl_display_prepare_read_queue(struct wl_display
> *display,
> >   * wl_display_dispatch(display);
> >   * \endcode
> >   *
> > - * There are two races here: first, before blocking in poll(), the fd
> > - * could become readable and another thread reads the events.  Some of
> > - * these events may be for the main queue and the other thread will
> > - * queue them there and then the main thread will go to sleep in
> > - * poll().  This will stall the application, which could be waiting
> > - * for a event to kick of the next animation frame, for example.
> > - *
> > - * The other race is immediately after poll(), where another thread
> > - * could preempt and read events before the main thread calls
> > - * wl_display_dispatch().  This call now blocks and starves the other
> > + * The race is immediately after poll(), where one thread
> > + * could preempt and read events before the other thread calls
> > + * wl_display_dispatch(). This call now blocks and starves the other
> >   * fds in the event loop.
> >   *
> > + * Another race would be when using more event queues.
> > + * When one thread calls wl_display_dispatch(_queue)(), then it
> > + * reads all events from display's fd and queues them in appropriate
> > + * queues. Then it dispatches only its own queue and the other events
> > + * are sitting in their queues, waiting for dispatching. If that happens
> > + * before the other thread managed to call poll(), it will
> > + * block with events queued.
> > + *
> >   * A correct sequence would be:
> >   * \code
> >   * while (wl_display_prepare_read(display) != 0)
> > @@ -1358,9 +1359,12 @@ wl_display_prepare_read_queue(struct wl_display
> *display,
> >   * Here we call wl_display_prepare_read(), which ensures that between
> >   * returning from that call and eventually calling
> >   * wl_display_read_events(), no other thread will read from the fd and
> > - * queue events in our queue.  If the call to
> > - * wl_display_prepare_read() fails, we dispatch the pending events and
> > - * try again until we're successful.
> > + * queue events in our queue. If the call to wl_display_prepare_read()
> fails,
> > + * we dispatch the pending events and try again until we're successful.
>
> > + * If we have more queues, we may want to use
> > + * wl_display_dispatch_queue_pending() instead of
> wl_display_dispatch_pending(),
> > + * to be sure that we dispatched all the queues (not only the default
> one).
> > + * Also use \ref wl_display_prepare_read_queue() in this case.
>
> That is inaccurate. wl_display_dispatch_queue_pending() will dispatch
> only one queue, but the text seems to imply it might dispatch all, and
> that the default queue would need to be dispatched always.
>
>
True


> The wl_display_prepare_read_queue() needs to always match the
> wl_display_dispatch_queue_pending().
>
> Each thread should dispatch only the queue(s) it owns, never any other
> queues, and I feel also that is not clearly communicated in this.
>
> If a thread has multiple queues, there is no requirement to handle them
> all at the same time. Each queue usually has a different context where
> it needs to dispatch, and dispatching elsewhere would be a problem.
>
> How about the following:
>
> "If the relevant queue is not the default queue, then
> wl_display_prepare_read_queue() and wl_display_dispatch_queue_pending()
> need to be used instead."
>
>
Yes, that sounds good


> >   *
> >   * \memberof wl_display
> >   */
> > @@ -1399,10 +1403,25 @@ wl_display_cancel_read(struct wl_display
> *display)
> >   * Dispatch all incoming events for objects assigned to the given
> >   * event queue. On failure -1 is returned and errno set appropriately.
> >   *
> > - * This function blocks if there are no events to dispatch. If calling
> from
> > - * the main thread, it will block reading data from the display fd. For
> other
> > - * threads this will block until the main thread queues events on the
> queue
> > - * passed as argument.
> > + * The behaviour of this function is exactly the same as the behaviour
> of
> > + * wl_display_dispatch(), but it dispatches events on given queue,
> > + * not on the default queue.
> > + *
> > + * This function blocks if there are no events to dispatch. When this
> > + * function returns after blocking, we can be sure that if some events
> > + * for given queue came, they are dispatched now. Since we can not be
> sure
> > + * that among incoming events are those for our queue, we need to check
> > + * the return value. If it is equal to 0, it means that
> > + * this function read events from fd and queued them to appropriate
> queues,
> > + * but since there were no events for our queue, it dispatched none.
>
> > + * Make sure you'll dispatch pending events on the other queues too in
> > + * this case.
>
> This last sentence: again, each queue needs to be dispatched from the
> correct context (thread, locks held, etc.), so I suggest rephrasing it
> more like: "Make sure also the other queues get dispatched in their
> appropriate context (thread, locks held, etc.)." Or maybe just leave it
> out, since saying no other queues got dispatched should be enough.


Leaving it out.


> > +
> > + * \note Display queue (for wl_display events like delete_id or so)
> > + * is dispatched always.
>
> I wonder if referring to the display queue is just confusing, because I
> don't see us really explaining what it is anywhere. And it's not the
> default queue. In fact, I don't think the users of libwayland would even
> care about it.
>

True.

Hmm, just thinking about it - I should rephrase the thing about checking
return value of wl_display_dispatch_queue(). Because when
it dispatched some events in display_queue, then it won't return 0 even
when it dispatched none of the queue-assigned events, so what I wrote in
the comment to wl_display_dispatch_queue() is wrong.

But to clarify that it can return non-0 even when it dispatched no events,
I have to say the thing about display queue, don't I?


>
> > + *
> > + * \sa wl_display_dispatch(), wl_display_dispatch_pending(),
> > + * wl_display_dispatch_queue_pending()
> >   *
> >   * \memberof wl_display
> >   */
> > @@ -1499,17 +1518,20 @@ wl_display_dispatch_queue_pending(struct
> wl_display *display,
> >   * \param display The display context object
> >   * \return The number of dispatched events on success or -1 on failure
> >   *
> > - * Dispatch the display's main event queue.
> > + * Dispatch the display's default event queue.
> >   *
> > - * If the main event queue is empty, this function blocks until there
> are
> > + * If the default event queue is empty, this function blocks until
> there are
> >   * events to be read from the display fd. Events are read and queued on
> > - * the appropriate event queues. Finally, events on the main event queue
> > + * the appropriate event queues. Finally, events on the default event
> queue
> >   * are dispatched.
> >   *
> > - * \note It is not possible to check if there are events on the main
> queue
> > - * or not. For dispatching main queue events without blocking, see \ref
> > + * \note It is not possible to check if there are events on the queue
> > + * or not. For dispatching default queue events without blocking, see
> \ref
> >   * wl_display_dispatch_pending().
> >   *
> > + * \note This function always dispatches display queue (the queue
> > + * for wl_display events - this is different from default queue)
>
> The same about the display queue here. I think it's more of an
> implementation detail than anything a user would care about.
>
> > + *
> >   * \sa wl_display_dispatch_pending(), wl_display_dispatch_queue()
> >   *
> >   * \memberof wl_display
> > @@ -1520,7 +1542,7 @@ wl_display_dispatch(struct wl_display *display)
> >       return wl_display_dispatch_queue(display, &display->default_queue);
> >  }
> >
> > -/** Dispatch main queue events without reading from the display fd
> > +/** Dispatch default queue events without reading from the display fd
> >   *
> >   * \param display The display context object
> >   * \return The number of dispatched events or -1 on failure
> > @@ -1532,8 +1554,8 @@ wl_display_dispatch(struct wl_display *display)
> >   * This is necessary when a client's main loop wakes up on some fd other
> >   * than the display fd (network socket, timer fd, etc) and calls \ref
> >   * wl_display_dispatch_queue() from that callback. This may queue up
> > - * events in the main queue while reading all data from the display fd.
> > - * When the main thread returns to the main loop to block, the display
> fd
> > + * events in other queues while reading all data from the display fd.
> > + * When the main loop returns from the handler, the display fd
> >   * no longer has data, causing a call to \em poll(2) (or similar
> >   * functions) to block indefinitely, even though there are events ready
> >   * to dispatch.
> > @@ -1542,16 +1564,14 @@ wl_display_dispatch(struct wl_display *display)
> >   * client should always call wl_display_dispatch_pending() and then
> >   * \ref wl_display_flush() prior to going back to sleep. At that point,
> >   * the fd typically doesn't have data so attempting I/O could block, but
> > - * events queued up on the main queue should be dispatched.
> > + * events queued up on the default queue should be dispatched.
> >   *
> >   * A real-world example is a main loop that wakes up on a timerfd (or a
> >   * sound card fd becoming writable, for example in a video player),
> which
> >   * then triggers GL rendering and eventually eglSwapBuffers().
> >   * eglSwapBuffers() may call wl_display_dispatch_queue() if it didn't
> >   * receive the frame event for the previous frame, and as such queue
> > - * events in the main queue.
> > - *
> > - * \note Calling this makes the current thread the main one.
> > + * events in the default queue.
> >   *
> >   * \sa wl_display_dispatch(), wl_display_dispatch_queue(),
> >   * wl_display_flush()
> > @@ -1735,7 +1755,7 @@ wl_proxy_get_class(struct wl_proxy *proxy)
> >   * \param queue The event queue that will handle this proxy
> >   *
> >   * Assign proxy to event queue. Events coming from \c proxy will be
> > - * queued in \c queue instead of the display's main queue.
> > + * queued in \c queue instead of the display's default queue.
>
> Correct up until "instead", because I believe we nowadays have queue
> inheritance: a new proxy will inherit the queue of the proxy it was
> created with. Some adjustment to that might be nice here (and verifying
> that I'm not lying.)
>

Yep, proxy_create and wl_create_proxy_for_id functions assings the proxy
from factory to the new proxy.


> >   *
> >   * \sa wl_display_dispatch_queue()
> >   *
> > diff --git a/src/wayland-client.h b/src/wayland-client.h
> > index dd42d7b..71cd822 100644
> > --- a/src/wayland-client.h
> > +++ b/src/wayland-client.h
> > @@ -71,7 +71,7 @@ struct wl_proxy;
> >   * added to a queue. On the dispatch step, the handler for the incoming
> >   * event set by the client on the corresponding \ref wl_proxy is called.
> >   *
> > - * A wl_display has at least one event queue, called the <em>main
> > + * A wl_display has at least one event queue, called the <em>default
> >   * queue</em>. Clients can create additional event queues with \ref
> >   * wl_display_create_queue() and assign \ref wl_proxy's to it. Events
> >   * occurring in a particular proxy are always queued in its assigned
> queue.
> > @@ -80,31 +80,27 @@ struct wl_proxy;
> >   * called by assigning that proxy to an event queue and making sure that
> >   * this queue is only dispatched when the assumption holds.
>
> (Btw. the above paragraph explains the term "context", even if it
> doesn't call it that.)
>
> >   *
> > - * The main queue is dispatched by calling \ref wl_display_dispatch().
> > - * This will dispatch any events queued on the main queue and attempt
> > - * to read from the display fd if its empty. Events read are then queued
> > - * on the appropriate queues according to the proxy assignment. Calling
> > - * that function makes the calling thread the <em>main thread</em>.
> > + * The default queue is dispatched by calling \ref
> wl_display_dispatch().
> > + * This will dispatch any events queued on the default queue and attempt
> > + * to read from the display fd if it's empty. Events read are then
> queued
> > + * on the appropriate queues according to the proxy assignment.
> >   *
> >   * A user created queue is dispatched with \ref
> wl_display_dispatch_queue().
> > - * If there are no events to dispatch this function will block. If this
> > - * is called by the main thread, this will attempt to read data from the
> > - * display fd and queue any events on the appropriate queues. If calling
> > - * from any other thread, the function will block until the main thread
> > - * queues an event on the queue being dispatched.
> > + * This function behaves exactly the same as wl_display_dispatch()
> > + * but it dispatches given queue instead of the default queue.
> >   *
> >   * A real world example of event queue usage is Mesa's implementation of
> >   * eglSwapBuffers() for the Wayland platform. This function might need
> > - * to block until a frame callback is received, but dispatching the main
> > + * to block until a frame callback is received, but dispatching the
> default
> >   * queue could cause an event handler on the client to start drawing
> >   * again. This problem is solved using another event queue, so that only
> >   * the events handled by the EGL code are dispatched during the block.
> >   *
> > - * This creates a problem where the main thread dispatches a non-main
> > + * This creates a problem where a thread dispatches a non-default
> >   * queue, reading all the data from the display fd. If the application
> >   * would call \em poll(2) after that it would block, even though there
> > - * might be events queued on the main queue. Those events should be
> > - * dispatched with \ref wl_display_dispatch_pending() before
> > + * might be events queued on the default queue. Those events should be
> > + * dispatched with \ref wl_display_dispatch_(queue_)pending() before
> >   * flushing and blocking.
> >   */
> >  struct wl_display;
>
> Everything I didn't comment about looks good. I really appreciate going
> through all this.
>
> One more thing we perhaps should add: the current threading
> support/model was first released in Wayland 1.2, right? We might want
> to add a warning somewhere, that Wayland before 1.2 behaved differently
> wrt. threads. (another patch, maybe)
>
> Another note is that the private display queue was added pretty late in
> b9eebce0aa5559855d835e403ba3bb5960baaadc which makes it first released
> in Wayland 1.5. That's probably not important to mention, though.
>
>
> Thanks,
> pq
>

Thanks!
Marek
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20141203/bfb79718/attachment-0001.html>