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 696143 - Optimize property and signal access
Optimize property and signal access
Status: RESOLVED FIXED
Product: pygobject
Classification: Bindings
Component: gobject
unspecified
Other Linux
: Normal normal
: GNOME 3.10
Assigned To: Nobody's working on this now (help wanted and appreciated)
Python bindings maintainers
Depends on:
Blocks:
 
 
Reported: 2013-03-19 14:54 UTC by Daniel Drake
Modified: 2013-04-18 14:49 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
0001-Simplify-registration-of-custom-types.patch (8.28 KB, patch)
2013-03-19 14:54 UTC, Daniel Drake
needs-work Details | Review
0002-Optimize-property-get-set.patch (3.54 KB, patch)
2013-03-19 14:55 UTC, Daniel Drake
needs-work Details | Review
0003-Fix-setting-of-struct-property-values.patch (1.15 KB, patch)
2013-03-19 14:56 UTC, Daniel Drake
committed Details | Review
0004-Consolidate-signal-connection-code.patch (7.08 KB, patch)
2013-03-19 14:56 UTC, Daniel Drake
needs-work Details | Review
0005-Optimize-connection-of-Python-implemented-signals.patch (2.34 KB, patch)
2013-03-19 14:57 UTC, Daniel Drake
committed Details | Review
0006-Optimize-signal-lookup-in-gi-repository.patch (6.39 KB, patch)
2013-03-19 14:57 UTC, Daniel Drake
committed Details | Review
0001-Simplify-registration-of-custom-types.patch (8.53 KB, patch)
2013-04-12 16:18 UTC, Daniel Drake
committed Details | Review
0002-Optimize-property-get-set.patch (3.14 KB, patch)
2013-04-16 19:55 UTC, Daniel Drake
committed Details | Review
0004-Consolidate-signal-connection-code.patch (8.43 KB, patch)
2013-04-17 17:47 UTC, Daniel Drake
committed Details | Review

Description Daniel Drake 2013-03-19 14:54:54 UTC
Created attachment 239258 [details] [review]
0001-Simplify-registration-of-custom-types.patch

After porting Sugar from pygtk2+GTK2 to pygi+GTK3 we are seeing a regression in startup time, so I profiled it.

The main CPU-eater is libgobject-introspection, but upon investigating further, it is due to pygobject calling into it somewhat excessively for connection of signals and handling of properties.

These patches optimize those codepaths and cause Sugar to startup 3-5 seconds faster.
Comment 1 Daniel Drake 2013-03-19 14:55:50 UTC
Created attachment 239259 [details] [review]
0002-Optimize-property-get-set.patch
Comment 2 Daniel Drake 2013-03-19 14:56:29 UTC
Created attachment 239260 [details] [review]
0003-Fix-setting-of-struct-property-values.patch
Comment 3 Daniel Drake 2013-03-19 14:56:47 UTC
Created attachment 239261 [details] [review]
0004-Consolidate-signal-connection-code.patch
Comment 4 Daniel Drake 2013-03-19 14:57:10 UTC
Created attachment 239263 [details] [review]
0005-Optimize-connection-of-Python-implemented-signals.patch
Comment 5 Daniel Drake 2013-03-19 14:57:40 UTC
Created attachment 239264 [details] [review]
0006-Optimize-signal-lookup-in-gi-repository.patch
Comment 6 Simon Feltman 2013-03-21 03:53:40 UTC
Thanks for this! I (or someone) will review this soon. I'm marking this for 3.10 because we are currently in a hard code freeze with 3.8.

As a side note, I wonder if we could fast path all this and avoid marshaling altogether when properties are defined in Python and accessed in Python. Essentially if you look at gi/_gobject/propertyhelper.py:Property.__get__, it uses instance.get_property(self.name) which will jump through a lot of hoops marshaling stuff to return was was already directly available in Python. I would be curious to see some performance tests by changing this to use prop.fget(instance) directly.
Comment 7 Tomeu Vizoso 2013-03-21 07:21:42 UTC
Review of attachment 239258 [details] [review]:

Looks good. Would be very good to be able to test this, but AFAICS this isn't really exposed to python?
Comment 8 Tomeu Vizoso 2013-03-21 07:25:44 UTC
Review of attachment 239259 [details] [review]:

::: gi/_gobject/pygobject.c
@@ +303,3 @@
+     * read. */
+    if (pyg_gtype_is_custom (pspec->owner_type)) {
+        g_value_init(&value, G_PARAM_SPEC_VALUE_TYPE(pspec));

Please mind code style (space before "(")
Comment 9 Tomeu Vizoso 2013-03-21 07:27:16 UTC
Review of attachment 239260 [details] [review]:

Good catch, but this one really needs a test.
Comment 10 Simon Feltman 2013-03-21 08:30:32 UTC
Review of attachment 239258 [details] [review]:

Sorry to step on your review Tomeu but these things stuck out to me :)

