After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 696155 - Optimize g_irepository_find_by_gtype
Optimize g_irepository_find_by_gtype
Status: RESOLVED FIXED
Product: gobject-introspection
Classification: Platform
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gobject-introspection Maintainer(s)
gobject-introspection Maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2013-03-19 18:22 UTC by Daniel Drake
Modified: 2015-02-07 16:58 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch (11.03 KB, patch)
2013-03-19 18:22 UTC, Daniel Drake
reviewed Details | Review
only malloc once (3.04 KB, patch)
2013-04-07 17:39 UTC, Colin Walters
none Details | Review

Description Daniel Drake 2013-03-19 18:22:48 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.
Comment 1 Colin Walters 2013-03-21 21:57:51 UTC
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.
Comment 2 Martin Pitt 2013-03-22 10:56:39 UTC
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.
Comment 3 Colin Walters 2013-04-03 03:23:29 UTC
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...
Comment 4 Daniel Drake 2013-04-04 01:47:25 UTC
(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.
Comment 5 Colin Walters 2013-04-07 17:33:09 UTC
(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).
Comment 6 Colin Walters 2013-04-07 17:39:19 UTC
Created attachment 240896 [details] [review]
only malloc once

Does this further patch on top of yours look good?
Comment 7 Daniel Drake 2013-04-08 14:52:21 UTC
It does. I've given it a quick test as well, doesn't seem to introduce any problems. Thanks!
Comment 8 Daniel Drake 2013-04-08 19:21:32 UTC
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
Comment 9 Colin Walters 2013-04-08 19:32:57 UTC
(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.
Comment 11 Daniel Drake 2013-04-08 20:49:26 UTC
Thanks, seems to be working now.
Comment 12 Colin Walters 2013-04-11 08:58:42 UTC
Potential fallout:

https://bugzilla.gnome.org/show_bug.cgi?id=697759
Comment 13 André Klapper 2015-02-07 16:58:11 UTC
[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]