GNOME Bugzilla – Bug 730984
Faster instance type check for fundamentals
Last modified: 2014-05-31 13:48:47 UTC
When checking whether an instance type is of a given fundamental (such as G_TYPE_OBJECT), we can avoid several code path. The following patches add a new g_type_check_instance_is_a_fundamental() which works the same way as g_type_check_instance_is_a() except that it is only meant to be used against fundamentals. Amongst the various code path it speeds up, it makes g_object_unref/g_object_ref 40 to 50% faster, while still doing type checking
Created attachment 277520 [details] [review] gtype: Add check for fundamental instance type When checking whether an instance is of a given fundamental type (such as G_TYPE_OBJECT), we can avoid over 50% of the cost of checking types.
Created attachment 277521 [details] [review] gobject: Use fast fundamental instance type check Speeds up g_object_ref/_unref by 50% (i.e. takes 25% less time).
Review of attachment 277520 [details] [review]: This is a fantastic idea. Thanks for looking into this! Some concerns below with the specific approach. ::: gobject/gtype.c @@ +3979,3 @@ + + /* Checks already done in header file */ +#ifndef __GNUC__ No guarantee that the compiler for GLib is the one in use at the time the code using GLib is being built... @@ +3985,3 @@ + + node = lookup_type_node_I (type_instance->g_class->g_type); + if (G_LIKELY (node)) Probably better to just rewrite this as returning the result of the comparison directly. The actual branch taken in response to this comparison is in a different compilation unit (probably a different library, in fact) so the hinting won't help anyway... ::: gobject/gtype.h @@ +1943,3 @@ __r; \ })) +# define _G_TYPE_CITF(ip, gt) (G_GNUC_EXTENSION ({ \ I don't like all of the extra checks here. It makes the implementation more complex, and we'll only speed up the "error" case anyway (except for one exception which I deal separately with below). @@ +1948,3 @@ + __r = FALSE; \ + else if (__inst->g_class->g_type == __t) \ + __r = TRUE; \ This check is particular is a bit silly -- it adds one more check (slowing down the general case), and it's almost never the case that we will be calling this on a GObject itself.
Review of attachment 277521 [details] [review]: If the test takes 75% as long then performance improvement is 33%... ;) This one makes a good deal of sense after the other one, clearly.
Review of attachment 277520 [details] [review]: ::: gobject/gtype.c @@ +3979,3 @@ + + /* Checks already done in header file */ +#ifndef __GNUC__ Indeed @@ +3985,3 @@ + + node = lookup_type_node_I (type_instance->g_class->g_type); + if (G_LIKELY (node)) so we're guaranteed to always have a node ? ::: gobject/gtype.h @@ +1948,3 @@ + __r = FALSE; \ + else if (__inst->g_class->g_type == __t) \ + __r = TRUE; \ yah, realized it after pushing. *facepalm*
Review of attachment 277520 [details] [review]: ::: gobject/gtype.c @@ +3985,3 @@ + + node = lookup_type_node_I (type_instance->g_class->g_type); + if (G_LIKELY (node)) Sorry. I misread this. fwiw, though, the "lookup" isn't really a lookup, but rather some math. Non-fundamental GTypes are really just mangled pointers. static inline TypeNode* lookup_type_node_I (register GType utype) { if (utype > G_TYPE_FUNDAMENTAL_MAX) return (TypeNode*) (utype & ~TYPE_ID_MASK); else return static_fundamental_type_nodes[utype >> G_TYPE_FUNDAMENTAL_SHIFT]; } Since we take the g_type out of an instance, the only way we can crash is if we give an invalid instance... which would probably almost always crash anyway (excepting that the value we read is 0, which is probably a _somewhat_ common case when dealing with non-instances).
Created attachment 277525 [details] [review] gtype: Add check for fundamental instance type When checking whether an instance is of a given fundamental type (such as G_TYPE_OBJECT), we can avoid over 60%+ of the cost of checking types.
Created attachment 277526 [details] [review] gobject: Use fast fundamental instance type check Speeds up g_object_ref/_unref by 50%-65% (i.e. takes 60-65% of the time it used to take).
Even faster this time :)
Created attachment 277530 [details] [review] gtype: Add check for fundamental instance type When checking whether an instance is of a given fundamental type (such as G_TYPE_OBJECT), we can avoid over 60%+ of the cost of checking types.
Created attachment 277531 [details] [review] gobject: Use fast fundamental instance type check Speeds up g_object_ref/_unref by 50%-65% (i.e. takes 60-65% of the time it used to take).
Created attachment 277532 [details] [review] GParamSpec: Use new fundamental instance check
Review of attachment 277530 [details] [review]: ::: gobject/gtype.h @@ +1891,3 @@ GType iface_type) G_GNUC_PURE; GLIB_AVAILABLE_IN_ALL +gboolean g_type_check_instance_is_a_fundamental (GTypeInstance *instance, We miss the addition to the docs for this function. Also: please tag it with the correct version tag. This is not "available in all". Finally, the name is a bit weird. Looking at the name of this function, I would expect that it tells me if the instance that I pass it has a type that is exactly equal to a fundamental type. The best other names I can think of: g_type_check_instance_is_a_subtype_of_fundamental or g_type_check_instance_is_a_for_fundamental too bad we can't put parens in function names :)
(In reply to comment #13) > Review of attachment 277530 [details] [review]: > > ::: gobject/gtype.h > @@ +1891,3 @@ > GType iface_type) G_GNUC_PURE; > GLIB_AVAILABLE_IN_ALL > +gboolean g_type_check_instance_is_a_fundamental (GTypeInstance > *instance, > > We miss the addition to the docs for this function. > > Also: please tag it with the correct version tag. This is not "available in > all". Ok, I'll use GLIB_AVAILABLE_IN_2_42 > > Finally, the name is a bit weird. Looking at the name of this function, I > would expect that it tells me if the instance that I pass it has a type that is > exactly equal to a fundamental type. > > The best other names I can think of: > > g_type_check_instance_is_a_subtype_of_fundamental It'll be a bit ugly/long, but I'll use that one :)
Created attachment 277539 [details] [review] gtype: Add check for fundamental instance type When checking whether an instance is of a given fundamental type (such as G_TYPE_OBJECT), we can avoid over 60%+ of the cost of checking types.
Created attachment 277540 [details] [review] gobject: Use fast fundamental instance type check Speeds up g_object_ref/_unref by 50%-65% (i.e. takes 60-65% of the time it used to take).
Created attachment 277541 [details] [review] GParamSpec: Use new fundamental instance check
how about g_type_check_instance_is_fundamentally_a ?
Attachment 277539 [details] pushed as 6072e36 - gtype: Add check for fundamental instance type Attachment 277540 [details] pushed as faceb89 - gobject: Use fast fundamental instance type check Attachment 277541 [details] pushed as f8595a4 - GParamSpec: Use new fundamental instance check Instead of going around a few more times, I've committed the patches with the following changes: - function renamed to g_type_check_instance_is_fundamentally_a() - macro renamed to _FUNDAMENTAL_TYPE instead of _TYPE_FUNDAMENTAL - some small whitespace fixes - docs section additions Thanks very much for doing the research to find this issue and taking the time to come up with this solution.