Howdy, I'm working on wrapping a C++ library that heavily uses threads internally, and makes callbacks to user-defined functions on various non-user-created threads. (This is basically the situation that provided the impetus for PEP 311, after discussion on python-dev -- note that I'm working with Python 2.3 because of this, but I beileve that the PEP 311 patch could be back-ported to 2.2 should anyone need it.) However, I'm running into problems getting the correct wrappers for my classes, to get the various Py_BEGIN_ALLOW_THREADS/Py_END_ALLOW_THREADS and PyGILState_Ensure/PyGILState_Release combos in the various places. A little background about my problem: from an IDL for a Foo interface, I generate two C++ classes, "Foo" and "FooProxy". Both of these are wrapped in smart pointers and are always passed as such in the C++ library, so we have FooHandle and FooProxyHandle. If Foo defines a "bar();" operation, then class Foo will have a "virtual void bar() = 0;", and I have to subclass and implement it, from python or C++. I have a wrapper class that does the call_method<>() dance; in this wrapper class, I also place PyGILState_Ensure/PyGILState_Release around the call_method<>() call. FooProxy is a remote object that I get as a result of calling various functions to get a proxy -- it has a "void bar();" (non-virtual) that I can call. However, because the call can be made synchronously, I need to wrap the call to bar() with Py_BEGIN_ALLOW_THREADS and Py_END_ALLOW_THREADS. It's this problem that I'm directly dealing with -- I can't figure out a way to wrap class FooProxy such that I can manage the current ThreadState and the GIL around them. I can create another wrapper class that does the right thing and calls FooProxy::bar(), but functions are returning a FooProxy (not mywrapper_for_FooProxy), and I already have a Handle<FooProxy> as a wrapper specified in the boost::python class_... so I end up with incorrect types half the time. However, an alternative solution that I was thinknig of is to bake the thread smarts directly into boost::python. It seems to me that call_method and similar should always be wrapped with a PyGILState_Ensure/PyGILState_Release. Also, an additional call policy could be added, something like "allow_threads", which would cause b::p to call Py_BEGIN_ALLOW_THREADS and Py_END_ALLOW_THREADS around the function invocation on C++. This seems like a much more generally useful solution than me trying to do the dance in wrapper classes. Is this the right way to go? If so, I'd appreciate any pointers on implementing a new call policy, especially if it's possible to both specify allow_threads and a return_value_policy. Thanks, - Vladimir
However, an alternative solution that I was thinknig of is to bake the thread smarts directly into boost::python. It seems to me that call_method and similar should always be wrapped with a PyGILState_Ensure/PyGILState_Release.
I guess you can do that without a problem?
Also, an additional call policy could be added, something like "allow_threads", which would cause b::p to call Py_BEGIN_ALLOW_THREADS and Py_END_ALLOW_THREADS around the function invocation on C++. This seems like a much more generally useful solution than me trying to do the dance in wrapper classes.
Is this the right way to go? If so, I'd appreciate any pointers on implementing a new call policy, especially if it's possible to both specify allow_threads and a return_value_policy.
Look here: http://boost.org/libs/python/doc/v2/CallPolicies.html It should be very easy for you to write a call policy. And than you can just use it like that return_value_policy<copy_const_reference, your_new_threading_policy>()
Thanks, - Vladimir
Nikolay Mladenov wrote:
However, an alternative solution that I was thinknig of is to bake the thread smarts directly into boost::python. It seems to me that call_method and similar should always be wrapped with a PyGILState_Ensure/PyGILState_Release.
I guess you can do that without a problem?
Yeah, though I'm having a slight issue with call_method invocations where the return-type is void, since I need to split up the conversion and the actual "return r;" call.
Also, an additional call policy could be added, something like "allow_threads", which would cause b::p to call Py_BEGIN_ALLOW_THREADS and Py_END_ALLOW_THREADS around the function invocation on C++. This seems like a much more generally useful solution than me trying to do the dance in wrapper classes.
Is this the right way to go? If so, I'd appreciate any pointers on implementing a new call policy, especially if it's possible to both specify allow_threads and a return_value_policy.
Look here: http://boost.org/libs/python/doc/v2/CallPolicies.html It should be very easy for you to write a call policy. And than you can just use it like that
return_value_policy<copy_const_reference, your_new_threading_policy>()
Whoops, I completely missed the overview concept page.. thank you, looks straightforward enough. However, in looking at the code more, I don't think that a call policy would do it -- since no Py_* API functions can be called betwen BEGIN/END macros, they need to go around the final call into C++, thus need to be inserted directly into invoke.hpp. The changes aren't much, but I'm getting a crash-course in boost::python innards :) Thanks, - Vlad
Vladimir Vukicevic wrote:
Nikolay Mladenov wrote:
However, an alternative solution that I was thinknig of is to bake the thread smarts directly into boost::python. It seems to me that call_method and similar should always be wrapped with a PyGILState_Ensure/PyGILState_Release.
I guess you can do that without a problem?
Yeah, though I'm having a slight issue with call_method invocations where the return-type is void, since I need to split up the conversion and the actual "return r;" call.
Also, an additional call policy could be added, something like "allow_threads", which would cause b::p to call Py_BEGIN_ALLOW_THREADS and Py_END_ALLOW_THREADS around the function invocation on C++. This seems like a much more generally useful solution than me trying to do the dance in wrapper classes.
Is this the right way to go? If so, I'd appreciate any pointers on implementing a new call policy, especially if it's possible to both specify allow_threads and a return_value_policy.
Look here: http://boost.org/libs/python/doc/v2/CallPolicies.html It should be very easy for you to write a call policy. And than you can just use it like that
return_value_policy<copy_const_reference, your_new_threading_policy>()
Whoops, I completely missed the overview concept page.. thank you, looks straightforward enough. However, in looking at the code more, I don't think that a call policy would do it -- since no Py_* API functions can be called betwen BEGIN/END macros,
I didn't see this in the python docs?
they need to go around the final call into C++, thus need to be inserted directly into invoke.hpp.
And how can you be sure that the c++ call won't call back to python?
The changes aren't much, but I'm getting a crash-course in boost::python innards :)
Thanks, - Vlad
Nikolay Mladenov <nickm@sitius.com> writes:
However, an alternative solution that I was thinknig of is to bake the thread smarts directly into boost::python. It seems to me that call_method and similar should always be wrapped with a PyGILState_Ensure/PyGILState_Release.
I guess you can do that without a problem?
Also, an additional call policy could be added, something like "allow_threads", which would cause b::p to call Py_BEGIN_ALLOW_THREADS and Py_END_ALLOW_THREADS around the function invocation on C++. This seems like a much more generally useful solution than me trying to do the dance in wrapper classes.
Is this the right way to go? If so, I'd appreciate any pointers on implementing a new call policy, especially if it's possible to both specify allow_threads and a return_value_policy.
Look here: http://boost.org/libs/python/doc/v2/CallPolicies.html It should be very easy for you to write a call policy. And than you can just use it like that
return_value_policy<copy_const_reference, your_new_threading_policy>()
I don't believe that call policies can do what he wants yet, since there's no provision for state to be carried across the precall() and postcall() functions as required by Py_BEGIN_ALLOW_THREADS and Py_END_ALLOW_THREADS. Some kind of redesign would probably be in order. -- Dave Abrahams Boost Consulting www.boost-consulting.com
David Abrahams wrote:
I don't believe that call policies can do what he wants yet, since there's no provision for state to be carried across the precall() and postcall() functions as required by Py_BEGIN_ALLOW_THREADS and Py_END_ALLOW_THREADS.
Yeah, ok. But this seems to be a bug, since the docs say x is obbject of type P - model of CallPolicy: than x.precall() .... x.postcall() and x should be able to carry the thread state. The fact that most call policies are static and cannot chain this type of calls should probably be fixed?
Nikolay Mladenov <nickm@sitius.com> writes:
David Abrahams wrote:
I don't believe that call policies can do what he wants yet, since there's no provision for state to be carried across the precall() and postcall() functions as required by Py_BEGIN_ALLOW_THREADS and Py_END_ALLOW_THREADS.
Yeah, ok. But this seems to be a bug, since the docs say x is obbject of type P - model of CallPolicy: than x.precall() .... x.postcall()
Yes; that is not a bug.
and x should be able to carry the thread state.
Not if the wrapped function is re-entered. The docs don't say anything about when the policies object is created. In fact each wrapped function has a single instance of the policies object.
The fact that most call policies are static and cannot chain this type of calls should probably be fixed?
Maybe so, but that's not enough to fix the problem. -- Dave Abrahams Boost Consulting www.boost-consulting.com
Vladimir, In addition to my earlier responses I'd like to point you at this posting from Patrick Hartling. http://article.gmane.org/gmane.comp.python.c++/2833 I'd really like to get all these threading issues squared away in Boost.Python, and I especially hope that since threading is so important in lua that threading integration will benefit from the luabind discussions. In any case, proposals for patches to the Boost.Python core are very much appreciated. -Dave -- Dave Abrahams Boost Consulting www.boost-consulting.com
Vladimir, In addition to my earlier responses I'd like to point you at this posting from Patrick Hartling. http://article.gmane.org/gmane.comp.python.c++/2833 I'd really like to get all these threading issues squared away in Boost.Python, and I especially hope that since threading is so important in lua that threading integration will benefit from the luabind discussions. In any case, proposals for patches to the Boost.Python core are very much appreciated. -Dave -- Dave Abrahams Boost Consulting www.boost-consulting.com
David Abrahams wrote:
Vladimir,
In addition to my earlier responses I'd like to point you at this posting from Patrick Hartling.
http://article.gmane.org/gmane.comp.python.c++/2833
I'd really like to get all these threading issues squared away in Boost.Python, and I especially hope that since threading is so important in lua that threading integration will benefit from the luabind discussions. In any case, proposals for patches to the Boost.Python core are very much appreciated.
Thanks for the article pointer.. I see what you meant by the guard class earlier. Patrick seems to have basically implemented (in a fairly elegant way!) what the Python 2.3 PyGILStateEnsure/Release functions do; my patches to call.hpp and call_method.hpp do virtually the same thing, by creating a call_method_with_gil that one can use in wrappers instead of call_method. The difficulty seems to be calling from python into C++, in invoke.hpp. My patch took a first stab at it, but I didn't look closely enough -- the arguments passed to f() in the invoke() calls are all arg converters. What I'd really like to have happen is for a function foo, to have an invoke() and an invoke_helper, where the invoke_helper has the exact same signature as foo but inserts Py_BEGIN_ALLOW_THREADS and Py_END_ALLOW_THREADS around the actual call to foo() (or, more accurately, the pointer to foo). This can't happen in the actual invoke() call, as my patch attempts to do, because the arg converters need to use Python API calls (oops!). I'm working on adding an invoke_helper which will do the two-step invocation, but I need to study the to_python interface in more detail. - Vlad
Vladimir Vukicevic <vladimir@pobox.com> writes:
David Abrahams wrote:
Vladimir, In addition to my earlier responses I'd like to point you at this posting from Patrick Hartling. http://article.gmane.org/gmane.comp.python.c++/2833
I'd really like to get all these threading issues squared away in Boost.Python, and I especially hope that since threading is so important in lua that threading integration will benefit from the luabind discussions. In any case, proposals for patches to the Boost.Python core are very much appreciated.
Thanks for the article pointer.. I see what you meant by the guard class earlier. Patrick seems to have basically implemented (in a fairly elegant way!) what the Python 2.3 PyGILStateEnsure/Release functions do; my patches to call.hpp and call_method.hpp do virtually the same thing, by creating a call_method_with_gil that one can use in wrappers instead of call_method.
The difficulty seems to be calling from python into C++, in invoke.hpp. My patch took a first stab at it, but I didn't look closely enough -- the arguments passed to f() in the invoke() calls are all arg converters. What I'd really like to have happen is for a function foo, to have an invoke() and an invoke_helper, where the invoke_helper has the exact same signature as foo but inserts Py_BEGIN_ALLOW_THREADS and Py_END_ALLOW_THREADS around the actual call to foo() (or, more accurately, the pointer to foo). This can't happen in the actual invoke() call, as my patch attempts to do, because the arg converters need to use Python API calls (oops!). I'm working on adding an invoke_helper which will do the two-step invocation, but I need to study the to_python interface in more detail.
I think the key here is to find a way to wrap the wrapped (member) function pointer f in a function *adapter* which does Py_BEGIN_ALLOW_THREADS/Py_END_ALLOW_THREADS. -- Dave Abrahams Boost Consulting www.boost-consulting.com
Vladimir Vukicevic <vladimir@pobox.com> writes:
Howdy,
I'm working on wrapping a C++ library that heavily uses threads internally, and makes callbacks to user-defined functions on various non-user-created threads. (This is basically the situation that provided the impetus for PEP 311, after discussion on python-dev --
Yes, I was a prime instigator of that discussion because users were having difficulty wrapping functions which call into QT, which in turn may invoke PyQT callbacks, which, because they may be called on a thread, would unconditionally acquire the GIL. If users' functions didn't release the GIL before calling into QT, it was very bad juju.
note that I'm working with Python 2.3 because of this, but I beileve that the PEP 311 patch could be back-ported to 2.2 should anyone need it.) However, I'm running into problems getting the correct wrappers for my classes, to get the various Py_BEGIN_ALLOW_THREADS/Py_END_ALLOW_THREADS and PyGILState_Ensure/PyGILState_Release combos in the various places.
A little background about my problem: from an IDL for a Foo interface, I generate two C++ classes, "Foo" and "FooProxy". Both of these are wrapped in smart pointers and are always passed as such in the C++ library, so we have FooHandle and FooProxyHandle. If Foo defines a "bar();" operation, then class Foo will have a "virtual void bar() = 0;", and I have to subclass and implement it, from python or C++. I have a wrapper class that does the call_method<>() dance; in this wrapper class, I also place PyGILState_Ensure/PyGILState_Release around the call_method<>() call.
Sounds right.
FooProxy is a remote object that I get as a result of calling various functions to get a proxy -- it has a "void bar();" (non-virtual) that I can call.
Hmm. This is totally off-topic, but it seems odd to me (and inconvenient, in C++) that FooProxy is not derived from Foo.
However, because the call can be made synchronously, I need to wrap the call to bar() with Py_BEGIN_ALLOW_THREADS and Py_END_ALLOW_THREADS.
Well, not exactly, IIUC. The reasons to wrap it in Py_BEGIN_ALLOW_THREADS and Py_END_ALLOW_THREADS are: 1. Because it might take a long time and you want other threads to be able to use the Python interpreter. 2. Because the call actually *depends* on other threads using the Python interpreter during its execution in order to be able to reach completion.
It's this problem that I'm directly dealing with -- I can't figure out a way to wrap class FooProxy such that I can manage the current ThreadState and the GIL around them. I can create another wrapper class
Do you mean like a virtual-function-dispatching class, the kind with the initial "PyObject* self" argument to its constructors?
that does the right thing and calls FooProxy::bar(), but functions are returning a FooProxy (not mywrapper_for_FooProxy)
I thought you said that all the C++ interfaces passed FooProxyHandle (?)
and I already have a Handle<FooProxy> as a wrapper specified in the boost::python class_... so I end up with incorrect types half the time.
Specifically, what problem are you having? Could you show a function you are unable to wrap properly and describe the specific problems you're having?
However, an alternative solution that I was thinknig of is to bake the thread smarts directly into boost::python.
I have always thought that should happen in some form or other anyway.
It seems to me that call_method and similar should always be wrapped with a PyGILState_Ensure/PyGILState_Release.
Some people know their extensions are only being called on the main thread and not everyone will be ready to pay the cost for acquiring the GIL on every callback. IMO supplying a GILState class which acquires on construction and releases on destruction *should* be enough.
Also, an additional call policy could be added, something like "allow_threads", which would cause b::p to call Py_BEGIN_ALLOW_THREADS and Py_END_ALLOW_THREADS around the function invocation on C++. This seems like a much more generally useful solution than me trying to do the dance in wrapper classes.
I think some change to the requirements and usage of CallPolicies would be neccessary in order to support that (see my reply to Nikolay), but I agree with the general idea.
Is this the right way to go?
Probably. I also think that some people want automatic Py_BEGIN_ALLOW_THREADS/Py_END_ALLOW_THREADS for *all* of their wrapped functions, and that should probably be configurable, too. You have to be careful with that, though: once you do Py_BEGIN_ALLOW_THREADS you can't use any Boost.Python facilities which use the Python 'C' API (e.g. object, handle<>, ...) until you reach Py_END_ALLOW_THREADS.
If so, I'd appreciate any pointers on implementing a new call policy, especially if it's possible to both specify allow_threads and a return_value_policy.
I think you'd better start by looking at the macro definition of Py_BEGIN_ALLOW_THREADS et al and then at how policies are used to see how state can be stored for the duration of the call they affect, as the macros are wont to do. -- Dave Abrahams Boost Consulting www.boost-consulting.com
David Abrahams wrote:
FooProxy is a remote object that I get as a result of calling various functions to get a proxy -- it has a "void bar();" (non-virtual) that I can call.
Hmm. This is totally off-topic, but it seems odd to me (and inconvenient, in C++) that FooProxy is not derived from Foo.
Live servants and proxies live in different class hierarchies; the Proxy objects just know how to add their tag to a packet and serialize arguments to operations, whereas Foo might have additional data members and the like. (The library in question is the Ice communications library, at http://www.zeroc.com/ .)
However, because the call can be made synchronously, I need to wrap the call to bar() with Py_BEGIN_ALLOW_THREADS and Py_END_ALLOW_THREADS.
Well, not exactly, IIUC. The reasons to wrap it in Py_BEGIN_ALLOW_THREADS and Py_END_ALLOW_THREADS are:
1. Because it might take a long time and you want other threads to be able to use the Python interpreter.
2. Because the call actually *depends* on other threads using the Python interpreter during its execution in order to be able to reach completion.
Right; I have both cases happening -- i.e. there's a "waitForShutdown();" that you can call on the main message dispatcher -- if you call that without releasing the GIL, you shoot yourself in the foot :). The case #2 is what I'm trying to deal with now, as I have a RPC invoked by process B on process A, and then in A's handler it attempts to execute an rpc on process B -- and it blocks, since B is still waiting for the completion of the original call (while holding the GIL).
It's this problem that I'm directly dealing with -- I can't figure out a way to wrap class FooProxy such that I can manage the current ThreadState and the GIL around them. I can create another wrapper class
Do you mean like a virtual-function-dispatching class, the kind with the initial "PyObject* self" argument to its constructors?
Yes.
that does the right thing and calls FooProxy::bar(), but functions are returning a FooProxy (not mywrapper_for_FooProxy)
I thought you said that all the C++ interfaces passed FooProxyHandle (?)
Sorry, my mistake.. it is a FooProxyHandle. Internally FooProxy is typedef'd to ::Ice::ProxyHandle< ::IceProxy::Foo> :)
and I already have a Handle<FooProxy> as a wrapper specified in the boost::python class_... so I end up with incorrect types half the time.
Specifically, what problem are you having? Could you show a function you are unable to wrap properly and describe the specific problems you're having?
Sure; see end of message, as it's somewhat lengthy and I didn't want to clutter up the replies. (*)
However, an alternative solution that I was thinking of is to bake the thread smarts directly into boost::python.
I have always thought that should happen in some form or other anyway.
It seems to me that call_method and similar should always be wrapped with a PyGILState_Ensure/PyGILState_Release.
Some people know their extensions are only being called on the main thread and not everyone will be ready to pay the cost for acquiring the GIL on every callback. IMO supplying a GILState class which acquires on construction and releases on destruction *should* be enough.
Hmm, I'm not sure what you mean; where would you apply the GILState class? I agree though, acquiring the GIL on every callback is heavyhanded, but necessary for some applications (like mine, where I don't know exactly which thread a callback will get invoked on for a particular object instance).
Also, an additional call policy could be added, something like "allow_threads", which would cause b::p to call Py_BEGIN_ALLOW_THREADS and Py_END_ALLOW_THREADS around the function invocation on C++. This seems like a much more generally useful solution than me trying to do the dance in wrapper classes.
I think some change to the requirements and usage of CallPolicies would be neccessary in order to support that (see my reply to Nikolay), but I agree with the general idea.
Is this the right way to go?
Probably. I also think that some people want automatic Py_BEGIN_ALLOW_THREADS/Py_END_ALLOW_THREADS for *all* of their wrapped functions, and that should probably be configurable, too. You have to be careful with that, though: once you do Py_BEGIN_ALLOW_THREADS you can't use any Boost.Python facilities which use the Python 'C' API (e.g. object, handle<>, ...) until you reach Py_END_ALLOW_THREADS.
Yep; I assume invoke.hpp is the lowest-level at which the C++ function is actually invoked. I'd like to do the wrapping in caller.hpp, at which point I could extend call policies, but invoke() uses a result converter which will need to make Python API calls. So I wrap the actual call to f() in invoke.hpp. The only wrinkle I'm having now is in call<> and call_method<>, where the return type is void (because the compiler doesn't like R _r = baz(); return _r; where R is void :) -- I've tried, unsuccessfully, to specialize these functions for return types of void, but I'm afraid my C++-fu isn't making the grade.
If so, I'd appreciate any pointers on implementing a new call policy, especially if it's possible to both specify allow_threads and a return_value_policy.
I think you'd better start by looking at the macro definition of Py_BEGIN_ALLOW_THREADS et al and then at how policies are used to see how state can be stored for the duration of the call they affect, as the macros are wont to do.
* Specific function problem: Each ProxyHandle<Proxy::Foo> exports (via the ProxyHandle class) a checkedCast() static member, that can be used to convert ProxyHandle<Proxy::Object> (which is the root of all proxies) to a specific proxy type. checkedCast() checkedcast actually creates a new ProxyHandle<Proxy::Foo> object, and then copies the target server information into it from the ProxyHandle<Proxy::Object>, so there's no "cast" in the C++ sense going on (though it does check if it can dynamic_cast<> the Proxy::Object* to a Proxy::Foo*, and if so, just does that). To have support for checkedCast in python, I create wrapper functions such as: namespace pyce_Hello_casts { ::IceInternal::ProxyHandle< ::IceProxy::Hello> _Hello_checkedCast (const ::Ice::ObjectPrx& o) { return ::IceInternal::ProxyHandle< ::IceProxy::Hello>::checkedCast (o); } }; and my class_<> def looks like: class_< ::IceProxy::Hello , ::IceInternal::ProxyHandle< ::IceProxy::Hello >, bases< ::IceProxy::Ice::Object >, boost::noncopyable > ("HelloPrx") .def("sayHello", &::IceProxy::Hello::sayHello) .def("checkedCast", pyce_Hello_casts::_Hello_checkedCast) .staticmethod("checkedCast") ; This all works fine, until I want to intercept the call to sayHello() (to save/restore the ThreadState goop). I can create a wrapper that derives from ::IceProxy::Hello, but then I have to deal with that PyObject* constructor, for which I have no need for here -- I'm never going to utilize call_method, and HelloPrx will never be derived from in python-land. The sayHello() and other member functions are also not virtual. I could do a dance with a custom-written overload dispatch for each function. If I went the wrapper route, I would need to implement my own version of checkedCast that can convert to my wrapper, and add some implicitly_convertible statements, and make sure that no references to ::IceProxy::Hello (or the equivalent handle) get exposed anywhere.. certainly feasable, but a pain, especially for cases where I get a handle to a ::IceProxy::Hello in C++ and I'd have to convert. But both approaches seem like they're far too verbose. I'll probably create a call_method_with_threads<> such that the non-GIL-acquiring call_method<> is still available; extending call policies would allow a nice implementation for GIL-releasing and non-GIL-releasing C++ calls in caller.hpp, but the problem with the ResultConverter being called from invoke.hpp still remains. If there was a base class for all result converters, it could perhaps acquire the GIL in its constructor and release in the destructor, though no such class exists (that I can see?), and it would be yet another mutex acquisition/release. I'm currently running into a snag in invoke.hpp, though; I've modified the non-void-return calls to invoke as such: template <class RC, class F BOOST_PP_ENUM_TRAILING_PARAMS_Z(1, N, class AC)> inline PyObject* invoke(fn_tag, RC*, F& f BOOST_PP_ENUM_TRAILING_BINARY_PARAMS_Z(1, N, AC, & ac) ) { PyThreadState *_save; Py_UNBLOCK_THREADS typename boost::function_traits<F>::result_type _r = f( BOOST_PP_ENUM_BINARY_PARAMS_Z(1, N, ac, () BOOST_PP_INTERCEPT) ); Py_BLOCK_THREADS return RC()(_r); } .. but the function_traits invocation doesn't seem to be doing what I want, as I'm getting errors of the form: /usr/local/boost/boost/python/detail/invoke.hpp:105: base class ` boost::detail::function_traits_helper<Ice::ObjectPtr (Ice::Stream::**)(const std::string&, const std::string&, const Ice::ObjectFactoryPtr&)>' has incomplete type I apologize for the long message; I hope I've come across clearer instead of muddying up the waters more. Thanks for the help and ideas! - Vlad
Attached is a patch which seems to work for me, under some light testing. I'm writing some heavy stress-test code right now. I'm very open to suggestions on how to do away with call_void_method_with_gil/call_void_with_gil so that call_method_with_gil and call_with_gil can be used with void-returning functions as well. Note that I'm not necessarily submitting this for review for possible inclusion in b::p, but am more curious if this approach is valid -- though if there's interest I'd be willing to do any work necessary to make it ready. I think with some #ifdefs it could play nice with cases where the user doesn't care about the GIL and other issues; however, it does depend on Python 2.3. - Vlad ? libs/python/build/.jamdeps ? libs/python/build/bin ? libs/python/build/bin-stage ? libs/python/example/tutorial/.jamdeps Index: boost/python/call.hpp =================================================================== RCS file: /cvsroot/boost/boost/boost/python/call.hpp,v retrieving revision 1.15 diff -u -w -b -u -r1.15 call.hpp --- boost/python/call.hpp 31 May 2003 14:53:01 -0000 1.15 +++ boost/python/call.hpp 6 Jul 2003 22:51:08 -0000 @@ -30,10 +30,18 @@ # define BOOST_PYTHON_FAST_ARG_TO_PYTHON_GET(z, n, _) \ , converter::arg_to_python<A##n>(a##n).get() +# define BOOST_PYTHON_CONVERT_POBJ_LIST(z, n, _n) \ + handle<> _pA##n = converter::arg_to_python<A##n>(a##n); + +# define BOOST_PYTHON_POBJ_LIST(z, n, _) \ + , _pA##n.get() + # define BOOST_PP_ITERATION_PARAMS_1 (3, (0, BOOST_PYTHON_MAX_ARITY, <boost/python/call.hpp>)) # include BOOST_PP_ITERATE() # undef BOOST_PYTHON_FAST_ARG_TO_PYTHON_GET +# undef BOOST_PYTHON_CONVERT_POBJ_LIST +# undef BOOST_PYTHON_POBJ_LIST }} // namespace boost::python @@ -70,6 +78,60 @@ // pointer/reference detection. converter::return_from_python<R> converter; return converter(result); +} + +template < + class R + BOOST_PP_ENUM_TRAILING_PARAMS_Z(1, N, class A) + > +typename detail::returnable<R>::type +call_with_gil(PyObject* callable + BOOST_PP_COMMA_IF(N) BOOST_PP_ENUM_BINARY_PARAMS_Z(1, N, A, const& a) + , boost::type<R>* = 0 + ) +{ + PyGILState_STATE _state = PyGILState_Ensure(); + + BOOST_PP_REPEAT_1ST(N, BOOST_PYTHON_CONVERT_POBJ_LIST, nil) + + PyObject* const result = + PyEval_CallFunction( + callable + , const_cast<char*>("(" BOOST_PP_REPEAT_1ST(N, BOOST_PYTHON_FIXED, "O") ")") + BOOST_PP_REPEAT_1ST(N, BOOST_PYTHON_POBJ_LIST, nil) + ); + + converter::return_from_python<R> converter; + typename detail::returnable<R>::type r = converter(result); + PyGILState_Release(_state); + return r; +} + +template < + class R + BOOST_PP_ENUM_TRAILING_PARAMS_Z(1, N, class A) + > +typename detail::returnable<R>::type +call_void_with_gil(PyObject* callable + BOOST_PP_COMMA_IF(N) BOOST_PP_ENUM_BINARY_PARAMS_Z(1, N, A, const& a) + , boost::type<R>* = 0 + ) +{ + PyGILState_STATE _state = PyGILState_Ensure(); + + BOOST_PP_REPEAT_1ST(N, BOOST_PYTHON_CONVERT_POBJ_LIST, nil) + + PyObject* const result = + PyEval_CallFunction( + callable + , const_cast<char*>("(" BOOST_PP_REPEAT_1ST(N, BOOST_PYTHON_FIXED, "O") ")") + BOOST_PP_REPEAT_1ST(N, BOOST_PYTHON_POBJ_LIST, nil) + ); + + converter::return_from_python<R> converter; + converter(result); + + PyGILState_Release(_state); } # undef N Index: boost/python/call_method.hpp =================================================================== RCS file: /cvsroot/boost/boost/boost/python/call_method.hpp,v retrieving revision 1.11 diff -u -w -b -u -r1.11 call_method.hpp --- boost/python/call_method.hpp 31 May 2003 14:53:01 -0000 1.11 +++ boost/python/call_method.hpp 6 Jul 2003 22:51:12 -0000 @@ -29,10 +29,18 @@ # define BOOST_PYTHON_FAST_ARG_TO_PYTHON_GET(z, n, _) \ , converter::arg_to_python<A##n>(a##n).get() +# define BOOST_PYTHON_CONVERT_POBJ_LIST(z, n, _n) \ + handle<> _pA##n = converter::arg_to_python<A##n>(a##n); + +# define BOOST_PYTHON_POBJ_LIST(z, n, _) \ + , _pA##n.get() + # define BOOST_PP_ITERATION_PARAMS_1 (3, (0, BOOST_PYTHON_MAX_ARITY, <boost/python/call_method.hpp>)) # include BOOST_PP_ITERATE() # undef BOOST_PYTHON_FAST_ARG_TO_PYTHON_GET +# undef BOOST_PYTHON_CONVERT_POBJ_LIST +# undef BOOST_PYTHON_POBJ_LIST }} // namespace boost::python @@ -70,6 +78,63 @@ // pointer/reference detection. converter::return_from_python<R> converter; return converter(result); + return _ret; +} + +template < + class R + BOOST_PP_ENUM_TRAILING_PARAMS_Z(1, N, class A) + > +typename detail::returnable<R>::type +call_method_with_gil(PyObject* self, char const* name + BOOST_PP_COMMA_IF(N) BOOST_PP_ENUM_BINARY_PARAMS_Z(1, N, A, const& a) + , boost::type<R>* = 0 + ) +{ + PyGILState_STATE _state = PyGILState_Ensure(); + + BOOST_PP_REPEAT_1ST(N, BOOST_PYTHON_CONVERT_POBJ_LIST, nil) + + PyObject* const result = + PyEval_CallMethod( + self + , const_cast<char*>(name) + , const_cast<char*>("(" BOOST_PP_REPEAT_1ST(N, BOOST_PYTHON_FIXED, "O") ")") + BOOST_PP_REPEAT_1ST(N, BOOST_PYTHON_POBJ_LIST, nil) + ); + + converter::return_from_python<R> converter; + typename detail::returnable<R>::type _ret = converter(result); + PyGILState_Release(_state); + return _ret; +} + +template < + class R + BOOST_PP_ENUM_TRAILING_PARAMS_Z(1, N, class A) + > +void +call_void_method_with_gil(PyObject* self, char const* name + BOOST_PP_COMMA_IF(N) BOOST_PP_ENUM_BINARY_PARAMS_Z(1, N, A, const& a) + , boost::type<R>* = 0 + ) +{ + PyGILState_STATE _state = PyGILState_Ensure(); + + BOOST_PP_REPEAT_1ST(N, BOOST_PYTHON_CONVERT_POBJ_LIST, nil) + + PyObject* const result = + PyEval_CallMethod( + self + , const_cast<char*>(name) + , const_cast<char*>("(" BOOST_PP_REPEAT_1ST(N, BOOST_PYTHON_FIXED, "O") ")") + BOOST_PP_REPEAT_1ST(N, BOOST_PYTHON_POBJ_LIST, nil) + ); + + converter::return_from_python<R> converter; + converter(result); + + PyGILState_Release(_state); } # undef N Index: boost/python/detail/caller.hpp =================================================================== RCS file: /cvsroot/boost/boost/boost/python/detail/caller.hpp,v retrieving revision 1.16 diff -u -w -b -u -r1.16 caller.hpp --- boost/python/detail/caller.hpp 27 Nov 2002 07:04:32 -0000 1.16 +++ boost/python/detail/caller.hpp 6 Jul 2003 22:51:23 -0000 @@ -159,7 +159,7 @@ typedef typename detail::invoke_tag<F>::type tag; PyObject* result = detail::invoke( - tag(), result_converter(), m_data.first() BOOST_PP_ENUM_TRAILING_PARAMS(N, c)); + tag(), result_converter(), m_data.first(), Sig() BOOST_PP_ENUM_TRAILING_PARAMS(N, c)); return m_data.second().postcall(args_, result); } Index: boost/python/detail/invoke.hpp =================================================================== RCS file: /cvsroot/boost/boost/boost/python/detail/invoke.hpp,v retrieving revision 1.3 diff -u -w -b -u -r1.3 invoke.hpp --- boost/python/detail/invoke.hpp 3 Jun 2003 02:57:12 -0000 1.3 +++ boost/python/detail/invoke.hpp 6 Jul 2003 22:51:23 -0000 @@ -13,6 +13,11 @@ # include <boost/python/detail/none.hpp> # include <boost/type_traits/is_member_function_pointer.hpp> +# include <boost/type_traits/function_traits.hpp> + +# include <boost/mpl/identity.hpp> +# include <boost/mpl/apply_if.hpp> +# include <boost/mpl/front.hpp> # include <boost/preprocessor/iterate.hpp> # include <boost/preprocessor/facilities/intercept.hpp> @@ -20,6 +25,7 @@ # include <boost/preprocessor/repetition/enum_trailing_binary_params.hpp> # include <boost/preprocessor/repetition/enum_binary_params.hpp> # include <boost/python/to_python_value.hpp> +# include <boost/python/signature.hpp> // This file declares a series of overloaded invoke(...) functions, // used to invoke wrapped C++ function (object)s from Python. Each one @@ -74,29 +80,42 @@ # define N BOOST_PP_ITERATION() -template <class RC, class F BOOST_PP_ENUM_TRAILING_PARAMS_Z(1, N, class AC)> -inline PyObject* invoke(fn_tag, RC*, F& f BOOST_PP_ENUM_TRAILING_BINARY_PARAMS_Z(1, N, AC, & ac) ) +template <class RC, class F, class Sig BOOST_PP_ENUM_TRAILING_PARAMS_Z(1, N, class AC)> +inline PyObject* invoke(fn_tag, RC*, F& f, Sig const& BOOST_PP_ENUM_TRAILING_BINARY_PARAMS_Z(1, N, AC, & ac) ) { - return RC()(f( BOOST_PP_ENUM_BINARY_PARAMS_Z(1, N, ac, () BOOST_PP_INTERCEPT) )); + PyThreadState *_save; + Py_UNBLOCK_THREADS + typename mpl::front<Sig>::type _r = f( BOOST_PP_ENUM_BINARY_PARAMS_Z(1, N, ac, () BOOST_PP_INTERCEPT) ); + Py_BLOCK_THREADS + return RC()(_r); } -template <class F BOOST_PP_ENUM_TRAILING_PARAMS_Z(1, N, class AC)> -inline PyObject* invoke(fn_tag, void_result_to_python, F& f BOOST_PP_ENUM_TRAILING_BINARY_PARAMS_Z(1, N, AC, & ac) ) +template <class F, class Sig BOOST_PP_ENUM_TRAILING_PARAMS_Z(1, N, class AC)> +inline PyObject* invoke(fn_tag, void_result_to_python, F& f, Sig const& BOOST_PP_ENUM_TRAILING_BINARY_PARAMS_Z(1, N, AC, & ac) ) { + Py_BEGIN_ALLOW_THREADS f( BOOST_PP_ENUM_BINARY_PARAMS_Z(1, N, ac, () BOOST_PP_INTERCEPT) ); + Py_END_ALLOW_THREADS return none(); } -template <class RC, class F, class TC BOOST_PP_ENUM_TRAILING_PARAMS_Z(1, N, class AC)> -inline PyObject* invoke(mem_fn_tag, RC*, F& f, TC& tc BOOST_PP_ENUM_TRAILING_BINARY_PARAMS_Z(1, N, AC, & ac) ) +template <class RC, class F, class Sig, class TC BOOST_PP_ENUM_TRAILING_PARAMS_Z(1, N, class AC)> +inline PyObject* invoke(mem_fn_tag, RC*, F& f, Sig const& _sig, TC& tc BOOST_PP_ENUM_TRAILING_BINARY_PARAMS_Z(1, N, AC, & ac) ) { - return RC()( (tc().*f)(BOOST_PP_ENUM_BINARY_PARAMS_Z(1, N, ac, () BOOST_PP_INTERCEPT)) ); + PyThreadState *_save; + Py_UNBLOCK_THREADS + typename mpl::front<Sig>::type _r = (tc().*f)(BOOST_PP_ENUM_BINARY_PARAMS_Z(1, N, ac, () BOOST_PP_INTERCEPT)); + Py_BLOCK_THREADS + return RC()(_r); + } -template <class F, class TC BOOST_PP_ENUM_TRAILING_PARAMS_Z(1, N, class AC)> -inline PyObject* invoke(mem_fn_tag, void_result_to_python, F& f, TC& tc BOOST_PP_ENUM_TRAILING_BINARY_PARAMS_Z(1, N, AC, & ac) ) +template <class F, class Sig ,class TC BOOST_PP_ENUM_TRAILING_PARAMS_Z(1, N, class AC)> +inline PyObject* invoke(mem_fn_tag, void_result_to_python, F& f, Sig const&, TC& tc BOOST_PP_ENUM_TRAILING_BINARY_PARAMS_Z(1, N, AC, & ac) ) { + Py_BEGIN_ALLOW_THREADS (tc().*f)(BOOST_PP_ENUM_BINARY_PARAMS_Z(1, N, ac, () BOOST_PP_INTERCEPT)); + Py_END_ALLOW_THREADS return none(); } Index: libs/python/src/module.cpp =================================================================== RCS file: /cvsroot/boost/boost/libs/python/src/module.cpp,v retrieving revision 1.20 diff -u -w -b -u -r1.20 module.cpp --- libs/python/src/module.cpp 4 Oct 2002 21:58:21 -0000 1.20 +++ libs/python/src/module.cpp 6 Jul 2003 22:51:36 -0000 @@ -26,6 +26,7 @@ BOOST_PYTHON_DECL void init_module(char const* name, void(*init_function)()) { + PyEval_InitThreads(); PyObject* m = Py_InitModule(const_cast<char*>(name), initial_methods);
Vladimir Vukicevic <vladimir@pobox.com> writes:
David Abrahams wrote:
FooProxy is a remote object that I get as a result of calling various functions to get a proxy -- it has a "void bar();" (non-virtual) that I can call. >
Hmm. This is totally off-topic, but it seems odd to me (and inconvenient, in C++) that FooProxy is not derived from Foo.
Live servants and proxies live in different class hierarchies; the Proxy objects just know how to add their tag to a packet and serialize arguments to operations, whereas Foo might have additional data members and the like.
Which is why Foo ought to be an empty abstract base class. But anyway...
It's this problem that I'm directly dealing with -- I can't figure out a way to wrap class FooProxy such that I can manage the current ThreadState and the GIL around them. I can create another wrapper class
Do you mean like a virtual-function-dispatching class, the kind with the initial "PyObject* self" argument to its constructors?
Yes.
that does the right thing and calls FooProxy::bar(), but functions are returning a FooProxy (not mywrapper_for_FooProxy)
I thought you said that all the C++ interfaces passed FooProxyHandle (?)
Sorry, my mistake.. it is a FooProxyHandle. Internally FooProxy is typedef'd to ::Ice::ProxyHandle< ::IceProxy::Foo> :)
See boost/python/converter/register_ptr_to_python.hpp in the CVS.
and I already have a Handle<FooProxy> as a wrapper specified in the boost::python class_... so I end up with incorrect types half the time.
Specifically, what problem are you having? Could you show a function you are unable to wrap properly and describe the specific problems you're having?
Sure; see end of message, as it's somewhat lengthy and I didn't want to clutter up the replies. (*)
However, an alternative solution that I was thinking of is to bake the thread smarts directly into boost::python. >
I have always thought that should happen in some form or other anyway.
It seems to me that call_method and similar should always be wrapped with a PyGILState_Ensure/PyGILState_Release. >
Some people know their extensions are only being called on the main thread and not everyone will be ready to pay the cost for acquiring the GIL on every callback. IMO supplying a GILState class which acquires on construction and releases on destruction *should* be enough.
Hmm, I'm not sure what you mean; where would you apply the GILState class? I agree though, acquiring the GIL on every callback is heavyhanded, but necessary for some applications (like mine, where I don't know exactly which thread a callback will get invoked on for a particular object instance).
I guess from your other messages you figured that out.
Also, an additional call policy could be added, something like "allow_threads", which would cause b::p to call Py_BEGIN_ALLOW_THREADS and Py_END_ALLOW_THREADS around the function invocation on C++. This seems like a much more generally useful solution than me trying to do the dance in wrapper classes.
I think some change to the requirements and usage of CallPolicies would be neccessary in order to support that (see my reply to Nikolay), but I agree with the general idea.
Is this the right way to go? > Probably. I also think that some people want automatic Py_BEGIN_ALLOW_THREADS/Py_END_ALLOW_THREADS for *all* of their wrapped functions, and that should probably be configurable, too. You have to be careful with that, though: once you do Py_BEGIN_ALLOW_THREADS you can't use any Boost.Python facilities which use the Python 'C' API (e.g. object, handle<>, ...) until you reach Py_END_ALLOW_THREADS.
Yep; I assume invoke.hpp is the lowest-level at which the C++ function is actually invoked.
Close.
If so, I'd appreciate any pointers on implementing a new call policy, especially if it's possible to both specify allow_threads and a return_value_policy.
I think you'd better start by looking at the macro definition of Py_BEGIN_ALLOW_THREADS et al and then at how policies are used to see how state can be stored for the duration of the call they affect, as the macros are wont to do.
* Specific function problem:
Each ProxyHandle<Proxy::Foo> exports (via the ProxyHandle class) a checkedCast() static member, that can be used to convert ProxyHandle<Proxy::Object> (which is the root of all proxies) to a specific proxy type. checkedCast() checkedcast actually creates a new ProxyHandle<Proxy::Foo> object, and then copies the target server information into it from the ProxyHandle<Proxy::Object>, so there's no "cast" in the C++ sense going on (though it does check if it can dynamic_cast<> the Proxy::Object* to a Proxy::Foo*, and if so, just does that).
To have support for checkedCast in python, I create wrapper functions such as:
namespace pyce_Hello_casts { ::IceInternal::ProxyHandle< ::IceProxy::Hello> _Hello_checkedCast ^^^^^^^^^^^^^^^^^^ This name is reserved for your C++ implementation.
(const ::Ice::ObjectPrx& o) { return ::IceInternal::ProxyHandle< ::IceProxy::Hello>::checkedCast (o); } };
and my class_<> def looks like:
class_< ::IceProxy::Hello , ::IceInternal::ProxyHandle< ::IceProxy::Hello >, bases< ::IceProxy::Ice::Object >, boost::noncopyable > ("HelloPrx") .def("sayHello", &::IceProxy::Hello::sayHello) .def("checkedCast", pyce_Hello_casts::_Hello_checkedCast) .staticmethod("checkedCast") ;
This all works fine, until I want to intercept the call to sayHello() (to save/restore the ThreadState goop). I can create a wrapper that derives from ::IceProxy::Hello, but then I have to deal with that PyObject* constructor, for which I have no need for here -- I'm never going to utilize call_method, and HelloPrx will never be derived from in python-land. The sayHello() and other member functions are also not virtual. I could do a dance with a custom-written overload dispatch for each function. If I went the wrapper route, I would need to implement my own version of checkedCast that can convert to my wrapper, and add some implicitly_convertible statements, and make sure that no references to ::IceProxy::Hello (or the equivalent handle) get exposed anywhere.. certainly feasable, but a pain, especially for cases where I get a handle to a ::IceProxy::Hello in C++ and I'd have to convert.
But both approaches seem like they're far too verbose. I'll probably create a call_method_with_threads<> such that the non-GIL-acquiring call_method<> is still available; extending call policies would allow a nice implementation for GIL-releasing and non-GIL-releasing C++ calls in caller.hpp, but the problem with the ResultConverter being called from invoke.hpp still remains. If there was a base class for all result converters, it could perhaps acquire the GIL in its constructor and release in the destructor, though no such class exists (that I can see?), and it would be yet another mutex acquisition/release.
This is all very complicated sounding, and I can't tell whether it's still an issue in light of the guard object idea, and I have so much to catch up on I don't think I have time to analyze. If there's still an issue here, could you boil it down a bit? -- Dave Abrahams Boost Consulting www.boost-consulting.com
On Mon, 2003-07-07 at 13:13, David Abrahams wrote:
Vladimir Vukicevic <vladimir@pobox.com> writes: [...]
This is all very complicated sounding, and I can't tell whether it's still an issue in light of the guard object idea, and I have so much to catch up on I don't think I have time to analyze. If there's still an issue here, could you boil it down a bit?
Sorry 'bout that, I was in a hurry when I wrote that and didn't manage to explain concisely. Essentially what I think needs to happen is the following: 1. The GIL needs to be acquired at every point that python calls into boost::python glue code. (Any such acquisition should be optional, such that extensions that don't care about threads don't have to deal with the overhead.) 2. The GIL needs to be released (threads "allowed") around every call to a native C++ function wrapped by boost::python. (This function should, if it needs to call Python or boost::python API calls, acquire the GIL itself.) I realized that I was being quite silly in my last patch, and implemented both a GIL guard class (as suggested) and also a threads allow class that do the right thing in their constructors/destructors; this simplified things tremendously. A newer patch is attached. It seems to work for me, though I'm sure that I'd need to spend some time coming up with specific test cases to be really comfortable with it. - Vlad
Vladimir Vukicevic <vladimir@pobox.com> writes:
On Mon, 2003-07-07 at 13:13, David Abrahams wrote:
Vladimir Vukicevic <vladimir@pobox.com> writes: [...]
This is all very complicated sounding, and I can't tell whether it's still an issue in light of the guard object idea, and I have so much to catch up on I don't think I have time to analyze. If there's still an issue here, could you boil it down a bit?
Sorry 'bout that, I was in a hurry when I wrote that and didn't manage to explain concisely.
Essentially what I think needs to happen is the following:
1. The GIL needs to be acquired at every point that python calls into boost::python glue code.
I don't think so! When Python calls boost::python it *always* has the GIL already. It looks like you have way too many detail::gil_lock instances in your patch. GUI acquisition needs to be done when C++ code calls back into python.
(Any such acquisition should be optional, such that extensions that don't care about threads don't have to deal with the overhead.)
Which is why I don't understand the need for call_with_gil<>. What's wrong with asking the user to create a local gil_lock of his own? He might need one anyway, since he may need to use the Boost.Python/Python API himself. void my_callback() { x = call<int>(make_tuple("a", "b", "c")); }
2. The GIL needs to be released (threads "allowed") around every call to a native C++ function wrapped by boost::python. (This function should, if it needs to call Python or boost::python API calls, acquire the GIL itself.)
No way. That incurs way too much overhead for small wrapped functions. Releasing the GIL when we call a wrapped C++ function needs to be optional (possibly globally settable). In any case, even if it's not optional you have a problem because you're releasing the GIL when calling into functions that accept, e.g. python::object parameters. You haven't patched object::object(object const&) to acquire the gil. You'd need to find *every* PyXXXX in the library and make sure the GIL is acquired. All that aqcuiring/releasing could get expensive, and I'm not comfortable with that. -- Dave Abrahams Boost Consulting www.boost-consulting.com
On Sun, 2003-07-13 at 14:40, David Abrahams wrote:
Vladimir Vukicevic <vladimir@pobox.com> writes:
Sorry 'bout that, I was in a hurry when I wrote that and didn't manage to explain concisely.
Essentially what I think needs to happen is the following:
1. The GIL needs to be acquired at every point that python calls into boost::python glue code.
I don't think so!
When Python calls boost::python it *always* has the GIL already. It looks like you have way too many detail::gil_lock instances in your patch. GUI acquisition needs to be done when C++ code calls back into python.
Hmm.. you're right. There were a few problems that I had going on that made me put in a lot of gil_lock's, more so than what are needed. I'll have to go through and examine again exactly where they need to go.
(Any such acquisition should be optional, such that extensions that don't care about threads don't have to deal with the overhead.)
Which is why I don't understand the need for call_with_gil<>. What's wrong with asking the user to create a local gil_lock of his own? He might need one anyway, since he may need to use the Boost.Python/Python API himself.
Sure, sounds good.
2. The GIL needs to be released (threads "allowed") around every call to a native C++ function wrapped by boost::python. (This function should, if it needs to call Python or boost::python API calls, acquire the GIL itself.)
No way. That incurs way too much overhead for small wrapped functions. Releasing the GIL when we call a wrapped C++ function needs to be optional (possibly globally settable).
Are you thinking settable at runtime or at compile time? If at compile time, then defining BOOST_PYTHON_NO_THREADING causes gil_lock/thread_block to just be dummy stubs. I don't see how you'd do it for some methods and not for others, without adding extra logic to the path that leads to invoke() -- which I guess may be the right thing to do for a fully general solution.
In any case, even if it's not optional you have a problem because you're releasing the GIL when calling into functions that accept, e.g. python::object parameters. You haven't patched object::object(object const&) to acquire the gil. You'd need to find *every* PyXXXX in the library and make sure the GIL is acquired. All that aqcuiring/releasing could get expensive, and I'm not comfortable with that.
Yup, there's still a bunch of places where the locks aren't correct. Expensive or not though, I don't see a way around it -- you're either not thread safe (in which case you go the BOOST_PYTHON_NO_THREADING route at compile time), or you are -- eventually one could probably build separate boost_python .so with and without thread safety so that extensions can choose which version to link/build with. Thanks for the feedback, - Vlad
Vladimir Vukicevic <vladimir@pobox.com> writes:
On Sun, 2003-07-13 at 14:40, David Abrahams wrote:
Vladimir Vukicevic <vladimir@pobox.com> writes:
Sorry 'bout that, I was in a hurry when I wrote that and didn't manage to explain concisely.
Essentially what I think needs to happen is the following:
1. The GIL needs to be acquired at every point that python calls into boost::python glue code.
I don't think so!
When Python calls boost::python it *always* has the GIL already. It looks like you have way too many detail::gil_lock instances in your patch. GUI acquisition needs to be done when C++ code calls back into python.
Hmm.. you're right. There were a few problems that I had going on that made me put in a lot of gil_lock's, more so than what are needed. I'll have to go through and examine again exactly where they need to go.
(Any such acquisition should be optional, such that extensions that don't care about threads don't have to deal with the overhead.)
Which is why I don't understand the need for call_with_gil<>. What's wrong with asking the user to create a local gil_lock of his own? He might need one anyway, since he may need to use the Boost.Python/Python API himself.
Sure, sounds good.
2. The GIL needs to be released (threads "allowed") around every call to a native C++ function wrapped by boost::python. (This function should, if it needs to call Python or boost::python API calls, acquire the GIL itself.)
No way. That incurs way too much overhead for small wrapped functions. Releasing the GIL when we call a wrapped C++ function needs to be optional (possibly globally settable).
Are you thinking settable at runtime or at compile time?
I was thinking the latter, but we do need to watch out for potential ODR problems in that case.
If at compile time, then defining BOOST_PYTHON_NO_THREADING causes gil_lock/thread_block to just be dummy stubs. I don't see how you'd do it for some methods and not for others
I think we should; the Lua people for example want to be able to do threading *and* not pay for things they won't use. Shouldn't Python users have the same privilege? Releasing the GIL just to set an integer data member on a wrapped object seems like overkill. Am I being silly?
, without adding extra logic to the path that leads to invoke() -- which I guess may be the right thing to do for a fully general solution.
As I said earlier, I think the solution is to wrap the *function* being def'd in a functor which releases the GIL, something like: def("foobar", threaded(foobar))
In any case, even if it's not optional you have a problem because you're releasing the GIL when calling into functions that accept, e.g. python::object parameters. You haven't patched object::object(object const&) to acquire the gil. You'd need to find *every* PyXXXX in the library and make sure the GIL is acquired. All that aqcuiring/releasing could get expensive, and I'm not comfortable with that.
Yup, there's still a bunch of places where the locks aren't correct. Expensive or not though, I don't see a way around it -- you're either not thread safe (in which case you go the BOOST_PYTHON_NO_THREADING route at compile time), or you are --
No, it's not that simple. If you never release the GIL and never try to acquire it, you're perfectly thread-safe, so long as you don't need Python code to execute concurrently with your C++ code. Once you release the GIL then you have to worry about Python calling into your C++ on another thread, so the thread-safety of your C++ code is at issue. You also have to make sure you acquire the GIL before you touch the Python API again. But there's no reason we can't ask users to manage the latter issue themselves if they've explicitly released the GIL. There are two use-cases when Python calls into C++: 1. The user writes a thin wrapper function, and can simply put the Py_BEGIN_ALLOW_THREADS/Py_END_ALLOW_THREADS (or some smarter C++ RAII equivalent in his wrapper) 2. The user wants to wrap the function directly without a thin wrapper. In that case the function is highly unlikely to have any manipulation of the Python API in it anyway.
eventually one could probably build separate boost_python .so with and without thread safety so that extensions can choose which version to link/build with.
I dunno, I think we need to discuss this some more until we can reach a common understanding. -- Dave Abrahams Boost Consulting www.boost-consulting.com
On Mon, 2003-07-07 at 13:13, David Abrahams wrote:
Vladimir Vukicevic <vladimir@pobox.com> writes: [...]
This is all very complicated sounding, and I can't tell whether it's still an issue in light of the guard object idea, and I have so much to catch up on I don't think I have time to analyze. If there's still an issue here, could you boil it down a bit?
Sorry 'bout that, I was in a hurry when I wrote that and didn't manage to explain concisely. Essentially what I think needs to happen is the following: 1. The GIL needs to be acquired at every point that python calls into boost::python glue code. (Any such acquisition should be optional, such that extensions that don't care about threads don't have to deal with the overhead.) 2. The GIL needs to be released (threads "allowed") around every call to a native C++ function wrapped by boost::python. (This function should, if it needs to call Python or boost::python API calls, acquire the GIL itself.) I realized that I was being quite silly in my last patch, and implemented both a GIL guard class (as suggested) and also a threads allow class that do the right thing in their constructors/destructors; this simplified things tremendously. A newer patch is attached. It seems to work for me, though I'm sure that I'd need to spend some time coming up with specific test cases to be really comfortable with it. - Vlad
participants (3)
-
David Abrahams -
Nikolay Mladenov -
Vladimir Vukicevic