After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 320888 - optimization for g_main_context_wakeup
optimization for g_main_context_wakeup
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: mainloop
2.8.x
Other Linux
: Normal normal
: ---
Assigned To: Allison Karlitskaya (desrt)
gtkdev
Depends on:
Blocks:
 
 
Reported: 2005-11-07 15:54 UTC by Wim Taymans
Modified: 2011-09-10 02:32 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch to optimize g_main_context_wakeup (2.12 KB, patch)
2005-11-07 15:56 UTC, Wim Taymans
none Details | Review
patch with updated docs (4.20 KB, patch)
2006-09-06 17:07 UTC, Benjamin Otte (Company)
rejected Details | Review
gmain: get rid of poll_waiting (4.37 KB, patch)
2011-09-10 01:48 UTC, Allison Karlitskaya (desrt)
committed Details | Review

Description Wim Taymans 2005-11-07 15:54:47 UTC
g_main_context_wakeup acquires the CONTEXT_LOCK of the GMainContext. The lock is
taken to protect a boolean, which can also be updated atomically. 

In combination with bug #320886, where the lock can potentially be taken for a
long time in garbage collected languages, performance improvements are
substancial making the _wakeup() atomic.
Comment 1 Wim Taymans 2005-11-07 15:56:14 UTC
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.
Comment 2 Matthias Clasen 2005-12-02 21:43:46 UTC
So I don't really feel qualified to judge the merits of this. Do we have a
benchmark to measure the alleged performance improvements ?
Comment 3 Wim Taymans 2005-12-05 16:29:15 UTC
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. 
Comment 4 Benjamin Otte (Company) 2006-09-06 17:07:28 UTC
Created attachment 72322 [details] [review]
patch with updated docs
Comment 5 Benjamin Otte (Company) 2006-09-06 17:15:47 UTC
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).
Comment 6 Sebastian Wilhelmi 2006-09-06 18:42:55 UTC
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.
Comment 7 Benjamin Otte (Company) 2006-09-07 11:56:37 UTC
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?
Comment 8 Owen Taylor 2006-09-07 12:37:07 UTC
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().
Comment 9 Gustavo Carneiro 2006-09-08 16:37:04 UTC
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?
Comment 10 Matthias Clasen 2006-09-11 03:10:21 UTC
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.
Comment 11 Allison Karlitskaya (desrt) 2011-09-10 00:44:49 UTC
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).
Comment 12 Allison Karlitskaya (desrt) 2011-09-10 01:48:23 UTC
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
Comment 13 Matthias Clasen 2011-09-10 02:21:54 UTC
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.