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

[David Henningsson]

On 2014-11-05 12:01, Peter Meerwald wrote:
>
>>> 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;

Hmm, but yet you listed this one as one of your "critical patches"...?

> 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

Sorry, I missed that you removed clear_wakeup from prepare.

>
>> 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?

So, consider the case where someone calls pa_mainloop_wakeup when it 
does not need to be woken up.

In that case, doing clear_wakeup as late as possible is preferable, 
because then you don't have to poll just to wakeup because the pipe is 
readable.

In addition, pa_fdsem would deal with this situation by not doing 
anything at all on the writer side, because it knows noone is waiting on 
the reader side.

To sum up, it seems to me like pa_fdsem is the best tool for the job. 
But if you don't want to consider that, how about setting a variable in 
dispatch_pollfds that indicates that the pipe should be cleared in 
pa_mainloop_prepare?

>
> 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)
>>>
>>
>>
>

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic