GNOME Bugzilla – Bug 696155
Optimize g_irepository_find_by_gtype
Last modified: 2015-02-07 16:58:11 UTC
Created attachment 239294 [details] [review] patch While profiling Sugar startup (uses pygi) I found that when g_irepository_find_by_gtype() is forced to perform an exhaustive search, a considerable amount of CPU time is consumed. Here is a patch to optimize this function.
This patch sounds good on the surface, but there are some corner cases here, and so I'd like to wait until after GNOME 3.8 before applying this. At the moment g-i isn't branched.
For the record, pygobject's tests still work fine with this applied. It uses g_irepository_find_by_gtype() in several places which all have fairly decent test coverage.
Review of attachment 239294 [details] [review]: Part 1) roughly makes sense. I'm more skeptical of 2. For example, there's Gdk-3.0.gir and GdkPixbuf-2.0.gir, both with c:prefix Gdk. We need to search both for GdkSomeObject. With your patch, if we happen to search GdkPixbuf first, we'll error out early. Right? We could use some test cases for this...
(In reply to comment #3) > Review of attachment 239294 [details] [review]: > > Part 1) roughly makes sense. > > I'm more skeptical of 2. For example, there's Gdk-3.0.gir and > GdkPixbuf-2.0.gir, both with c:prefix Gdk. We need to search both for > GdkSomeObject. With your patch, if we happen to search GdkPixbuf first, we'll > error out early. Right? No, it will still search both as before. However, after finding that GdkSomeObject is not present in GdkPixbuf nor Gdk (both of which claimed support for the Gdk prefix), it won't bother doing an (expensive) exhaustive search, since it already found one or more typelibs claiming support for the prefix I think we can assume that we won't find it elsewhere.
(In reply to comment #4) > > > No, it will still search both as before. Ok, I played around with this some more, and yes, it makes sense. One comment: * I really dislike code doing: g_strsplit() iterate g_strfreev() since there's a lot of banging on malloc(). I'll attach a string iterator patch on top of this (Ideally this might be in GLib, needs more thought).
Created attachment 240896 [details] [review] only malloc once Does this further patch on top of yours look good?
It does. I've given it a quick test as well, doesn't seem to introduce any problems. Thanks!
Sorry, I made a mistake in my testing - forgot to apply the patch. Actually this does cause a segfault. Some typelibs don't specify a prefix, and the strsplit stuff is not 100% happy about that. I imagine you can easily reproduce this with something like: c_prefix = ""; strsplit_iter_init (&split_iter, c_prefix, ","); while (strsplit_iter_next (&split_iter, &prefix)) { size_t len = strlen (prefix); } When passed a c_prefix of "", strsplit_iter_next returns true, and we enter the loop with a prefix of NULL. strlen(NULL) = segfault
(In reply to comment #8) > Sorry, I made a mistake in my testing Yeah, me too, I should have tried running a full VM instead of just the gjs tests. Fixing now.
Should be fixed by: https://git.gnome.org/browse/gobject-introspection/commit/?id=d03bbfa52c328f2250b40777077b464e6978d47f
Thanks, seems to be working now.
Potential fallout: https://bugzilla.gnome.org/show_bug.cgi?id=697759
[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]