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 <[email protected]>
---
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
_______________________________________________
pulseaudio-discuss mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
