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 704037 - Replace gi function wrapping with direct static info objects
Replace gi function wrapping with direct static info objects
Status: RESOLVED FIXED
Product: pygobject
Classification: Bindings
Component: introspection
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Simon Feltman
Python bindings maintainers
Depends on:
Blocks: 672207 693243
 
 
Reported: 2013-07-11 18:02 UTC by Simon Feltman
Modified: 2013-07-27 04:59 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Move doc string generator into separate module (11.96 KB, patch)
2013-07-12 19:30 UTC, Simon Feltman
committed Details | Review
Remove redundant info.get_name calls (2.13 KB, patch)
2013-07-14 01:18 UTC, Simon Feltman
committed Details | Review
Add API for explicitly enabling doc strings (gi.enable_doc_strings) (2.08 KB, text/plain)
2013-07-14 06:00 UTC, Simon Feltman
  Details
Merge method and constructor setup (2.44 KB, patch)
2013-07-14 06:23 UTC, Simon Feltman
none Details | Review
Merge method and constructor setup (2.20 KB, patch)
2013-07-14 06:32 UTC, Simon Feltman
committed Details | Review
Add common attribute accessors to PyGIBaseInfo (5.76 KB, patch)
2013-07-16 20:18 UTC, Simon Feltman
committed Details | Review
Add callable and descriptor protocols to PyGICallableInfo (17.56 KB, patch)
2013-07-17 00:03 UTC, Simon Feltman
none Details | Review
Fixed spelling mistakes in commit message. (17.56 KB, patch)
2013-07-17 03:32 UTC, Simon Feltman
needs-work Details | Review
Add callable and descriptor protocols to PyGICallableInfo (19.05 KB, patch)
2013-07-17 20:49 UTC, Simon Feltman
committed Details | Review
Replace Python VFunc descriptor directly with PyGIVFuncInfo (4.60 KB, patch)
2013-07-17 23:19 UTC, Simon Feltman
committed Details | Review

Description Simon Feltman 2013-07-11 18:02:55 UTC
This is a performance idea which will cut the number of function objects in half, and might also optimize call times because we remove an additional indirection of python calls.

We use a decorator for wrapping every function/method brought in by gi. By providing the double-under attributes used for function objects directly on the info struct, we would no longer need the wrapped function objects. We essentially do the following in types.py:

Foo.bar = Function(bar_info)
https://git.gnome.org/browse/pygobject/tree/gi/types.py?id=3.8.3#n150

We could remove this wrapping by providing __call__ and the other double-under attribute directly on the info objects, cutting the code down to:

Foo.bar = bar_info

The following attributes/aliases would need to be added to various info structs for this to work:

__name__
__module__
__doc__
__call__

https://git.gnome.org/browse/pygobject/tree/gi/pygi-info.c?id=3.8.3#n657

__doc__ should become a lazily evaluated attribute which calls into a registered doc string generator written in Python. This requires a new Python gi module API to set the doc string generator, something along the lines of:

import gi

def func(giinfo):
    return ""
gi.register_doc_string_generator(func)
Comment 1 Simon Feltman 2013-07-11 23:46:46 UTC
Taking a look at this.
Comment 2 Simon Feltman 2013-07-12 19:30:09 UTC
Created attachment 249045 [details] [review]
Move doc string generator into separate module

Move the doc string generator for creating function signatures
into "gi.docstring". This includes a new API for getting and
setting the doc string creation functions:

gi.docstring.get_doc_string_generator
gi.docstring.set_doc_string_generator
gi.docstring.generate_doc_string

Beyond adding the ability for custom doc string generators,
this API is a necessary step for adding lazy __doc__
attribute access for optimization.
Comment 3 Simon Feltman 2013-07-14 01:18:28 UTC
Created attachment 249097 [details] [review]
Remove redundant info.get_name calls

Remove a number of redundant calls to GIBaseInfo.get_name. Info
names are already cached on function objects so re-use them when
possible. This gives a small load time improvement by removing over
2000 calls when importing Gtk.

$ cat profile_gtk.py 
from gi.repository import Gtk

