On Thu, 13 Jan 2022 14:05:47 +0200 Maksim Sisov <msi...@igalia.com> wrote:
> On 2022-01-13 11:42, Pekka Paalanen wrote: > > On Thu, 13 Jan 2022 10:05:56 +0200 > > Maksim Sisov <msi...@igalia.com> wrote: > > ... > >> I see. The only only problem here is that we don't use poll, but rather > >> libevent, which is wrapped around Chromium's MessagePumpLibevent. It > >> constantly notifies us that a fd is ready to be read. > > > > You say "constantly", do you mean it never waits? > > > >> Here is a diagram that shows the flow without a dedicated thread - > >> https://drive.google.com/file/d/1Jq8aX4cWszwL5fleE8ayGOeond70xChS/view?usp=sharing > >> . > >> > >> As you can see, it returns from OnCanRead and waits until libevent > >> notifies us the next time that we can actually read events (if prepared > >> to read previously). And if the main thread blocks because of the above > >> mentioned reason (libevent didn't have a chance to notify us), the > >> events will never be read. > > > > Are you describing the old approach or the new approach with the > > separate polling thread? > > I'm describing the old one as having a dedicated thread doesn't really > bring much benefits as I said in my first message. > > > > > Doing prepare_read from OnCanRead handler seems a bit backwards. The > > prepare_read dance is to make sure that when you are going to wait for > > new events (block), you have processed all incoming events so far and > > flushed out all requests that might have resulted from those, so that > > if you are waiting for replies to those requests, they will actually > > arrive. So the prepare_read/dispatch/flush dance should be done from a > > "the event loop is going to sleep now" hook, not from "the fd has bytes > > to read" hook. > > I checked libevent - they added prepare/check watchers in > https://github.com/libevent/libevent/pull/793 in Spring, 2019. Chromium > still uses libevent 1.4.15 > (https://source.chromium.org/chromium/chromium/src/+/main:base/third_party/libevent/README.chromium), > which looks to be old. It seems like we need to make an update that > hasn't been done for ages. Moreover, it seems to be patched :\ I'll need > to check if we can upgrade. For your reference, SardemFF7 (in OFTC IRC) just pointed me to https://github.com/sardemff7/libgwater/blob/master/wayland/libgwater-wayland.c as an example of hooking libwayland-client into glib event loop. An important detail is that "check" (read) and "dispatch" are separate steps as well. Those should help to avoid the below I think. > > Hmm, yeah... I suppose when your event loop decides to do something > > else than actually read the Wayland socket after it was prepared for > > reading, and that something else is a blocking thing, you would have to > > cancel the read first just in case, and then prepare_read again to be > > able to read afterwards. If check only reads, and dispatch is the step that might e.g. tear down other threads synchronously, you shouldn't need to cancel_read in order to do synchronous things between threads. Also the time it takes from Wayland socket becoming readable to actually calling read_events should be minimized, because other threads might have done prepare_read, have been woken up, and are now waiting for the last reader to enter the read_events. Not doing long operations in the check/read step should help with that too. Thanks, pq
pgpcejpJmKi1q.pgp
Description: OpenPGP digital signature