Created attachment 278820 [details] [review]
gvalue: New g_value_from_instance

Used for the commonly used case (in signal emission) where we
initialize and set a GValue for an instance

Includes a fast-path for GObject

Overall makes it 6 times faster than the previous combination
of g_value_init + g_value_set_instance

Makes signal emission around 10% faster

*** Bug 605666 has been marked as a duplicate of this bug. ***

Sorry about the name thing again, but I would expect to see init_from_instance() in this case...

Created attachment 278974 [details] [review]
gvalue: New g_value_init_from_instance

Used for the commonly used case (in signal emission) where we
initialize and set a GValue for an instance

Includes a fast-path for GObject

Overall makes it 6 times faster than the previous combination
of g_value_init + g_value_set_instance

Makes signal emission around 10% faster

Review of attachment 278974 [details] [review]:

in general, it looks good to me - except for a minor doc issue, and for an annotation/behaviour one.

::: gobject/gvalue.c
@@ +377,3 @@
+ * g_value_init_from_instance:
+ * @value: An uninitialized #GValue structure.
+ * @instance: the instance

I wonder if we should annotate this gpointer as `(type GObject)` for introspection-based bindings, or as `(type GTypeInstance)`. probably the former, but we'll have to check we the bindings/introspection developers.

now that I think of it, can the `instance` ever be `NULL`? if it can, then we'll need a `(nullable)` annotation, and possibly a fast path. if it can't be nullable, then we'll need a check.

@@ +381,3 @@
+ * Initializes and sets @value from an instantiatable type via the
+ * value_table's collect_value() function.
+ */

missing `Since: 2.42` annotation.

Review of attachment 278974 [details] [review]:

::: gobject/gvalue.c
@@ +377,3 @@
+ * g_value_init_from_instance:
+ * @value: An uninitialized #GValue structure.
+ * @instance: the instance

allowing NULL for instance makes no sense whatsoever. What GType do you use if it's NULL ??

The check for it being non-NULL is present (first line)

Created attachment 278993 [details] [review]
gvalue: New g_value_init_from_instance

Used for the commonly used case (in signal emission) where we
initialize and set a GValue for an instance

Includes a fast-path for GObject

Overall makes it 6 times faster than the previous combination
of g_value_init + g_value_set_instance

Makes signal emission around 10% faster

(In reply to comment #6)
> allowing NULL for instance makes no sense whatsoever. What GType do you use if
> it's NULL ??

This is actually a bit of an interesting point...


If the signature of the signal or property (or whatever we use this GValue for) says that we expect (for example) a GObject and we pass through a GtkLabel, then should the type of the GValue be GObject or GtkLabel?

The old signal code used the type of the passed-in instance that the signal was being emitted on, of course, which is a bit odd...  One might expect that the type of the container was actually the type that the signal was defined on...  Not that it would really matter for this case since we just pass the thing onto the marshal code...

If we added a type parameter, though, we'd probably want to do a typecheck and then we'd be back into (some of) the same bad performance situation that we were with separate functions anyway...


I think we can probably resolve this with some small documentation improvements.  The new function should advertise:

 - the created GValue is of exactly the type of the passed-in instance

 - if you want to create a GValue that could be set to a different type
   in the future (including a supertype of the passed-in instance) or if you
   would like to create a GValue containing NULL then you must use _init
   (and separately, _set_instance, if appropriate).

Good point indeed.

I was only trying to optimize the usage of these 2 lines:
g_value_init(&value, G_TYPE_FROM_INSTANCE(something));
g_value_set_instance(&value, something);

In regards to that GValue usage ... does it matter whether it's setting/using the *exact* instance GType or a parent GType ? That GValue would still be compatible with the signature (assuming checks have been done *before* calling this function of course).

Take the signal emission code for example (and this is the current behaviour, which I've only compressed):
* You have a signal with a certain instance type signature (say G_TYPE_OBJECT)
* g_signal_emit* checks whether the instance type is compatible with the signal's instance type signature (g_is_a (G_TYPE_PONY, G_TYPE_OBJECT))
* The instance GValue is created with G_TYPE_PONY
* The signal is emitted
* callbacks expect G_TYPE_OBJECT ... and that GValue is indeed of that type (G_IS_OBJECT(G_VALUE_TYPE(value)) will succeed).

optimizing the other case (where you want to use a *specific* GType for that value) is sped up a bit by the other round of GValue optimization (bug #732084 ).

I'll update the documentation accordingly to make it clear what the consequences of this are.

Created attachment 279085 [details] [review]
gvalue: New g_value_init_from_instance

Used for the commonly used case (in signal emission) where we
initialize and set a GValue for an instance

Includes a fast-path for GObject

Overall makes it 6 times faster than the previous combination
of g_value_init + g_value_set_instance

Makes signal emission around 10% faster

(In reply to comment #9)
> In regards to that GValue usage ... does it matter whether it's setting/using
> the *exact* instance GType or a parent GType ? That GValue would still be
> compatible with the signature (assuming checks have been done *before* calling
> this function of course).

My concern is for someone replacing this code:



g_value_init (&v, G_TYPE_OBJECT);
g_value_set_instance (&v, obj1);
...
g_value_set_instance (&v, obj2);

with this:

g_value_init_from_value (&v, obj1);
...
g_value_set_instance (&v, obj2);



ie: reusing the GValue later for a new value with a compatible (but not exactly equal) type.  If obj2 is a subtype of obj1 then this will work, but otherwise not.  That's strangely side-effecty.

For the cases we intend to use it for, it's completely fine, however.  We will not be reusing these GValues so there's absolutely no problem.

With your documentation changes, I think we're fine.

Attachment 279085 [details] pushed as c5c3c32 - gvalue: New g_value_init_from_instance


Pushed with the following minor changes:

 - removed the G_LIKELY() around G_IS_OBJECT() -- we typically only use this
   for these situations:

     - the "unlikely" case only happens once per program run

     - the "unlikely" case is highly expensive to handle anyway, so the
       cost of the failed branch prediction is very small by comparison

     - the "unlikely" case is an error that will result in at least a
       g_warning()

 - slightly reworded the docs for clarity

 - added entry in gobject-sections.txt