$ python -m cProfile -s tottime profile_gtk.py

 ncalls  tottime  percall  cumtime  percall filename:lineno(function)
   8541    0.004    0.000    0.004    0.000 {method 'get_name' of 'gi.BaseInfo' objects}

After patch:
   6506    0.003    0.000    0.003    0.000 {method 'get_name' of 'gi.BaseInfo' objects}
Comment 4 Simon Feltman 2013-07-14 06:00:29 UTC
Created attachment 249100 [details]
Add API for explicitly enabling doc strings (gi.enable_doc_strings)

Replace default of always generating doc strings with requiring
explicit enablement. While doc strings are nice to have by default,
the majority of run times will not make use of them. We gain roughly
a %15 import time performance improvement by not generating them.
Use: gi.enable_doc_strings() enable doc strings.

$ cat profile_gtk.py 
from gi.repository import Gtk

$ cat ntime
TIMEFORMAT='scale=3; %3U + %3S'
total=0.0
count=$1
shift
for i in $(seq 1 $count)
do
    current=$(echo $( { time $@ 2>/dev/null; } 2>&1 ) | bc)
    total=$(echo "scale=3; $total + $current" | bc)
done
echo "scale=3; $total / $count" | bc
 

$ ntime 100 python profile_gtk.py
.085

After patch:

$ ntime 100 python profile_gtk.py
.072
Comment 5 Simon Feltman 2013-07-14 06:04:45 UTC
Review of attachment 249100 [details]:

Needs test updates.
Comment 6 Simon Feltman 2013-07-14 06:23:45 UTC
Created attachment 249102 [details] [review]
Merge method and constructor setup

Merge _setup_constructors into _setup_methods as they contain same
basic logic. This removes an unnecessary call with additional
filtering of GIObjectInfo.get_methods() which can be large for
objects with many methods.

This is a small change which may give a %1 or %2 percent speedup.

Before:
$ ntime 100 python profile_gtk.py
.072

After:
$ ntime 100 python profile_gtk.py
.070
Comment 7 Simon Feltman 2013-07-14 06:32:58 UTC
Created attachment 249103 [details] [review]
Merge method and constructor setup

Fixed incorrect constructor wrapping.
Comment 8 Simon Feltman 2013-07-16 20:18:15 UTC
Created attachment 249320 [details] [review]
Add common attribute accessors to PyGIBaseInfo

Add __name__, __module__, and __doc__ accessors to
PyGIBaseInfo object. This is a precursory patch for setting
up PyGICallableInfo as a directly callable object with lazy
doc string evaluation.
Comment 9 Simon Feltman 2013-07-16 20:19:34 UTC
Review of attachment 249100 [details]:

No longer necessary as lazy evaluation will work out.
Comment 10 Simon Feltman 2013-07-17 00:03:59 UTC
Created attachment 249335 [details] [review]
Add callable and descriptor protocols to PyGICallableInfo

Add tp_call (__call__) function to callable info objects.
This allows for replacement of wrapped invoke methods directly
with the already created callable info object. This has the
additional side affect of making doc strings lazily bound
(only generated when __doc__ is accessed).

Add tp_desc_get which returns a bound version of the callable
info for both methods and constructors.

Update various internal type checks to reflect the changes.
Update tests to reflect the new callable type being the same
accross Python 2 & 3.

This patch gives roughly a %17 speedup for Gtk imports and
an %11 speedup for single argument GI method calls.

Additional notes on performance numbers:
- Tests were run using Python 2.7 with debug builds of the GNOME stack
- Import times were tested using the "ntime" script from comment #4
- Method call times were tested using the %timeit in ipython.

Before patch:
$ ntime 100 python profile_gtk.py
.088

$ ipython
In [1]: from gi.repository import Gtk
In [2]: button = Gtk.Button()
In [3]: %timeit button.get_size_request()
100000 loops, best of 3: 3.43 us per loop

After:
$ ntime 100 python profile_gtk.py
.073

$ ipython
...
In [3]: %timeit button.get_size_request()
100000 loops, best of 3: 3.04 us per loop
Comment 11 Simon Feltman 2013-07-17 03:32:56 UTC
Created attachment 249343 [details] [review]
Fixed spelling mistakes in commit message.
Comment 12 Simon Feltman 2013-07-17 20:35:22 UTC
Review of attachment 249343 [details] [review]:

