Created attachment 239259 [details] [review]
0002-Optimize-property-get-set.patch

Created attachment 239260 [details] [review]
0003-Fix-setting-of-struct-property-values.patch

Created attachment 239261 [details] [review]
0004-Consolidate-signal-connection-code.patch

Created attachment 239263 [details] [review]
0005-Optimize-connection-of-Python-implemented-signals.patch

Created attachment 239264 [details] [review]
0006-Optimize-signal-lookup-in-gi-repository.patch

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.

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?

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 "(")

Review of attachment 239260 [details] [review]:

Good catch, but this one really needs a test.

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.

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.

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?

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.

(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 :)

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.

(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?

(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.

(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.

(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

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.

(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):

+ Trace 231778

After the patch, the test succeeds.

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.

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).

(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?

(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.

Created attachment 241684 [details] [review]
0002-Optimize-property-get-set.patch

Thanks, here is an updated patch with that in mind.

Comment on attachment 241684 [details] [review]
0002-Optimize-property-get-set.patch

Thanks, pushed with a simplified commit message.

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.

Created attachment 241763 [details] [review]
0004-Consolidate-signal-connection-code.patch

Added basic tests for all the signal connection techniques.

(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.

(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 on attachment 241763 [details] [review]
0004-Consolidate-signal-connection-code.patch

Thanks, this looks fine.

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.

(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.

(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.