GNOME Bugzilla – Bug 547104
improve type wrapper creation
Last modified: 2008-08-11 20:18:35 UTC
As I noted in bug #546120, we need to improve runtime type wrapper creation in order for patch in that bug to work. Patch follows.
Created attachment 116264 [details] [review] improvement patch Patch that makes newly created type wrappers inherit some slots following some algorithm. It is best described in comments in the patch.
Created attachment 116290 [details] [review] improvement patch, with nasty pointer arithmetics hidden in a macro Other than the macro stuff, the patch is the same. Result of this patch + GFile slots in bug #546120: >>> import gio >>> f = gio.File ('test'); g = gio.File ('test') >>> f <__main__.GLocalFile at 0x407d570c: file:///home/paul/svn/pygobject/test> >>> f == g True >>> hash(f) == hash(g) True
Comment on attachment 116290 [details] [review] improvement patch, with nasty pointer arithmetics hidden in a macro >Index: gobject/pygobject.c How can we test this modification?>+static void >+pygobject_pick_slot_value(PyTypeObject *type, PyObject *bases, int slot_offset) >+{ >+#define TYPE_SLOT_VALUE(type) \ >+ (* (void **) (((char *) (type)) + slot_offset)) >+ >+ void *gobject_value = TYPE_SLOT_VALUE(&PyGObject_Type); >+ void *object_value = TYPE_SLOT_VALUE(&PyBaseObject_Type); These can probably go, let's trust the compiler to optimize for us. >+ void *value = NULL; >+ int k; Why not just i? >+ int num_bases; >+ >+ for (k = 0, num_bases = PyTuple_Size(bases); k < num_bases; ++k) { I'd move num_bases outside of the for. >+ PyTypeObject *base_type = (PyTypeObject *) PyTuple_GetItem(bases, k); >+ void *slot_value = TYPE_SLOT_VALUE(base_type); >+ >+ if (slot_value && slot_value != gobject_value && slot_value != object_value) { >+ if (value) { >+ value = NULL; >+ break; >+ } >+ else >+ value = slot_value; >+ } >+ } Can this if be rewritten some thing like: if (slot_value == NULL) continue if (slot_value == TYPE_SLOT_VALUE(&PyGObject_Type) || slot_value == TYPE_SLOT_VALUE(&PyBaseObject_Type)) continue if (value == NULL) return value = slot_value; >+ >+ if (value) >+ TYPE_SLOT_VALUE(type) = value; And then you could remove the if here. >+#undef TYPE_SLOT_VALUE
Created attachment 116292 [details] [review] improvement patch, revision 3 (In reply to comment #3) [Removed parts are just done.] > How can we test this modification? This I frankly don't know for sure. A couple of non-direct results: - all PyGObject and PyGTK tests keep passing; - the patch with GFile slots starts working as intended (and it doesn't without this patch); - PyGObject/PyGTK actually seldom use these slots anyway (as of yet), so the impact of the patch must be limited to relatively few cases. > Why not just i? Oh, I'm just weird with naming conventions. Changed. > Can this if be rewritten some thing like: > [...] Done. I also noticed one minor bug here: if multiple bases give us the same slot value (e.g. because one already inherited custom slot from another via this way), we must treat it like one value. Fixed in this revision. > >+ > >+ if (value) > >+ TYPE_SLOT_VALUE(type) = value; > > And then you could remove the if here. Actually, no, I can't. See the comments in the patch.
Comment on attachment 116292 [details] [review] improvement patch, revision 3 Okay, getting better. value -> found_slot base_value -> slot TYPE_SLOT_VALUE -> TYPE_SLOT pygobject_pick_slot_value -> pygobject_find_slot_for Right?
Created attachment 116293 [details] [review] improvement patch, revision 4 Well, OK, this was at least simple :)
Thanks for the quick review! Sending ChangeLog Sending gobject/pygobject.c Transmitting file data .. Committed revision 938. 2008-08-10 Paul Pogonyshev <pogonyshev@gmx.net> Bug 547104 – improve runtime type wrapper creation * gobject/pygobject.c (pygobject_new_with_interfaces): Use new pygobject_find_slot_for() for `tp_richcompare', `tp_compare`, `tp_hash', `tp_iter', `tp_repr', `tp_str' and `tp_print'. (pygobject_find_slot_for): New static function.
I'm reopening the bug, as I understood that we can do the same not only for runtime-created types, but for normal ones as well, without modifying codegen in any way. Rationale for doing this also for normal types: * Less code duplication: example is 'gicon.override' which currently has to duplicate 'tp_richcompare' and 'tp_hash' for all the different types. In addition to code duplication being bad as it is, this is also error-prone if GLib adds more implementation, but we forget to copy custom slots for the wrappers of these new implementation. * Sometimes we need to duplicate code across module boundaries. E.g. types gio.AppInfo and gio.unix.DesktopAppInfo are an interface and its implementation residing in different modules. They have a natural 'tp_richcompare' implementation, but there are technical difficulties with PyGAppInfo_Type not being available in gio.unix module. The difficulties can be solved in other ways too, but with proposed changes we don't need to have them at all. Patch in a minute.
Created attachment 116300 [details] [review] improvement patch, next generation (basing on what is already committed) This patch improves type creation for normal types (passed through pygobject_register_class), not only runtime-created types. I made some restructuring of the code, but the changes are practically technical and don't make any real differences. Additionally, I removed all duplicated in 'gicon.override' to show that it does indeed work as intended. After removal all tests keep passing. That wouldn't be the case without changes in 'pygobject.c'.
Created attachment 116302 [details] [review] patch adding two slots to gio.AppInfo This patch is only somewhat related to the bug in question. However, it underlines the point of second type creation improvement patch. It changes only slots of gio.AppInfo. If patch in attachment 116300 [details] [review] is not applied, it makes no visible changes, because in reality all appinfos are of type gio.unix.DesktopAppInfo (implementation of gio.AppInfo). However, when that patch is applied, the two slots from gio.AppInfo get propagated into gio.unix.DesktopAppInfo, now making a difference: >>> gio.app_info_get_all()[0] <gio.unix.DesktopAppInfo at 0x401fffa4: kazehakase> Also, with both patches applied, the test_eq starts passing.
Approved by Johan on IRC. Sending ChangeLog Sending gio/gicon.override Sending gobject/pygobject.c Transmitting file data ... Committed revision 940. 2008-08-11 Paul Pogonyshev <pogonyshev@gmx.net> Bug 547104 – improve type wrapper creation * gobject/pygobject.c (pygobject_register_class): Use new pygobject_inherit_slots() to propagate custom slots in normal types too. (pygobject_inherit_slots): New function, break out of pygobject_new_with_interfaces() and rewrite a bit. (pygobject_find_slot_for): Add new argument that can forbid overriding non-NULL slots. * gio/gicon.override (pygio_do_icon_richcompare): Remove, the code is now directly in _wrap_g_icon_tp_richcompare(). (_wrap_g_file_icon_tp_richcompare, _wrap_g_file_icon_tp_hash) (_wrap_g_themed_icon_tp_richcompare, _wrap_g_themed_icon_tp_hash): Remove, duplicating code in this way is not needed anymore. Sending ChangeLog Sending gio/gappinfo.override Sending tests/test_gio.py Transmitting file data ... Committed revision 941. 2008-08-11 Paul Pogonyshev <pogonyshev@gmx.net> Bug 547104 – improve type wrapper creation * gio/gappinfo.override (_wrap_g_app_info_tp_richcompare) (_wrap_g_app_info_tp_repr): New functions. * tests/test_gio.py (TestAppInfo.test_eq): New test.