Review of attachment 248673 [details] [review]:

Hi Daniel,

This piece of code has always stuck out to me a bit so I'm all for trying to short circuit it where possible. But I think  __mro__ is already calculated by Python and doing this will circumvent the new version completely? The reason we might be seeing similar print statements is types.py:mro() is actually a copy of the builtin __mro__ with some tweaks so it will be the same most of the time. I'm not sure exactly why types.py:mro() exists but bug 642437 covers some of it. It would be nice to find a better explanation and a concrete example of why it is needed... hopefully Tomeu is listening :)

__mro__ isn't already calculated by Python. It is where the results of *our* MRO function get stored. That is why this optimization works.

When Python wants to calculate the MRO it first looks if the class has a mro() method, if so it calls it. The result is stored in __mro__ and then class.mro() is never called again. Python's internal MRO calculation is never executed for our classes as a result of this override. http://docs.python.org/2/library/stdtypes.html#special-attributes

It may be clearer to think about this in another way. When something at the Python level touches the Gtk.Button class for the first time, Python auto-loads all the base classes as well (like Gtk.Widget), and all their bases, calculating all the MROs where they haven't already been calculated (in this case it invokes our class override). So actually there is no need for this function to be recursive at all, since when reaching this point it is guaranteed that base class MRO's are already available (and calculated by us, where appropriate). You could replace my patch with this new one:

-        bases_of_subclasses += list(map(mro, C.__bases__)) + [list(C.__bases__)]
+        bases_of_subclasses += list([list(base.__mro__) for base in C.__bases__]) + [list(C.__bases__)]

As for why this code is necessary in the first place, http://en.wikipedia.org/wiki/Diamond_problem clarified the problem space for me. The next question is why GObject interfaces don't face the diamond problem (this is mentioned in the comment). I would also appreciate a more complete explanation here. But after a quick read of the gobject manual and source I suspect it is because in gobject, the last person who implements an interface in the hierarchy "wins", there is never any conflict.

Another way to see the requirement for this code is to comment it out and run the unit tests. There is a unit test which fails when this code is removed.

The second version of my patch (included in the above comment) is actually faster than the original as it avoids branching into a recursive function call. Although the difference is quite small, it is perhaps only a 0.005 second speedup in my "hello world" test on this platform.


I also figured out the discrepancy in my numbers:

Previously, when launching a "Hello World" Sugar app, 1.9 seconds was spent in this function.

After the patch, 0.04 seconds is spent in this function.

Overall, the app startup time is decreased by 0.5 seconds.



My initial 1.9 seconds measurement was inaccurate. I was too simplistic in measuring this, and I counted time multiple times when the function recursed into itself. Taking a better measurement, I see that before the patch we spend about 0.5 seconds in this function. And that agrees with my "app startup" timing.

Review of attachment 248673 [details] [review]:

Ah, that makes sense. Thanks for the clarification. Looks like an easily confused topic :)
https://plus.google.com/115212051037621986145/posts/NXA7C6yDGtT

Created attachment 248751 [details] [review]
Speed up MRO calculation

I ran into a problem running the test suite through Python 2.7 as it
does not support __mro__ for old style classes. Luckily there was an
occurance of using an old style mixin with a GObject derived class
causing the suite to fail. I've updated the patch to check explicitly
check for this and print a warning if __mro__ is not on the class.

Daniel,

It would be great if you could review and test my additions to the patch as the additional "hasattr" may affect performance.

Created attachment 248770 [details] [review]
new patch

That looks good, but here is an updated patch along the same lines, which adds in all the other tricks and considerations that we've mentioned above.

Review of attachment 248770 [details] [review]:

The code looks good. A few things:

Beyond making things faster, I think it is pertinent for this bit of code to become more maintainable. Simply adding a better comment/description to the function itself would do. Ascii art presenting the problem case of why we need to override mro in the first place, etc...

We need more tests. We currently have a single test in tests/test_gi.py:TestInterfaces.test_mro
I think breaking this out into a new test case "TestMRO" would be worthwhile. I added a test for the warning in comment #5 which shouldn't get lost. Basically a couple of additional tests which actually test the result of mro() for both the problem case with interfaces and the non-problem case where standard Python mro would have been sufficient. These tests should generally be done as a separate patch which is committed before the optimization to ensure the behaviour is the same for both the pre-optimized and optimized code.

As a maintainer I would like to ensure:
1. We don't regress now or in the future.
2. The reason for overriding mro() is easily discoverable, ideally in the code itself.

I am happy to help with this but you are the current expert with this bit of code :)

::: gi/types.py
@@ +329,3 @@
+                              RuntimeWarning)
+                # Calculating the C3-ish MRO for an old style class is not
+            else:  # old style class (Python2 only)

