GNOME Bugzilla – Bug 694380
Improve signal argument collection performance
Last modified: 2013-05-24 02:11:01 UTC
See patchset.
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.
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.
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).
Review of attachment 237075 [details] [review]: ack
Review of attachment 237076 [details] [review]: ack
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?
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?
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....
(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.
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