On 28 November 2014 at 14:30, Pekka Paalanen <ppaala...@gmail.com> wrote:
> On Fri, 28 Nov 2014 12:18:55 +0100 > Marek Chalupa <mchqwe...@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 <mchqwe...@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
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel