GNOME Bugzilla – Bug 640629
_setup_native_vfuncs() unnecessarily recurses to all base classes; makes startup slow
Last modified: 2011-02-01 06:08:00 UTC
The current implementation recurses too all base classes, and installs native vfuncs from all base classes onto the new subclass. For example, Gtk.Dialog has a vfunc "do_close". Gtk.AboutDialog subclasses Gtk.Dialog. In the current implementation, the vfunc "do_close" is wrapped once when it is installed on Gtk.Dialog, once when installed on Gtk.AboutDialog and once for any other subclasses. That is, after _setup_native_vfuncs() is run, there will be different function wrappers created as Gtk.Dialog.do_close, and Gtk.AboutDialog.do_close, which both point to the same native function. Creating separate wrapper objects is unnecessary, and it is not even necessary to set the attribute "do_close" on Gtk.AboutDialog. Because if Python's getattr() can't find do_close on Gtk.AboutDialog it will search the base classes, and return the one found on Gtk.Dialog. Without setting it explicitly on Gtk.AboutDialog, The vfunc will still be accessible, and everything will still work the same. Additionally, there is another problem which causes vfuncs to be installed unnecessarily. For example let's say PythonSubObject inherits from Gtk.Dialog. When _setup_native_vfuncs() is called, it tries to get the __info__ attribute, which is only available in the __dict__ of wrapped native classes. Since no '__info__' exists in PythonSubObject.__dict__, retrieving PythonSubObject.__info__ will search the base class and return the Gtk.Dialog.__info__ attribute. Then "do_close" will be installed on PythonSubObject again, because we think we are handing a new set of vfuncs in a new __info__ object. These factors together make each vfunc in C be wrapped far more times that it needs to be. In the case of subclasses implemented in overrides, it is 4 times more. If an application uses larger class hierarchies, it would be much more.
Created attachment 179363 [details] [review] [gi] Speed up _setup_native_vfuncs() This changes _setup_native_vfuncs() to only install native vfunc wrappers from the current class on the current class. Native vfuncs will not be propogated up or down the class hierarchy as this is unnecessary and wastes CPU and memory. Since the normal process in python to retrieve a method or attribute recurses to the base classes if an attribute is not found in the subclass, there is no need to setup all base class virtual functions on a subclass. This patch removes the recursion in _setup_native_vfuncs() and lets Python find them in the base classes like a normal Python class would work. This significantly increases the speed of any class which is or inherits from a C class which includes virtual methods.
Review of attachment 179363 [details] [review]: This looks nice. Do you think it would be possible to somehow write a test ensuring we are not overriding the same function more than once? Also, does it make a difference in the memory used? I guess the gain in time is not measurable.
Yes, I will try to make some tests for the next patch. Actually, the gain in time was the point of this patch, as I was trying to speed up the import of Gtk. Time spent in _setup_native_vfuncs() during warm import of Gtk (including time spent in functions it all calls): - git master: 0.078s - this patch: 0.001s Time spent for entire warm import of Gtk: - git master: 0.12s - this patch: 0.04s But also fewer NativeVFunc wrappers are created, so memory is saved. Here is the resident memory size after importing Gtk: - git master: 16,272 KB - this patch: 14,784 KB
Wow, this is really impressive.
Created attachment 179376 [details] [review] [gi] Speed up _setup_native_vfuncs() This changes _setup_native_vfuncs() to only install native vfunc wrappers from the current class on the current class. Native vfuncs will not be propogated up or down the class hierarchy as this is unnecessary and wastes CPU and memory. This patch is the same as the previous one, but with a test case included.
Review of attachment 179376 [details] [review]: ::: gi/types.py @@ +155,3 @@ + # Also check if __info__ in __dict__, not hasattr('__info__', ...) + # because we do not want to accidentally retrieve __info__ from a base class. + if not '__info__' in cls.__dict__ or \ You can write this a bit cleaner like this. # __info__ is set in base classes, avoid traversing the class heirachy, # and only get it for the current class. class_info = cls.__dict__.get('__info__') if class_info is None or not isinstance(class_info, ObjectInfo): return How many times will this isinstance() be called for a normal gtk+ import? We could do some integer checking instead, by exposing the GIInfoType on the GIBaseInfo wrapper? That'd be even faster - since this seems performance sensitive. @@ +158,3 @@ + not isinstance(cls.__info__, ObjectInfo): + return + for vfunc_info in cls.__info__.get_vfuncs(): Use class_info from above her.
(In reply to comment #6) > Review of attachment 179376 [details] [review]: > > ::: gi/types.py > @@ +155,3 @@ > + # Also check if __info__ in __dict__, not hasattr('__info__', ...) > + # because we do not want to accidentally retrieve __info__ from a base > class. > + if not '__info__' in cls.__dict__ or \ > > You can write this a bit cleaner like this. > > # __info__ is set in base classes, avoid traversing the class heirachy, > # and only get it for the current class. > class_info = cls.__dict__.get('__info__') > if class_info is None or not isinstance(class_info, ObjectInfo): > return Thinking about it, class_info.__class__ is ObjectInfo will work as well and should be what we want here. Slightly evil optimization but...
Created attachment 179382 [details] [review] [gi] Speed up _setup_native_vfuncs() Same as previous patch, with Johan's comments included.
Comment on attachment 179382 [details] [review] [gi] Speed up _setup_native_vfuncs() Does not apply to HEAD. Can you rebase so I can test. Thanks.
It does not apply, because this patch was already committed. So I am changing status of bug.