mark florisson, 28.02.2012 21:20:
> On 28 February 2012 19:58, Stefan Behnel wrote:
>> mark florisson, 28.02.2012 16:35:
>>> Basically, the cleanup code only needs a matching release because the
>>> corresponding acquire is in EnsureGILNode, which wraps the function
>>> body in case of a nogil function with a 'with gil' block. Any changes
>>> to the conditions in FuncDefNode will have to be reflected by the code
>>> that does that wrapping. Changing the refnanny macro for the cleanup
>>> code will not avail anything, as the GIL is already ensured.
>>
>> Regarding the "with gil" code, ISTM that the "finally" code in the with_gil
>> test is being duplicated. I noticed this when I moved the refnanny's GIL
>> state into a block local variable and that broke the C code. Basically, the
>> with-gil block had declared the variable in its own block, but was then
>> trying to access that variable in a second finally clause, further down and
>> outside of the with-gil block.
>>
>> Looks like a bug to me.
>
> That's not a bug, that's how it is implemented. At setup, a variable
> __pyx_gilstate_save is declared, which is also used for teardown. Any
> with GIL blocks use C block scoping to do the same thing. The with
> block is itself a try/finally, so you get a try/finally wrapped in a
> try/finally. The code uses try/finally for it's way of trapping
> control flow, allowing some code to execute and resuming control flow
> afterwards.
Ok, so what really got me confused is that the code used the variable
"acquire_gil_for_refnanny_only" in places that didn't have anything to do
with the refnanny. Similarly, "acquire_gil_for_var_decls_only" was used for
cleanup even though the GIL had already been released if this flag was set,
way before the end of the function.
I think I fixed both issues in the patch I attached. At least, it still
passes the gil related tests and doesn't raise any C compiler warnings
about the GIL state variable being unused.
Does this look about right?
Stefan
# HG changeset patch
# Parent c059cc6f46c3613a117e30a0e696180e93b420f4
moved refnanny GIL handling code for nogil functions into a C macro
diff -r c059cc6f46c3 Cython/Compiler/Code.py
--- a/Cython/Compiler/Code.py Sun Feb 26 16:04:17 2012 +0000
+++ b/Cython/Compiler/Code.py Tue Feb 28 21:56:36 2012 +0100
@@ -1845,8 +1845,11 @@
def put_declare_refcount_context(self):
self.putln('__Pyx_RefNannyDeclarations')
- def put_setup_refcount_context(self, name):
- self.putln('__Pyx_RefNannySetupContext("%s");' % name)
+ def put_setup_refcount_context(self, name, acquire_gil=False):
+ if acquire_gil:
+ from Cython.Compiler import Nodes
+ self.globalstate.use_utility_code(Nodes.force_init_threads_utility_code)
+ self.putln('__Pyx_RefNannySetupContext("%s", %d);' % (name, acquire_gil and 1 or 0))
def put_finish_refcount_context(self):
self.putln("__Pyx_RefNannyFinishContext();")
diff -r c059cc6f46c3 Cython/Compiler/ModuleNode.py
--- a/Cython/Compiler/ModuleNode.py Sun Feb 26 16:04:17 2012 +0000
+++ b/Cython/Compiler/ModuleNode.py Tue Feb 28 21:56:36 2012 +0100
@@ -2742,8 +2742,19 @@
static __Pyx_RefNannyAPIStruct *__Pyx_RefNanny = NULL;
static __Pyx_RefNannyAPIStruct *__Pyx_RefNannyImportAPI(const char *modname); /*proto*/
#define __Pyx_RefNannyDeclarations void *__pyx_refnanny = NULL;
- #define __Pyx_RefNannySetupContext(name) \
+#ifdef WITH_THREAD
+ #define __Pyx_RefNannySetupContext(name, acquire_gil) \
+ if (acquire_gil) { \
+ PyGILState_STATE __pyx_gilstate_save = PyGILState_Ensure(); \
+ __pyx_refnanny = __Pyx_RefNanny->SetupContext((name), __LINE__, __FILE__); \
+ PyGILState_Release(__pyx_gilstate_save); \
+ } else { \
+ __pyx_refnanny = __Pyx_RefNanny->SetupContext((name), __LINE__, __FILE__); \
+ }
+#else
+ #define __Pyx_RefNannySetupContext(name, acquire_gil) \
__pyx_refnanny = __Pyx_RefNanny->SetupContext((name), __LINE__, __FILE__)
+#endif
#define __Pyx_RefNannyFinishContext() \
__Pyx_RefNanny->FinishContext(&__pyx_refnanny)
#define __Pyx_INCREF(r) __Pyx_RefNanny->INCREF(__pyx_refnanny, (PyObject *)(r), __LINE__)
diff -r c059cc6f46c3 Cython/Compiler/Nodes.py
--- a/Cython/Compiler/Nodes.py Sun Feb 26 16:04:17 2012 +0000
+++ b/Cython/Compiler/Nodes.py Tue Feb 28 21:56:36 2012 +0100
@@ -1504,22 +1504,14 @@
if acquire_gil or acquire_gil_for_var_decls_only:
code.put_ensure_gil()
+ elif lenv.nogil and lenv.has_with_gil_block:
+ code.declare_gilstate()
# ----- set up refnanny
if use_refnanny:
- if acquire_gil_for_refnanny_only:
- code.declare_gilstate()
- code.putln("#if CYTHON_REFNANNY")
- code.put_ensure_gil(declare_gilstate=False)
- code.putln("#endif /* CYTHON_REFNANNY */")
-
tempvardecl_code.put_declare_refcount_context()
- code.put_setup_refcount_context(self.entry.name)
-
- if acquire_gil_for_refnanny_only:
- code.putln("#if CYTHON_REFNANNY")
- code.put_release_ensured_gil()
- code.putln("#endif /* CYTHON_REFNANNY */")
+ code.put_setup_refcount_context(
+ self.entry.name, acquire_gil=acquire_gil_for_refnanny_only)
# ----- Automatic lead-ins for certain special functions
if is_getbuffer_slot:
@@ -1775,8 +1767,7 @@
# GIL holding funcion
code.put_finish_refcount_context()
- if (acquire_gil or acquire_gil_for_var_decls_only or
- acquire_gil_for_refnanny_only):
+ if acquire_gil or (lenv.nogil and lenv.has_with_gil_block):
code.put_release_ensured_gil()
if not self.return_type.is_void:
_______________________________________________
cython-devel mailing list
[email protected]
http://mail.python.org/mailman/listinfo/cython-devel
