[pulseaudio-discuss] [PATCH RFCv3 25/51] mainloop: Clear wakeup pipe only when necessary

[Peter Meerwald]

> > when the wakeup pipe became ready, and poll() returned
> > just one descriptor, we can stop scanning io_events
> 
> Eh, does this really make things faster?

probably not in a measureable way;

I think the patch makes the codes more logical: 
* if we know that no io_event triggered, there is no point in scanning 
them (I had 23 io_events for a simple PA setup, not sure where they are 
all coming from)
* if we know that the wakeup pipe triggered, we clear it (currently the 
wakeup pipe is cleared always, no matter what; this should save a read() 
when nothing is in the wakeup pipe)

> Right now it seems to add another syscall - with the patch, we call 
> clear_wakeup -> pa_read -> read syscall twice per iteration, once from 
> dispatch_pollfds and once from pa_mainloop_prepare.

I don't think so; the patch remove the clearing from prepare and only does 
it in dispatch_pollfds when necessary
 
> In addition, I don't see anything particular time consuming in looping 
> through a few io_events.

probably, yet it's pointless and doesn't convey the idea of the polling 
framework
 
> Replacing the wakeup pipe with a pa_fdsem would be an interesting 
> optimisation though, as pa_fdsem has a few atomic operations to avoid 
> the entire write-poll-read cycle if no thread is currently waiting for 
> the fdsem.
 
though about it as well; I micro-benchmarked pipe() vs. eventfd()
write-poll-read and there was only a slight benefit for eventfd

since the fdsem would be used in poll, there would be almost-always
someone waiting?

regards, p.

> > Signed-off-by: Peter Meerwald <pmeerw at pmeerw.net>
> > ---
> >   src/pulse/mainloop.c | 24 ++++++++++++++----------
> >   1 file changed, 14 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/pulse/mainloop.c b/src/pulse/mainloop.c
> > index c7a5236..a7a3c48 100644
> > --- a/src/pulse/mainloop.c
> > +++ b/src/pulse/mainloop.c
> > @@ -635,6 +635,15 @@ static void rebuild_pollfds(pa_mainloop *m) {
> >       m->rebuild_pollfds = false;
> >   }
> >
> > +static void clear_wakeup(pa_mainloop *m) {
> > +    char c[10];
> > +
> > +    pa_assert(m);
> > +
> > +    while (pa_read(m->wakeup_pipe[0], &c, sizeof(c), &m->wakeup_pipe_type)
> == sizeof(c))
> > +        ;
> > +}
> > +
> >   static unsigned dispatch_pollfds(pa_mainloop *m) {
> >       pa_io_event *e;
> >       unsigned r = 0, k;
> > @@ -642,6 +651,11 @@ static unsigned dispatch_pollfds(pa_mainloop *m) {
> >       pa_assert(m->poll_func_ret > 0);
> >
> >       k = m->poll_func_ret;
> > +    if (m->pollfds[0].revents) {
> > +        clear_wakeup(m);
> > +        m->pollfds[0].revents = 0;
> > +        k--;
> > +    }
> >
> >       PA_LLIST_FOREACH(e, m->io_events) {
> >
> > @@ -775,20 +789,10 @@ void pa_mainloop_wakeup(pa_mainloop *m) {
> >           pa_log("pa_write() failed while trying to wake up the mainloop:
> %s", pa_cstrerror(errno));
> >   }
> >
> > -static void clear_wakeup(pa_mainloop *m) {
> > -    char c[10];
> > -
> > -    pa_assert(m);
> > -
> > -    while (pa_read(m->wakeup_pipe[0], &c, sizeof(c), &m->wakeup_pipe_type)
> == sizeof(c))
> > -        ;
> > -}
> > -
> >   int pa_mainloop_prepare(pa_mainloop *m, int timeout) {
> >       pa_assert(m);
> >       pa_assert(m->state == STATE_PASSIVE);
> >
> > -    clear_wakeup(m);
> >       scan_dead(m);
> >
> >       if (m->quit)
> >
> 
> 

-- 

Peter Meerwald
+43-664-2444418 (mobile)