<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On 28 November 2014 at 14:30, Pekka Paalanen <span dir="ltr"><<a href="mailto:ppaalanen@gmail.com" target="_blank">ppaalanen@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Fri, 28 Nov 2014 12:18:55 +0100<br> Marek Chalupa <<a href="mailto:mchqwerty@gmail.com">mchqwerty@gmail.com</a>> wrote:<br> <br> > 1) there is nothing like main thread since<br> > 3c7e8bfbb4745315b7bcbf69fa746c3d6718c305 anymore, so remove<br> > it from documentation and update the doc accordingly.<br> ><br> > 2) use calling 'default queue' instead of 'main queue'. In the code<br> > we use display->default_queue, so it'll be easier the understand.<br> ><br> > 3) update some obsolete or unprecise pieces of documentation<br> ><br> > Signed-off-by: Marek Chalupa <<a href="mailto:mchqwerty@gmail.com">mchqwerty@gmail.com</a>><br> > ---<br> > src/wayland-client.c | 80 ++++++++++++++++++++++++++++++++--------------------<br> > src/wayland-client.h | 26 ++++++++---------<br> > 2 files changed, 61 insertions(+), 45 deletions(-)<br> ><br> > diff --git a/src/wayland-client.c b/src/wayland-client.c<br> > index 36380fe..0ab94e9 100644<br> > --- a/src/wayland-client.c<br> > +++ b/src/wayland-client.c<br> > @@ -1333,18 +1333,19 @@ wl_display_prepare_read_queue(struct wl_display *display,<br> > * wl_display_dispatch(display);<br> > * \endcode<br> > *<br> > - * There are two races here: first, before blocking in poll(), the fd<br> > - * could become readable and another thread reads the events. Some of<br> > - * these events may be for the main queue and the other thread will<br> > - * queue them there and then the main thread will go to sleep in<br> > - * poll(). This will stall the application, which could be waiting<br> > - * for a event to kick of the next animation frame, for example.<br> > - *<br> > - * The other race is immediately after poll(), where another thread<br> > - * could preempt and read events before the main thread calls<br> > - * wl_display_dispatch(). This call now blocks and starves the other<br> > + * The race is immediately after poll(), where one thread<br> > + * could preempt and read events before the other thread calls<br> > + * wl_display_dispatch(). This call now blocks and starves the other<br> > * fds in the event loop.<br> > *<br> > + * Another race would be when using more event queues.<br> > + * When one thread calls wl_display_dispatch(_queue)(), then it<br> > + * reads all events from display's fd and queues them in appropriate<br> > + * queues. Then it dispatches only its own queue and the other events<br> > + * are sitting in their queues, waiting for dispatching. If that happens<br> > + * before the other thread managed to call poll(), it will<br> > + * block with events queued.<br> > + *<br> > * A correct sequence would be:<br> > * \code<br> > * while (wl_display_prepare_read(display) != 0)<br> > @@ -1358,9 +1359,12 @@ wl_display_prepare_read_queue(struct wl_display *display,<br> > * Here we call wl_display_prepare_read(), which ensures that between<br> > * returning from that call and eventually calling<br> > * wl_display_read_events(), no other thread will read from the fd and<br> > - * queue events in our queue. If the call to<br> > - * wl_display_prepare_read() fails, we dispatch the pending events and<br> > - * try again until we're successful.<br> > + * queue events in our queue. If the call to wl_display_prepare_read() fails,<br> > + * we dispatch the pending events and try again until we're successful.<br> <br> > + * If we have more queues, we may want to use<br> > + * wl_display_dispatch_queue_pending() instead of wl_display_dispatch_pending(),<br> > + * to be sure that we dispatched all the queues (not only the default one).<br> > + * Also use \ref wl_display_prepare_read_queue() in this case.<br> <br> </div></div>That is inaccurate. wl_display_dispatch_queue_pending() will dispatch<br> only one queue, but the text seems to imply it might dispatch all, and<br> that the default queue would need to be dispatched always.<br> <br></blockquote><div> </div><div style>True</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> The wl_display_prepare_read_queue() needs to always match the<br> wl_display_dispatch_queue_pending().<br> <br> Each thread should dispatch only the queue(s) it owns, never any other<br> queues, and I feel also that is not clearly communicated in this.<br> <br> If a thread has multiple queues, there is no requirement to handle them<br> all at the same time. Each queue usually has a different context where<br> it needs to dispatch, and dispatching elsewhere would be a problem.<br> <br> How about the following:<br> <br> "If the relevant queue is not the default queue, then<br> wl_display_prepare_read_queue() and wl_display_dispatch_queue_pending()<br> need to be used instead."<br> <span class=""><br></span></blockquote><div><br></div><div style>Yes, that sounds good</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""> > *<br> > * \memberof wl_display<br> > */<br> > @@ -1399,10 +1403,25 @@ wl_display_cancel_read(struct wl_display *display)<br> > * Dispatch all incoming events for objects assigned to the given<br> > * event queue. On failure -1 is returned and errno set appropriately.<br> > *<br> > - * This function blocks if there are no events to dispatch. If calling from<br> > - * the main thread, it will block reading data from the display fd. For other<br> > - * threads this will block until the main thread queues events on the queue<br> > - * passed as argument.<br> > + * The behaviour of this function is exactly the same as the behaviour of<br> > + * wl_display_dispatch(), but it dispatches events on given queue,<br> > + * not on the default queue.<br> > + *<br> > + * This function blocks if there are no events to dispatch. When this<br> > + * function returns after blocking, we can be sure that if some events<br> > + * for given queue came, they are dispatched now. Since we can not be sure<br> > + * that among incoming events are those for our queue, we need to check<br> > + * the return value. If it is equal to 0, it means that<br> > + * this function read events from fd and queued them to appropriate queues,<br> > + * but since there were no events for our queue, it dispatched none.<br> <br> > + * Make sure you'll dispatch pending events on the other queues too in<br> > + * this case.<br> <br> </span>This last sentence: again, each queue needs to be dispatched from the<br> correct context (thread, locks held, etc.), so I suggest rephrasing it<br> more like: "Make sure also the other queues get dispatched in their<br> appropriate context (thread, locks held, etc.)." Or maybe just leave it<br> out, since saying no other queues got dispatched should be enough.</blockquote><div><br></div><div style>Leaving it out.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""> > +<br> > + * \note Display queue (for wl_display events like delete_id or so)<br> > + * is dispatched always.<br> <br> </span>I wonder if referring to the display queue is just confusing, because I<br> don't see us really explaining what it is anywhere. And it's not the<br> default queue. In fact, I don't think the users of libwayland would even<br> care about it.<br></blockquote><div><br></div><div style>True.</div><div style><br></div><div style>Hmm, just thinking about it - I should rephrase the thing about checking return value of wl_display_dispatch_queue(). Because when</div><div style>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.</div><div style><br></div><div style>But to clarify that it can return non-0 even when it dispatched no events,</div><div style>I have to say the thing about display queue, don't I?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div class="h5"><br> > + *<br> > + * \sa wl_display_dispatch(), wl_display_dispatch_pending(),<br> > + * wl_display_dispatch_queue_pending()<br> > *<br> > * \memberof wl_display<br> > */<br> > @@ -1499,17 +1518,20 @@ wl_display_dispatch_queue_pending(struct wl_display *display,<br> > * \param display The display context object<br> > * \return The number of dispatched events on success or -1 on failure<br> > *<br> > - * Dispatch the display's main event queue.<br> > + * Dispatch the display's default event queue.<br> > *<br> > - * If the main event queue is empty, this function blocks until there are<br> > + * If the default event queue is empty, this function blocks until there are<br> > * events to be read from the display fd. Events are read and queued on<br> > - * the appropriate event queues. Finally, events on the main event queue<br> > + * the appropriate event queues. Finally, events on the default event queue<br> > * are dispatched.<br> > *<br> > - * \note It is not possible to check if there are events on the main queue<br> > - * or not. For dispatching main queue events without blocking, see \ref<br> > + * \note It is not possible to check if there are events on the queue<br> > + * or not. For dispatching default queue events without blocking, see \ref<br> > * wl_display_dispatch_pending().<br> > *<br> > + * \note This function always dispatches display queue (the queue<br> > + * for wl_display events - this is different from default queue)<br> <br> </div></div>The same about the display queue here. I think it's more of an<br> implementation detail than anything a user would care about.<br> <div><div class="h5"><br> > + *<br> > * \sa wl_display_dispatch_pending(), wl_display_dispatch_queue()<br> > *<br> > * \memberof wl_display<br> > @@ -1520,7 +1542,7 @@ wl_display_dispatch(struct wl_display *display)<br> > return wl_display_dispatch_queue(display, &display->default_queue);<br> > }<br> ><br> > -/** Dispatch main queue events without reading from the display fd<br> > +/** Dispatch default queue events without reading from the display fd<br> > *<br> > * \param display The display context object<br> > * \return The number of dispatched events or -1 on failure<br> > @@ -1532,8 +1554,8 @@ wl_display_dispatch(struct wl_display *display)<br> > * This is necessary when a client's main loop wakes up on some fd other<br> > * than the display fd (network socket, timer fd, etc) and calls \ref<br> > * wl_display_dispatch_queue() from that callback. This may queue up<br> > - * events in the main queue while reading all data from the display fd.<br> > - * When the main thread returns to the main loop to block, the display fd<br> > + * events in other queues while reading all data from the display fd.<br> > + * When the main loop returns from the handler, the display fd<br> > * no longer has data, causing a call to \em poll(2) (or similar<br> > * functions) to block indefinitely, even though there are events ready<br> > * to dispatch.<br> > @@ -1542,16 +1564,14 @@ wl_display_dispatch(struct wl_display *display)<br> > * client should always call wl_display_dispatch_pending() and then<br> > * \ref wl_display_flush() prior to going back to sleep. At that point,<br> > * the fd typically doesn't have data so attempting I/O could block, but<br> > - * events queued up on the main queue should be dispatched.<br> > + * events queued up on the default queue should be dispatched.<br> > *<br> > * A real-world example is a main loop that wakes up on a timerfd (or a<br> > * sound card fd becoming writable, for example in a video player), which<br> > * then triggers GL rendering and eventually eglSwapBuffers().<br> > * eglSwapBuffers() may call wl_display_dispatch_queue() if it didn't<br> > * receive the frame event for the previous frame, and as such queue<br> > - * events in the main queue.<br> > - *<br> > - * \note Calling this makes the current thread the main one.<br> > + * events in the default queue.<br> > *<br> > * \sa wl_display_dispatch(), wl_display_dispatch_queue(),<br> > * wl_display_flush()<br> > @@ -1735,7 +1755,7 @@ wl_proxy_get_class(struct wl_proxy *proxy)<br> > * \param queue The event queue that will handle this proxy<br> > *<br> > * Assign proxy to event queue. Events coming from \c proxy will be<br> > - * queued in \c queue instead of the display's main queue.<br> > + * queued in \c queue instead of the display's default queue.<br> <br> </div></div>Correct up until "instead", because I believe we nowadays have queue<br> inheritance: a new proxy will inherit the queue of the proxy it was<br> created with. Some adjustment to that might be nice here (and verifying<br> that I'm not lying.)<br></blockquote><div><br></div><div style>Yep, proxy_create and wl_create_proxy_for_id functions assings the proxy from factory to the new proxy.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""> > *<br> > * \sa wl_display_dispatch_queue()<br> > *<br> > diff --git a/src/wayland-client.h b/src/wayland-client.h<br> > index dd42d7b..71cd822 100644<br> > --- a/src/wayland-client.h<br> > +++ b/src/wayland-client.h<br> > @@ -71,7 +71,7 @@ struct wl_proxy;<br> > * added to a queue. On the dispatch step, the handler for the incoming<br> > * event set by the client on the corresponding \ref wl_proxy is called.<br> > *<br> > - * A wl_display has at least one event queue, called the <em>main<br> > + * A wl_display has at least one event queue, called the <em>default<br> > * queue</em>. Clients can create additional event queues with \ref<br> > * wl_display_create_queue() and assign \ref wl_proxy's to it. Events<br> > * occurring in a particular proxy are always queued in its assigned queue.<br> > @@ -80,31 +80,27 @@ struct wl_proxy;<br> > * called by assigning that proxy to an event queue and making sure that<br> > * this queue is only dispatched when the assumption holds.<br> <br> </span>(Btw. the above paragraph explains the term "context", even if it<br> doesn't call it that.)<br> <div><div class="h5"><br> > *<br> > - * The main queue is dispatched by calling \ref wl_display_dispatch().<br> > - * This will dispatch any events queued on the main queue and attempt<br> > - * to read from the display fd if its empty. Events read are then queued<br> > - * on the appropriate queues according to the proxy assignment. Calling<br> > - * that function makes the calling thread the <em>main thread</em>.<br> > + * The default queue is dispatched by calling \ref wl_display_dispatch().<br> > + * This will dispatch any events queued on the default queue and attempt<br> > + * to read from the display fd if it's empty. Events read are then queued<br> > + * on the appropriate queues according to the proxy assignment.<br> > *<br> > * A user created queue is dispatched with \ref wl_display_dispatch_queue().<br> > - * If there are no events to dispatch this function will block. If this<br> > - * is called by the main thread, this will attempt to read data from the<br> > - * display fd and queue any events on the appropriate queues. If calling<br> > - * from any other thread, the function will block until the main thread<br> > - * queues an event on the queue being dispatched.<br> > + * This function behaves exactly the same as wl_display_dispatch()<br> > + * but it dispatches given queue instead of the default queue.<br> > *<br> > * A real world example of event queue usage is Mesa's implementation of<br> > * eglSwapBuffers() for the Wayland platform. This function might need<br> > - * to block until a frame callback is received, but dispatching the main<br> > + * to block until a frame callback is received, but dispatching the default<br> > * queue could cause an event handler on the client to start drawing<br> > * again. This problem is solved using another event queue, so that only<br> > * the events handled by the EGL code are dispatched during the block.<br> > *<br> > - * This creates a problem where the main thread dispatches a non-main<br> > + * This creates a problem where a thread dispatches a non-default<br> > * queue, reading all the data from the display fd. If the application<br> > * would call \em poll(2) after that it would block, even though there<br> > - * might be events queued on the main queue. Those events should be<br> > - * dispatched with \ref wl_display_dispatch_pending() before<br> > + * might be events queued on the default queue. Those events should be<br> > + * dispatched with \ref wl_display_dispatch_(queue_)pending() before<br> > * flushing and blocking.<br> > */<br> > struct wl_display;<br> <br> </div></div>Everything I didn't comment about looks good. I really appreciate going<br> through all this.<br> <br> One more thing we perhaps should add: the current threading<br> support/model was first released in Wayland 1.2, right? We might want<br> to add a warning somewhere, that Wayland before 1.2 behaved differently<br> wrt. threads. (another patch, maybe)<br> <br> Another note is that the private display queue was added pretty late in<br> b9eebce0aa5559855d835e403ba3bb5960baaadc which makes it first released<br> in Wayland 1.5. That's probably not important to mention, though.<br> <br> <br> Thanks,<br> pq<br> </blockquote></div><br></div><div class="gmail_extra" style>Thanks!</div><div class="gmail_extra" style>Marek</div></div>