Created attachment 54430 [details] [review]
patch to optimize g_main_context_wakeup

this patch optimizes g_main_context_wakeup() by making the poll_waiting
variable atomic.

So I don't really feel qualified to judge the merits of this. Do we have a
benchmark to measure the alleged performance improvements ?

This patch was created as an attempt to fix the mainloop locks being held too
long, caused by bug #320886. The patch is valid on its own but probably does not
add any measurable performance improvements now that the main bug is fixed. Can
be closed if nobody cares. 

Created attachment 72322 [details] [review]
patch with updated docs

This is basically the same patch, just with updated docs. I came up with it while wanting to wakeup the main context from a UNIX signal handler, see bug 354642 for details.

I'm pretty sure this fix is safe although I have no way to test it (as always with threads).

Note, that all the g_atomic functions _might_ use locks, so they are _not_ allowed to be called in a signal handler, which makes the whole point of this excerise a bit moot, doesn't it. Also it slows down things.

Hrm, that's correct.
That still leaves me with the question:
What is the suggested glib way to wake up a main context from a signal handler?

In a single-threaded app you generally won't need to wake up the
main loop at all; the signal will do it for you. (I don't think it's
even possible to get restartable behavior for poll(), but certainly
it isn't the default.)

In a multi-threaded app, the simplest thing to do is to write a byte
down a pipe - see the handling of child watches in gmain.c. (We originally 
were going to child-watch functionality in GLib a generic signal handler 
watch, but it turned out to be close to impossible to come up with
a general API that was robust and made sense. Child exit is easier because
we know the entire relationship between the signals and waitpid())

The other thing you could do in a multi-threaded app is to block your
signal in all but one thread and use sigsuspend in that thread - if managed
properly that can basically give you a "synchronous" signal handler where
you could use normal threaded mechanisms like mutexes, g_idle_add(),
or g_main_context_wakeup().

May I ask why poll_waiting is even needed.  Shouldn't writing a byte to the pipe, like it already does, be enough to wake up the main context?

IIRC poll_waiting is used to ensure that we are not writing more in the pipe than we are reading from it, since that would risk blocking the write() when the pipe fills up. 

That being said, my reading of SUS on write and pipe seems to indicate that
we should be able to prevent blocking in write by doing

fcntl (fd, F_SETFL, O_NONBLOCK) 

on the pipe.

GWakeup is non-blocking, so you can signal() infinitely many times (and without penalty in the case of using an eventfd, since it's only one read() call to clear it again).

Created attachment 196157 [details] [review]
gmain: get rid of poll_waiting

This variable, which is the cause of much grief, exists for two reasons:

  - ensuring the the wakeup pipe doesn't fill up

  - preventing the first poll() after adding a source from waking up
    immediately

The first point is no longer an issue with GWakeup.

The second point is addressed by using different logic: we only signal a
wakeup in the case that the context is currently acquired by a thread
that is not us.

As an added bonus, we can now implement g_main_context_wakeup() without
taking a lock.

https://bugzilla.gnome.org/show_bug.cgi?id=583511
https://bugzilla.gnome.org/show_bug.cgi?id=320888

Review of attachment 196157 [details] [review]:

This looks like a nice cleanup.

::: glib/gmain.c
@@ +3604,1 @@
 {

I guess g_main_context_wakeup should be documented as async-signal-safe now ?

@@ +3608,1 @@
   g_return_if_fail (g_atomic_int_get (&context->ref_count) > 0);

As an aside, I must say I don't really like these ref_count assertions that we have in some places.