::: gi/pygi-info.c
@@ +403,3 @@
+ */
+static PyObject *
+_callable_info_call (PyGICallableInfo *self, PyObject *args, PyObject *kwargs)

This needs to be broken out down into separate functions. A generic version for GICallableInfo and a specialized version for GIFunctionInfo (and eventually GIVFuncInfo)

@@ +482,3 @@
+ */
+static PyObject *
+                              "%s constructor cannot be used to create instances of "

Same comment as above
Comment 13 Simon Feltman 2013-07-17 20:49:39 UTC
Created attachment 249440 [details] [review]
Add callable and descriptor protocols to PyGICallableInfo

Add tp_call (__call__) function to callable info objects.
This allows for replacement of wrapped invoke methods directly
with the already created callable info object. This has the
additional side effect of making doc strings lazily bound
(only generated when __doc__ is accessed).

Add tp_desc_get (__get__) to PyGIFunctionInfo which returns
a bound version of itself for methods and constructors.

Update various internal type checks to reflect the changes.
Update tests to reflect the new callable type being the same
across Python 2 & 3.

This patch gives roughly a %17 speedup for Gtk imports and
an %11 speedup for GI method calls.

Additional notes on performance from comment #10 are still valid.
Comment 14 Simon Feltman 2013-07-17 23:19:00 UTC
Created attachment 249452 [details] [review]
Replace Python VFunc descriptor directly with PyGIVFuncInfo

Add tp_getdesc (__get__) to PyGIVFuncInfo to allow the object
to be used directly as a callable descriptor. This piggy backs
off the added support for functions and constructors in previous
patches.
Comment 15 Martin Pitt 2013-07-25 09:24:06 UTC
Comment on attachment 249045 [details] [review]
Move doc string generator into separate module

This is a nice factorization and should not have any performance impact, LGTM.
Comment 16 Martin Pitt 2013-07-25 09:33:35 UTC
Comment on attachment 249097 [details] [review]
Remove redundant info.get_name calls

Nice!
Comment 17 Martin Pitt 2013-07-25 09:40:36 UTC
Review of attachment 249320 [details] [review]:

Looks good otherwise, please push with the missing space fixed. Thanks!

::: gi/pygi-info.c
@@ +210,3 @@
+    static PyObject *docstr;
+    if (docstr == NULL) {
+        docstr= PYGLIB_PyUnicode_InternFromString("__doc__");

Only a tiny nitpick here, missing space before "=".
Comment 18 Martin Pitt 2013-07-25 13:29:22 UTC
For the remaining two, I'm honestly not feeling qualified enough to sensibly review them. I verified that they work fine for both py2 and py3 though, so if you feel good about them, please let's get them into 3.9.5 for wider testing.

Please do consider running check.valgrind before/after and ensure that this doesn't introduce more leaks. (NB you'll need to run with PYTHON=... pointing to a custom python build with --enable-valgrind, as the Ubuntu packages don't do that).

Thanks!
Comment 19 Simon Feltman 2013-07-26 23:42:50 UTC
Attachment 249045 [details] pushed as 6fdde25 - Move doc string generator into separate module
Attachment 249097 [details] pushed as 6b36fbe - Remove redundant info.get_name calls
Attachment 249103 [details] pushed as ea19440 - Merge method and constructor setup
Attachment 249320 [details] pushed as bec0b54 - Add common attribute accessors to PyGIBaseInfo
Comment 20 Simon Feltman 2013-07-27 04:59:21 UTC
Running valgrind was definitely a good call. While these patches by themself didn't
have any leaks, the new setup which removes layers of Python partials exposed
problems with how we hack vfunc return values. I've fixed this in a prior commit:
https://git.gnome.org/browse/pygobject/commit/?id=2339e030e4d

Attachment 249440 [details] pushed as 35f79b2 - Add callable and descriptor protocols to PyGICallableInfo
Attachment 249452 [details] pushed as 91c4982 - Replace Python VFunc descriptor directly with PyGIVFuncInfo