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 694380 - Improve signal argument collection performance
Improve signal argument collection performance
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gobject
unspecified
Other All
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2013-02-21 17:54 UTC by Allison Karlitskaya (desrt)
Modified: 2013-05-24 02:11 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
performance test: share some code (5.23 KB, patch)
2013-02-21 17:54 UTC, Allison Karlitskaya (desrt)
committed Details | Review
performance test: add signal test with args (3.34 KB, patch)
2013-02-21 17:54 UTC, Allison Karlitskaya (desrt)
committed Details | Review
gsignal: remove some pointless locking (2.18 KB, patch)
2013-02-21 17:55 UTC, Allison Karlitskaya (desrt)
committed Details | Review

Description Allison Karlitskaya (desrt) 2013-02-21 17:54:50 UTC
See patchset.
Comment 1 Allison Karlitskaya (desrt) 2013-02-21 17:54:52 UTC
Created attachment 237075 [details] [review]
performance test: share some code

The handled and unhandled cases share the same data and _run()
functions.  Refactor into a common section.
Comment 2 Allison Karlitskaya (desrt) 2013-02-21 17:54:57 UTC
Created attachment 237076 [details] [review]
performance test: add signal test with args

Add a signal that has some typical arguments (a uint and a pointer)
since all of the other signal performance tests are for signals with no
args.
Comment 3 Allison Karlitskaya (desrt) 2013-02-21 17:55:00 UTC
Created attachment 237077 [details] [review]
gsignal: remove some pointless locking

We previously hold a lock in the loop that collects the arguments for
g_signal_emit(), which we drop before calling into the argument
collection functions and reacquire again at the bottom of the loop (ie:
one release/acquire pair for each argument collected).  To make matters
worse, the lock is just released again after the loop.

Presumably that was done to protect the access to the parameter array,
but it's pretty unlikely that this is needed because the only way it
changes is if the signal is unloaded.  That only happens when unloading
types which is quite unlikely to happen while we are emitting on an
instance of that type (and, as an aside, never happens anymore anyway).

If we move the unlock below the loop up above it and remove the
acquire/release pair from the loop, we improve performance in the new
arg-collecting performance tests by ~15% (more like ~18% in the case
where we only emit to one handler -- where argument collection dominates
more).
Comment 4 Alexander Larsson 2013-02-21 17:58:49 UTC
Review of attachment 237075 [details] [review]:

ack
Comment 5 Alexander Larsson 2013-02-21 17:58:54 UTC
Review of attachment 237076 [details] [review]:

ack
Comment 6 Alexander Larsson 2013-02-21 18:05:47 UTC
Review of attachment 237077 [details] [review]:

This looks good to me. 
If i was really picky I'd also move the param_types to the permanent portion of SignalNode in g_signal_newv/signal_destroy_R, but I guess if we're prohibiting type implementation unloading that is not needed anymore?
Comment 7 Allison Karlitskaya (desrt) 2013-02-21 18:20:22 UTC
Review of attachment 237077 [details] [review]:

I don't want to make changes that assume that type modules will never be unloaded.  This new restriction was made provisionally and I want it to be easy to back out.

That said, I think my logic about a type module not being unloaded while emitting a signal on an instance of a type from that module is sound.  Right?
Comment 8 Alexander Larsson 2013-02-21 19:04:55 UTC
Review of attachment 237077 [details] [review]:

Its quite unlikely, but just moving a few mallocs and removing the frees would make it 100% correct.
signal_destroy_R even has some G_ENABLE_DEBUG code that g_criticals if this happens. Don't think i've ever seen that in the wild....
Comment 9 Allison Karlitskaya (desrt) 2013-02-21 22:27:55 UTC
(keeping in mind that my logic here assumes that types can still be unloaded for the reason I mention above)

I don't think that those mallocs can be moved without sacrificing a feature of type unloading: the ability to bring the type back with a signal with a different argument list.

Now that I've said that it sounds really ridiculous... but so is type unloading.
Comment 10 Matthias Clasen 2013-05-24 02:10:50 UTC
Attachment 237075 [details] pushed as 4b72bbf - performance test: share some code
Attachment 237076 [details] pushed as 8bb6a4a - performance test: add signal test with args
Attachment 237077 [details] pushed as 3d1d491 - gsignal: remove some pointless locking