::: gi/_gobject/pygobject.h
@@ -184,3 @@
                                           PyObject   *warning);
     void      (*disable_warning_redirections) (void);
-    void      (*type_register_custom)(const gchar *type_name,

It turns out pygobject.h is distributed and people actually rely on it. So removing this would break the ABI, but it seems like it could just point to a dummy version which prints a deprecation warning.

@@ -253,3 @@
 #define pyg_add_warning_redirection   (_PyGObject_API->add_warning_redirection)
 #define pyg_disable_warning_redirections (_PyGObject_API->disable_warning_redirections)
-#define pyg_type_register_custom_callback (_PyGObject_API->type_register_custom)

Removing this could potentially break the API for someone, so perhaps just define it as a no-op.
Comment 11 Tomeu Vizoso 2013-03-21 09:31:20 UTC
Review of attachment 239261 [details] [review]:

Apparently (at least lcov says so) we don't have tests for pygobject_connect_object and pygobject_connect_object_after, maybe those can be copied from the other signal ones, to cover your changes.
Comment 12 Tomeu Vizoso 2013-03-21 09:49:47 UTC
Review of attachment 239263 [details] [review]:

::: gi/_gobject/pygobject.c
@@ +1649,3 @@
     }
 
+    g_signal_query (sigid, &query_info);

Guess it shouldn't happen at this point, but maybe check for failure (signal_id == 0) to avoid hard to diagnose bugs?
Comment 13 Tomeu Vizoso 2013-03-21 09:56:58 UTC
Review of attachment 239264 [details] [review]:

Guess these changes are covered by the existing tests, but may not hurt to check the coverage report after applying all the patches.
Comment 14 Daniel Drake 2013-03-21 14:16:40 UTC
(In reply to comment #9)
> Review of attachment 239260 [details] [review]:
> 
> Good catch, but this one really needs a test.

It already has a test. Otherwise I wouldn't have written this patch :)
Comment 15 Martin Pitt 2013-03-25 08:25:23 UTC
Note that the A-C-F'ed patches depend on the first patch for pyg_gtype_is_custom(), so they can't go in yet. The freeze in master is over, though.
Comment 16 Daniel Drake 2013-03-25 19:06:56 UTC
(In reply to comment #6)
> As a side note, I wonder if we could fast path all this and avoid marshaling
> altogether when properties are defined in Python and accessed in Python.
> Essentially if you look at gi/_gobject/propertyhelper.py:Property.__get__, it
> uses instance.get_property(self.name) which will jump through a lot of hoops
> marshaling stuff to return was was already directly available in Python. I
> would be curious to see some performance tests by changing this to use
> prop.fget(instance) directly.

This seems to make a small improvement to Sugar startup time on XO-4 (our fastest platform), maybe by 100ms. Next week I will be able to test on XO-1, that will give a more definite answer thanks to its slowness :)

I think we might be optimizing two different things here. I think your optimization affects the codepath for reading of "myobj.myproperty" whereas mine affects "myobj.props.myproperty"? Not sure what the history here is, is one style is preferred over the other?
Comment 17 Daniel Drake 2013-03-25 19:37:31 UTC
(In reply to comment #10)
> It turns out pygobject.h is distributed and people actually rely on it. So
> removing this would break the ABI, but it seems like it could just point to a
> dummy version which prints a deprecation warning.

If this functionality really has a user, I imagine I have just broken someone elses app, so we probably need a more complete understanding here. Looking through google results this is the only mention I can find:

http://www.filewatcher.com/p/python3-gobject2-devel-2.28.6-9.1.3.x86_64.rpm.216453/usr/share/pygobject/2.0/codegen/codegen.py.html

I removed this functionality as it seemed to be unused, and seemed to have no function that could spring to mind. This whole system just seems to influence the results of _pyg_type_from_name(). I don't see whyt his was necessary - I expect custom gtypes to be registered with libgobject and hence g_type_from_name() should be enough to be able to find it later.
Comment 18 Daniel Drake 2013-04-08 15:29:11 UTC
(In reply to comment #16)> This seems to make a small improvement to Sugar startup time on XO-4 (our
> fastest platform), maybe by 100ms. Next week I will be able to test on XO-1,
> that will give a more definite answer thanks to its slowness :)

I think there might be a very small Sugar startup speedup here, but even on slow XO-1 it is hard to measure.

So, testing with a more simplistic test case, reading a Python-defined string property 100000 times, before applying any patches:

Accessing obj.myprop directly: 
With instance.get_property(): 23 seconds
With fget: 10 seconds

Accessing obj.props.myprop:
40 seconds (instance.get_property vs fget makes no difference)

So I would say this fget change is worth doing, makes a significant difference when reading Python-defined properties via obj.myprop. However it doesn't optimize access via .props, which my patches do optimize, and the .props approach seems to be what Sugar uses everywhere.
Comment 19 Simon Feltman 2013-04-11 11:37:54 UTC
(In reply to comment #17)
> I removed this functionality as it seemed to be unused, and seemed to have no
> function that could spring to mind. This whole system just seems to influence
> the results of _pyg_type_from_name(). I don't see whyt his was necessary - I
> expect custom gtypes to be registered with libgobject and hence
> g_type_from_name() should be enough to be able to find it later.

I doubt this particular function has a user, it looks like a relic from the past. I am more worried about changing the vtable it is added to (breaking ABI):
-    void      (*type_register_custom)(const gchar *type_name,

So I think just assigning this to NULL or possibly a dummy stub which raises a deprecation rather than remove it from the vtable seems fine.

(In reply to comment #18)
> So I would say this fget change is worth doing, makes a significant difference
> when reading Python-defined properties via obj.myprop. However it doesn't
> optimize access via .props, which my patches do optimize, and the .props
> approach seems to be what Sugar uses everywhere.

Thanks for the bench marks! I think your patches and the fget change both make sense. Just to clarify:

> With instance.get_property(): 23 seconds

> Accessing obj.props.myprop:
> 40 seconds (instance.get_property vs fget makes no difference)

Are both of these results using the same test? Eventually I would like to remove the static "props" accessor and turn into a Python overrides addition. I wonder if we could make these equally as fast by doing something like:

instance.props.__getitem__ = instance.get_property
Comment 20 Daniel Drake 2013-04-12 16:18:01 UTC
Created attachment 241368 [details] [review]
0001-Simplify-registration-of-custom-types.patch

Good point about ABI. Here is an updated 0001 patch which has a NULL pointer in place of the old function pointer.
Comment 21 Daniel Drake 2013-04-12 16:21:06 UTC
(In reply to comment #9)
> Review of attachment 239260 [details] [review]:
> 
> Good catch, but this one really needs a test.

I have double checked that there is already a test here.

Before the patch, this test fails:

FAIL: test_boxed (test_everything.TestProperties)
Traceback (most recent call last):
  • File "/home/dsd/projects/pygobject/tests/test_everything.py", line 984 in test_boxed
    self.assertTrue(isinstance(object_.props.boxed, Everything.TestBoxed))
AssertionError: False is not true

After the patch, the test succeeds.
Comment 22 Simon Feltman 2013-04-14 02:28:16 UTC
Comment on attachment 241368 [details] [review]
0001-Simplify-registration-of-custom-types.patch

Committed with some changelog cleanup and additional usage of GINT_TO_POINTER
for the qdata.
Comment 23 Simon Feltman 2013-04-14 03:51:13 UTC
Review of attachment 239259 [details] [review]:

There is an issue with this patch in that it will remove the ability to access dynamic attributes from other systems (AFAICT). I suggest something like the following for both getting and setting properties:

if not is_gtype_custom(owner_type):
    # try to use pygi
    res = pygi_get_property(...)
    if res:
        return res

# use standard property access
return pyg_get_property(...)

It would be nice if this could also be done to the "get_property" method implementation along with the props accessor (seems like these should be sharing more code as well).
Comment 24 Daniel Drake 2013-04-15 14:48:47 UTC
(In reply to comment #23)
> Review of attachment 239259 [details] [review]:
> 
> There is an issue with this patch in that it will remove the ability to access
> dynamic attributes from other systems (AFAICT). I suggest something like the
> following for both getting and setting properties:

Sorry for the ignorance. Can you give an example of a "dynamic attribute from another system" and what complications this would present here?
 
> if not is_gtype_custom(owner_type):
>     # try to use pygi
>     res = pygi_get_property(...)
>     if res:
>         return res
> 
> # use standard property access
> return pyg_get_property(...)

What is a "standard property access" here?
Comment 25 Simon Feltman 2013-04-15 16:34:33 UTC
(In reply to comment #24)
> (In reply to comment #23)
> > Review of attachment 239259 [details] [review] [details]:
> > 
> > There is an issue with this patch in that it will remove the ability to access
> > dynamic attributes from other systems (AFAICT). I suggest something like the
> > following for both getting and setting properties:
> 
> Sorry for the ignorance. Can you give an example of a "dynamic attribute from
> another system" and what complications this would present here?

Sure, this could be any system which has object types or attributes that are not defined in a GI typelib (not necessarily dynamically created at runtime like with Python, but that is also a possibility). So the patch is not a mere optimization because it is limiting property access to only Python defined types or properties defined in GI. A concrete example might be something like objects returned from gst_element_factory_make, which creates objects based on plugins which do not have a GI typelib.

> > # use standard property access
> > return pyg_get_property(...)
> 
> What is a "standard property access" here?

I just meant use g_object_get_property or some wrapped version which doesn't try to use GI information.
Comment 26 Daniel Drake 2013-04-16 19:55:03 UTC
Created attachment 241684 [details] [review]
0002-Optimize-property-get-set.patch

Thanks, here is an updated patch with that in mind.
Comment 27 Simon Feltman 2013-04-17 02:21:55 UTC
Comment on attachment 241684 [details] [review]
0002-Optimize-property-get-set.patch

Thanks, pushed with a simplified commit message.
Comment 28 Daniel Drake 2013-04-17 13:58:24 UTC
Thanks. I think patch 3 is ready for push as well, when you have time. I will work on the tests for patch 4 today.
Comment 29 Daniel Drake 2013-04-17 17:47:16 UTC
Created attachment 241763 [details] [review]
0004-Consolidate-signal-connection-code.patch

Added basic tests for all the signal connection techniques.
Comment 30 Martin Pitt 2013-04-18 05:38:21 UTC
(In reply to comment #14)
> (In reply to comment #9)
> > Review of attachment 239260 [details] [review] [details]:
> > 
> > Good catch, but this one really needs a test.
> 
> It already has a test. Otherwise I wouldn't have written this patch :)

This one is really curious. By the looks of it, the current code should completely break the setting of struct/boxed properties as the "goto out" would always circumvent the actual g_object_set_property() call. And yet it works, we have test_gi.TestPropertiesObject.test_{boxed_struct,boxed_glist}() which succeed. What am I missing here?

Daniel, how did you run into this, did it just catch your eye, or do you have some piece of code which breaks without this patch? As Tomeu already said, I'd really like to cover this with a test.
Comment 31 Martin Pitt 2013-04-18 06:10:47 UTC
(In reply to comment #30)
> What am I missing here?

Ah, it seems I don't fully understand what pygi_set_property_value_real() does. Short-circuiting it to immediately return -1 only breaks 5 test cases (the more complex ones in test_properties.TestProperty), but all the simple ones (like test_boolean) and all other test modules just work fine.

So our tests indeed cover this code path (breaking e. g. the g_value_set_variant() call in the case statement shows that), but somewhere in the code there is a fallback which still sets the property if pygi_set_property_value_real() fails. If someone could enlighten me how that works, I'd appreciate.

In the meantime, I think it's fine to apply that fix, it's obviously right.
Comment 32 Martin Pitt 2013-04-18 06:17:23 UTC
Comment on attachment 241763 [details] [review]
0004-Consolidate-signal-connection-code.patch

Thanks, this looks fine.
Comment 33 Martin Pitt 2013-04-18 06:27:20 UTC
The remaining two patches are now unblocked, pushed them. Many thanks!

I'd still like to solve the mystery of patch 0003, but the main point of this bug is done now, so closing.
Comment 34 Daniel Drake 2013-04-18 14:19:46 UTC
(In reply to comment #30)
> Daniel, how did you run into this, did it just catch your eye, or do you have
> some piece of code which breaks without this patch? As Tomeu already said, I'd
> really like to cover this with a test.

My code changes somehow exposed the issue. i.e. after applying the first two patches, running the test suite resulted in a few failures that weren't there before.

I don't have an explanation for why the test cases weren't failing before, I just went with the obvious fix and moved on.
Comment 35 Daniel Drake 2013-04-18 14:49:02 UTC
(In reply to comment #31)
> So our tests indeed cover this code path (breaking e. g. the
> g_value_set_variant() call in the case statement shows that), but somewhere in
> the code there is a fallback which still sets the property if
> pygi_set_property_value_real() fails. If someone could enlighten me how that
> works, I'd appreciate.

Maybe this is exactly what was touched upon in the code review of the 0002 patch.

In my original version, PyGProps_setattro for GI-defined classes would try to set the property with GI, and if that fails, bail with error. That would have caused some failing tests.

In the revised/committed version, for GI-defined classes PyGProps_setattro will try using GI to set the property first, and if that fails, it will fall back on set_property_from_pspec(). That might work as a fallback (not sure what the considerations involved here are), meaning that these tests could be no longer failing even though I did uncover a real bug in the GI path.