GNOME Bugzilla – Bug 686443
Replace static MainLoop/MainContext/Source bindings with GI
Last modified: 2012-10-24 05:19:00 UTC
GLib's GMainLoop and GMainContext work fine through introspection. We should drop our static bindings, which are now just duplication as well as they don't provide the full API (e. g. g_main_context_is_owner () is not available). This is part of the "remove static bindings" cleanup in meta bug 685373.
Created attachment 226797 [details] [review] WIP: Remove static MainLoop and MainContext bindings This is a first attempt at a patch. GMainLoop and GMainContext work very well. However, this currently breaks GSource (as it needs to instantiate GMainContexts), so do not apply yet! It also needs to put back the GObject.Main{Loop,Context} aliases, presumably in a new GObject override. I attach this early version for comments whether doing this is a good idea or whether we need the static bindings for some reason (Note that gjs does fine without static bindings).
I'm stuck with this right now, and I need some help with this. Above patch disables pyg_source_attach() and pyg_source_get_context() (search for "FIXME"), as they would now need to handle the dynamic GI objects for GLib.MainContext, not the static PyGMainContext any more. But I don't know a way how to use GI stuff in gi/_glib/, it seems I can't use any of the GI headers there. I also tried to drop the static bindings for GLib.Source, GLib.Idle, and GLib.Timeout; the latter work fine with tiny overrides to keep the backwards compat API, but the generic GSource API is not introspectable (g_source_new needs a struct with four function pointers with callbacks, etc.) I'm currently trying to add a more binding friendly API to glib, but that is a bigger undertaking. So I'd appreciate a hint how I could rewite this part to use the dynamic GI MainContext, any ideas? ------------- 8< -------------- PyGMainContext *py_context = NULL; GMainContext *context = NULL; if (!PyArg_ParseTupleAndKeywords (args, kwargs, "|O!:attach", kwlist, &PyGMainContext_Type, &py_context)) return NULL; if (py_context) context = py_context->context; ------------- 8< -------------- Thanks in advance!
Created attachment 226825 [details] [review] WIP: Remove static GSource bindings Just to avoid accidentally losing this, this is my experimental patch for removing the static GSource bindings. The GSource override constructor currently just returns a GLib.timeout_source_new(1) to have something which at least runs most of the tests, but this of course needs to be replaced by a bindable GLib API. So please disregard this patch for now.
Hi Martin, This is great and I hope a lot of the static bindings can go this direction. I did a little bit of investigative work and it looks like a MainContext is derived from gobject.GBoxed. >>> GLib.MainContext.mro() [gi.repository.GLib.MainContext, gi.Boxed, gobject.GBoxed, builtins.object] This means you should be able to pull the boxed type off of the PyObject using the pyg_boxed_check and pyg_boxed_get macros. And then finally cast it to a GMainContext pointer? As for going the other direction (pyg_source_get_context), I think, but am not sure that you will need to somehow call gi/pygi-boxed.c:_pygi_boxed_new or one of the marshalers wrapping it.
(In reply to comment #4) > This means you should be able to pull the boxed type off of the PyObject using > the pyg_boxed_check and pyg_boxed_get macros. Right, but for things like those I keep hitting the "it seems I can't use any of the GI headers there" boundaries -- PyGBoxed is in gi/_gobject (and thus in a separate library), while PyGSource is in gi/_glib; both directories build separate libraries and have a separate set of header files. > As for going the other direction (pyg_source_get_context), I think, but am not > sure that you will need to somehow call gi/pygi-boxed.c:_pygi_boxed_new or one > of the marshalers wrapping it. I can't use this from gi/_glib/ either. So it seems in order to do this we first need some refactoring to move GSource into gi/, so that it can use the introspection API?
Created attachment 226970 [details] [review] WIP: Remove static MainLoop, MainContext, and GSource bindings That's better. I completely dropped gi/_glib/pygsource.c and put the necessary static parts into gi/pygi-source.[hc], so that they can use the GI API. I dropped the custom PyGSource type, and only keep a static wrapper pyg_source_new() (we need that as g_source_new() is not bindable), and pyg_source_set_callback() (so that our C source function pyg_source_dispatch() can call the Python callback and has all the user data, destroy notify etc. encoded in the user_data argument). I have a feeling that pyg_source_set_callback() could be dropped, if pyg_source_dispatch() is able to get the Python callback and user data from its "real" arguments; but I don't know enough about the guts of pygobject yet to do that. For now this is mostly a code copy from the original gi/_glib/pygsource.c. All GSource test cases work now. TODO: - Eliminate gi/_glib/pygsource.h (this still defines PyGPollFD which is used in gi/_glib/pygiochannel.c). I propose we leave that to a separate commit and bug; presumably/hopefully we can completely drop these static bindings as well. - - test_mainloop.TestMainLoop.test_concurrency segfaults for full test suite run; it works fine if I just run test_mainloop; I'm investigating this now.
Created attachment 226973 [details] [review] WIP: Remove static MainLoop, MainContext, and some GSource bindings The previous patch was a bit overzealous. This puts back the PollFD parts of gi/_glib/pygsource.c, as these are still being used in gi/_glib/pygiochannel.c. Unfortunately we don't have any IOChannel tests, so I didn't notice that at first. Before this, and a future dropping of gi/_glib/pygiochannel.c get committed, we should add IOChannel tests. This updated patch also fixes the "TODO" in the previous version, and checks the type of the second argument of pyg_source_set_callback().
I found the reason for the failing main_loop test with this patch. It was due to test_source's GSources which were never cleaned up. I filed that as bug 686627 and sent a patch there.
Created attachment 226984 [details] [review] Remove static MainLoop, MainContext, and some GSource bindings I did some final cleanups and a proper changelog now. With the patch in bug 686627 applied, the whole test suite succeeds now. Reviews and comments appreciated!
Do you expect any performance impacts?
(In reply to comment #10) > Do you expect any performance impacts? By nature the GI bindings are a tad slower than static ones, due to more dynamic marshalling. However, the GI bindings go through quite some effort to cache as much as possible, so the impact should be negligible. Especially as constructing main loops or sources isn't something you usually do thousands of times in a second.
(In reply to comment #11) > (In reply to comment #10) > > Do you expect any performance impacts? > > By nature the GI bindings are a tad slower than static ones, due to more > dynamic marshalling. However, the GI bindings go through quite some effort to > cache as much as possible, so the impact should be negligible. Especially as > constructing main loops or sources isn't something you usually do thousands of > times in a second. How about adding and removing sources / timeouts? this is not an uncommon workload for threaded / io heavy apps
(In reply to comment #12) > How about adding and removing sources / timeouts? this is not an uncommon > workload for threaded / io heavy apps You could say the same for GdkPixbuf, Gtk widgets, or GStreamer elements. If you need raw speed, then C or Vala is the better choice anyway, given Python's performance cost in general (~ factor 30). I wouldn't keep static bindings, with all their incompleteness (lots of current GLib API is missing) and bugs (bug 684526 is a nice example, this patch fixes it as well by entirely removing the function), just for performance reasons.
Created attachment 227037 [details] benchmark I wrote a small benchmark testing MainLoop, MainContext, and Idle. With current git head (-O0) I get from three runs: MainLoop/MainContext 1000000 iterations: 4.950000 s Idle 1000000 iterations: 2.780000 s MainLoop/MainContext 1000000 iterations: 4.840000 s Idle 1000000 iterations: 2.770000 s MainLoop/MainContext 1000000 iterations: 4.780000 s Idle 1000000 iterations: 2.680000 s With this patch applied (GI bindings) I get: MainLoop/MainContext 1000000 iterations: 28.440000 s Idle 1000000 iterations: 35.060000 s MainLoop/MainContext 1000000 iterations: 27.910000 s Idle 1000000 iterations: 34.580000 s MainLoop/MainContext 1000000 iterations: 28.720000 s Idle 1000000 iterations: 34.270000 s So indeed GI bindings are one magnitude slower.
Created attachment 227038 [details] [review] Remove static MainLoop, MainContext, and some GSource bindings Rebased against current git, and this also removes the corresponding bits from docs/reference/pyglib-functions.xml.
Created attachment 227040 [details] [review] Remove static MainLoop, MainContext, and some GSource bindings Rebased against master (after the initial cleanup of _glib in http://git.gnome.org/browse/pygobject/commit/?id=fb473b3), and also removed the corresponding obsolete documentation files.
Created attachment 227042 [details] [review] Mark GLib API that is exposed in GObject as deprecated Followup patch to mark GLib API that is exposed in GObject as deprecated.
Review of attachment 227040 [details] [review]: Looks good. I mostly just have comments about unittests. We should have test coverage of everything being removed (ideally with coverage that includes error conditions and refcounts where applicable). The tests should work the same before and after the removal (which I see you've done in many cases already). I went through and grepped for each of the removed function names in the tests directory and found a few that seemed to be missing (although it could be an error on my part). Would it make sense for tests go in a separate commit applied before these changes? ::: gi/_glib/pygmainloop.c @@ -266,3 @@ - self->loop = g_main_loop_new(context, is_running); - - self->signal_source = pyg_signal_watch_new(); Is the same signal watching functionality available in the gi version? I don't know what this is for but is it tested? ::: gi/_glib/pygsource.c @@ -188,3 @@ - -static PyObject * -} Needs unittest. @@ -205,3 @@ - -static PyObject * - Py_DECREF(pysource->obj); Needs unittest. @@ -231,3 @@ - -static PyObject * - PyObject *result; Needs unittest. @@ -257,3 @@ - -static PyObject * -{ Needs unittest. @@ -360,3 @@ - {"priority", (getter)pyg_source_get_priority, (setter)pyg_source_set_priority }, - {"can_recurse", (getter)pyg_source_get_can_recurse, (setter)pyg_source_set_can_recurse }, - return pyg_main_context_new(context); The above removed properties will need overrides as direct python properties and unittests to ensure the same API with what is being removed.
(In reply to comment #14) > Created an attachment (id=227037) [details] > benchmark > > I wrote a small benchmark testing MainLoop, MainContext, and Idle. With current > git head (-O0) I get from three runs: Thank you for quantifying this; the data is much more useful than your last comment. I write performance and scientific python code for a living and could do without the lecture about the trade offs in python performance. As you say, one order of magnitude is the same as moving to Vala. A shame really.
(In reply to comment #18) > Would it make sense for tests go in a separate commit applied before these > changes? Yes, I think so. It needs to be reduced a bit, as some of the MainContext API is not provided by the static bindings (the main reason why we want to get rid of them). I committed http://git.gnome.org/browse/pygobject/commit/?id=3dba328010a4ffd9259700ffec95871c7341d491 > - self->signal_source = pyg_signal_watch_new(); > > Is the same signal watching functionality available in the gi version? I don't > know what this is for but is it tested? From reading pyg_signal_watch_check(), it is to quit the running main loop if the program receives a SIGINT (i. e. pressing Control-C). I added a test case for this ('test_sigint') to tests/test_mainloop.py. > pyg_source_get_context > Needs unittest. Done in above commit. > pyg_source_{add,remove}_poll > Needs unittest. The current API doesn't even work with Python 3:
+ Trace 231079
source.add_poll(pollfd)
For Python 2 it can still be polled, but I'm unable to actually make this work: I attempted to write a test (http://pastebin.com/CUYUeAte), but it never sees the added FD in the GMainContext iteration. Any hint appreciated. I only added a shallow test for now which only verifies that GLib.PollFD can be instantiated and calling add_poll()/remove_poll() works without errors. This should already provide enough coverage for the static -> GI conversion, though. I marked it as expectFailure, as it won't work with py3. This should be dropped with the next revision of the patch for this bug. > pyg_source_get_current_time > Needs unittest. Done in above commit. > @@ -360,3 @@ > - {"priority", (getter)pyg_source_get_priority, > (setter)pyg_source_set_priority }, > - {"can_recurse", (getter)pyg_source_get_can_recurse, > (setter)pyg_source_set_can_recurse }, > - return pyg_main_context_new(context); > > The above removed properties will need overrides as direct python properties > and unittests to ensure the same API with what is being removed. Covered in above commit.
Created attachment 227051 [details] [review] Remove static MainLoop, MainContext, and some GSource bindings This patch now fixes the remaining details (missing properties, SIGINT handling of main loops, what Simon pointed out) and all tests succeed with both Python 3.2 and 2.7. As the full API coverage of main loop etc. was committed to trunk already, this updated patch just adds back the tests which use API that is not available in the static bindings (such as GLib.MainContext.default() or GLib.MainContext.is_owner()), and drops the @unittest.expectedFailure from testAddRemovePoll(), as this works with Python 3 now.
Created attachment 227054 [details] [review] Mark GLib.Source.get_current_time() as deprecated Note that the second patch (https://bugzilla.gnome.org/attachment.cgi?id=227042&action=edit) is still valid, and goes on top of "Remove static MainLoop...". This third patch goes on top of "Mark GLib API that is exposed in GObject as deprecated" and additionally marks GLib.Source.get_current_time() as deprecated.
Created attachment 227119 [details] [review] Remove static MainLoop, MainContext, and some GSource bindings Simon pointed out that we still had the static PRIORITY_* definitions, this updated patch drops those as well. It also adds two missing overrides (GMainContext.iteration() with default argument and MainLoop constructor with a MainContext instance), which I found during testing. I tested this now with a range of big and small PyGI applications (Ubuntu software-center, apport, Rhythmbox plugins, Ubuntu software-properties, gtimelog), and they are now all working smoothly.
Review of attachment 227119 [details] [review]: Nice work!
Review of attachment 227042 [details] [review]: Looks good.
Review of attachment 227054 [details] [review]: Looks good.
Comment on attachment 227042 [details] [review] Mark GLib API that is exposed in GObject as deprecated All pushed. Thanks for the review!