GNOME Bugzilla – Bug 603590
Speed up G_VALUE_COLLECT
Last modified: 2010-01-18 15:22:43 UTC
G_VALUE_COLLECT is being used extensively when creating new GObject and emitting signals. The problem is that the current usage goes as such: g_value_init => calls g_type_value_table_peek() => initializes memory => calls ->value_init G_VALUE_COLLECT => calls g_type_value_table_peek() AGAIN => if present, frees the memory (wait, it's BLANK !) => initializes memory again The following patches create a new G_VALUE_COLLECT_FULL variant which initializes/sets the GValue in one go. This brings a 15-20% speedup to g_object_new_valist and g_signal_emit_valist
Created attachment 148901 [details] [review] gvaluecollector: Add variant of G_VALUE_COLLECT for most used cases. Most callers of G_VALUE_COLLECT previously had to initialize the GValue and then G_VALUE_COLLECT would still go through a cleanup phase. The new variant allows passing a unitialized GValue along with a GType and speedup the initialization/collection process.
Created attachment 148902 [details] [review] gsignal: Use new G_VALUE_COLLECT_FULL variant Makes g_signal_emit_valist from 15% to 20% faster. Results reported from profiling the pan newsreader which uses a variant of simple and complex signal emissions (i.e no args or various args)
Created attachment 148903 [details] [review] gobject: Use new G_VALUE_COLLECT_FULL variant Makes g_object_new_valist 20% to 30% faster (against 2321e5a). Profiled against the pan newsreader which uses a variant of simple and complex object creation.
I like this patch. Some comments about it (mostly from discussing on IRC): - G_VALUE_COLLECT_FULL() is a bad name. I don't know a better one though. Maybe G_VALUE_COLLECT_TYPE()? - Your docs say the value must be empty. If it has to be empty, we don't need to do the memset manually. We only need to do it if we say that value may point to random memory. In those cases, we don't need to pre-initialize the values in gsignal.c/gobject.c though. I have no strong opinion either way, but I think I'd prefer keeping the docs and moving the memset to G_VALUE_COLLECT(). - Edward mentioned on IRC that the performance metrics come from using valgrind counting instructions. Just thought I should mention that here.
The name is quite bad, xxx_FULL in glib apis generally means it takes more arguments than the non-full version and otherwise have the same behaviour. However, the behaviour here is different. What about combining init and collect, like G_VALUE_COLLECT_INIT()?
Created attachment 150496 [details] [review] gvaluecollector: Add variant of G_VALUE_COLLECT for most used cases. Most callers of G_VALUE_COLLECT previously had to initialize the GValue and then G_VALUE_COLLECT would still go through a cleanup phase. The new variant allows passing a unitialized GValue along with a GType and speedup the initialization/collection process.
Created attachment 150497 [details] [review] gvaluecollector: Add variant of G_VALUE_COLLECT for most used cases. Most callers of G_VALUE_COLLECT previously had to initialize the GValue and then G_VALUE_COLLECT would still go through a cleanup phase. The new variant allows passing a unitialized GValue along with a GType and speedup the initialization/collection process.
Created attachment 150498 [details] [review] gsignal: Use new G_VALUE_COLLECT_INIT variant Makes g_signal_emit_valist from 15% to 20% faster. Results reported from profiling the pan newsreader which uses a variant of simple and complex signal emissions (i.e no args or various args)
Created attachment 150499 [details] [review] gobject: Use new G_VALUE_COLLECT_INIT variant Makes g_object_new_valist 20% to 30% faster (against 2321e5a). Profiled against the pan newsreader which uses a variant of simple and complex object creation.
Changes of the above 3 patches: * renamed to G_VALUE_COLLECT_INIT * move the meminit to G_VALUE_COLLECT Comments welcome.
Review of attachment 150497 [details] [review]: ::: gobject/gvaluecollector.h @@ +83,3 @@ * va_list variables cannot be passed by reference. + * + * Since: 2.23.1 Generally we put since 2.24 (ie next major release) @@ +141,3 @@ + * + * Note: If you are creating the @value argument just before calling this macro, + * you should use the #G_VALUE_COLLECT_FULL variant and pass the unitialized This references the old name of the macro
Otherwise this looks good to me
G_VALUE_INIT_AND_COLLECT was also proposed as a name, not sure which is best.
I fixed the things i commented on above and commited. This seems obviously safe and better, so no need to keep it waiting.
This patch introduces a bug in GStreamer, see https://bugzilla.gnome.org/show_bug.cgi?id=607283
*** Bug 607283 has been marked as a duplicate of this bug. ***
Problem might be, that in commit 0f25115f the GValues are not really initialized but are only allocated by g_slice_alloc(), i.e. they contain random data.
Gah, the value_init docs say that the value will be memset to zeros, so I guess we should keep that guarantee. Still, those same docs also say no care must be taken to remove any old values, so I'd suggest making gst_mini_object_collect() not try to delete a nonexisting old value by using _replace().
Yeah, I just noticed the same. I'll fix the mini object collect function
Created attachment 151666 [details] [review] Ensure values are memset to 0 when calling G_VALUE_COLLECT_INIT() The reason we need to enforce this is that the GTypeValueTable documentation explicitly states that memory is memset to 0 when the value_init function is called.
*** Bug 607293 has been marked as a duplicate of this bug. ***
Comment on attachment 151666 [details] [review] Ensure values are memset to 0 when calling G_VALUE_COLLECT_INIT() Please commit
Comment on attachment 151666 [details] [review] Ensure values are memset to 0 when calling G_VALUE_COLLECT_INIT() landed as 914120b9701650ccf7bede1907b69b521ca43236