I don't think this comment is necessary. Using C3 with old style classes is not "incorrect", Python avoided it to keep things compatible. We only do it because there might be old style mixin classes (not the whole hierarchy) and we need to keep compatibility with the pre-optimized version.

(In reply to comment #8)
> I don't think this comment is necessary. Using C3 with old style classes is not
> "incorrect", Python avoided it to keep things compatible.

I can remove the comment if it helps review, after all it is not behaviour changed by this patch and there was no previous comment, but I would say it is definitely incorrect. This is changing the behaviour of core Python. At this point we are dealing with classes that fall totally outside gi and if we left Python to calculate the MRO here it would choose something different (since it uses an algorithm that gives different results than C3 on old-style classes). In this cases gi is reaching far beyond the boundaries of its own classes and changing behaviour of other elements of the system.

(In reply to comment #9)
> (In reply to comment #8)
> > I don't think this comment is necessary. Using C3 with old style classes is not
> > "incorrect", Python avoided it to keep things compatible.
> 
> I can remove the comment if it helps review, after all it is not behaviour
> changed by this patch and there was no previous comment, but I would say it is
> definitely incorrect. This is changing the behaviour of core Python. At this
> point we are dealing with classes that fall totally outside gi and if we left
> Python to calculate the MRO here it would choose something different (since it
> uses an algorithm that gives different results than C3 on old-style classes).
> In this cases gi is reaching far beyond the boundaries of its own classes and
> changing behaviour of other elements of the system.

Fair enough. Given a context, "incorrect" makes sense. But without context or reasoning, the comment leaves me (the reader) hanging. As a side note, for this to matter would mean someone is mixing a diamond pattern old style inheritance hierarchy into a GObject class. This would be a poor design choice at best, hence the new warning even for a simple case of mixing in an old style class :)

Created attachment 248854 [details] [review]
tests

Created attachment 248855 [details] [review]
optimization

I'm afraid I still can't offer a good explanation of why this function is necessary, because I don't know exactly why/how gobject avoids the diamond problem. However at least with these comments and tests I can more convincingly say we are not changing any existing behaviour.

Review of attachment 248854 [details] [review]:

This looks great, although it is currently failing for me with:

======================================================================
FAIL: test_mro (test_gi.TestMRO)
----------------------------------------------------------------------
Traceback (most recent call last):

+ Trace 232220

First differing element 7:
<class 'object'>
<class 'gi.repository.GObject.Object'>

Second tuple contains 2 additional elements.
First extra element 8:
<class 'gi._gobject.GObject'>

  (<class 'test_gi.TestMRO.test_mro.<locals>.E'>,
   <class 'test_gi.TestMRO.test_mro.<locals>.D'>,
   <class 'test_gi.TestMRO.test_mro.<locals>.B'>,
   <class 'test_gi.TestMRO.test_mro.<locals>.C'>,
   <class 'test_gi.TestMRO.test_mro.<locals>.A'>,
   <class 'gi.repository.GIMarshallingTests.Object'>,
   <class 'gi.overrides.GObject.Object'>,
+  <class 'gi.repository.GObject.Object'>,
+  <class 'gi._gobject.GObject'>,
   <class 'object'>)


I think this is due to the expected results only containing "GObject.Object" where there are actually a couple more classes below this (and are a little hard to get at).

expected = (..., GObject.Object, GObject.Object.__base__, gi._gobject.GObject, object)

Note you can also use "assertSequenceEqual" which doesn't care about list/tuple specificity.

Review of attachment 248855 [details] [review]:

Excellent, the additional comments and tests are much appreciated. Will commit once the problem with the tests are worked out.

(In reply to comment #13)
> Review of attachment 248854 [details] [review]:
> 
> This looks great, although it is currently failing for me with:

I don't have a system up-to-date enough to run master at the moment, so thats why I don't see it.  I guess the underlying GObject structure has changed since 3.4. Doesn't seem like anything to worry about.

Could you just fix up the test yourself according to those extra classes? assertSequenceEqual sounds fine to me as well.

Ah, yes, we did some serious re-work in regards to GObject.Object by removing static bindings and merging with the introspected version, along with adding overrides. Because of this, your performance patch will probably be even more drastic when testing with 3.9. I will fix this up.

Comment on attachment 248854 [details] [review]
tests

Pushed with GObject class updates since 3.8.

Comment on attachment 248855 [details] [review]
optimization

Committed with the addition of deriving PythonObjectWithNonVFuncDoMethod in test_gi.py to derive from "object" to avoid an unnecessary warning when running tests in Python 2.7.

Nice work, thank you!