From a428ead3a6bbfd22a05ebb0b6ed414bad015807f Mon Sep 17 00:00:00 2001 From: Stefan Behnel Date: Tue, 17 May 2022 16:12:34 +0200 Subject: Revert "Avoid acquiring the GIL at the end of nogil functions (GH-3556) (GH-4749)" This reverts commit 74073417c4f7e616af0a139a55f790f1ec7fe728. --- Cython/Compiler/Code.py | 4 +- Cython/Compiler/Nodes.py | 70 +++++++++------------------------- Cython/Compiler/ParseTreeTransforms.py | 14 +++++++ Cython/Utility/ModuleSetupCode.c | 6 --- tests/run/trace_nogil.pyx | 22 ----------- 5 files changed, 35 insertions(+), 81 deletions(-) delete mode 100644 tests/run/trace_nogil.pyx diff --git a/Cython/Compiler/Code.py b/Cython/Compiler/Code.py index ed7dceb35..45c5a325c 100644 --- a/Cython/Compiler/Code.py +++ b/Cython/Compiler/Code.py @@ -2407,8 +2407,8 @@ class CCodeWriter(object): UtilityCode.load_cached("ForceInitThreads", "ModuleSetupCode.c")) self.putln('__Pyx_RefNannySetupContext("%s", %d);' % (name, acquire_gil and 1 or 0)) - def put_finish_refcount_context(self, nogil=False): - self.putln("__Pyx_RefNannyFinishContextNogil()" if nogil else "__Pyx_RefNannyFinishContext();") + def put_finish_refcount_context(self): + self.putln("__Pyx_RefNannyFinishContext();") def put_add_traceback(self, qualified_name, include_cline=True): """ diff --git a/Cython/Compiler/Nodes.py b/Cython/Compiler/Nodes.py index 01ef96db2..339b1fa04 100644 --- a/Cython/Compiler/Nodes.py +++ b/Cython/Compiler/Nodes.py @@ -1862,12 +1862,11 @@ class FuncDefNode(StatNode, BlockNode): use_refnanny = not lenv.nogil or lenv.has_with_gil_block - gilstate_decl = None if acquire_gil or acquire_gil_for_var_decls_only: code.put_ensure_gil() code.funcstate.gil_owned = True - else: - gilstate_decl = code.insertion_point() + elif lenv.nogil and lenv.has_with_gil_block: + code.declare_gilstate() if profile or linetrace: if not self.is_generator: @@ -1990,19 +1989,6 @@ class FuncDefNode(StatNode, BlockNode): code.putln("") code.putln("/* function exit code */") - gil_owned = { - 'success': code.funcstate.gil_owned, - 'error': code.funcstate.gil_owned, - 'gil_state_declared': gilstate_decl is None, - } - def assure_gil(code_path): - if not gil_owned[code_path]: - if not gil_owned['gil_state_declared']: - gilstate_decl.declare_gilstate() - gil_owned['gil_state_declared'] = True - code.put_ensure_gil(declare_gilstate=False) - gil_owned[code_path] = True - # ----- Default return value if not self.body.is_terminator: if self.return_type.is_pyobject: @@ -2010,10 +1996,8 @@ class FuncDefNode(StatNode, BlockNode): # lhs = "(PyObject *)%s" % Naming.retval_cname #else: lhs = Naming.retval_cname - assure_gil('success') code.put_init_to_py_none(lhs, self.return_type) - elif not self.return_type.is_memoryviewslice: - # memory view structs receive their default value on initialisation + else: val = self.return_type.default_value if val: code.putln("%s = %s;" % (Naming.retval_cname, val)) @@ -2035,7 +2019,6 @@ class FuncDefNode(StatNode, BlockNode): code.globalstate.use_utility_code(restore_exception_utility_code) code.putln("{ PyObject *__pyx_type, *__pyx_value, *__pyx_tb;") code.putln("__Pyx_PyThreadState_declare") - assure_gil('error') code.putln("__Pyx_PyThreadState_assign") code.putln("__Pyx_ErrFetch(&__pyx_type, &__pyx_value, &__pyx_tb);") for entry in used_buffer_entries: @@ -2055,14 +2038,20 @@ class FuncDefNode(StatNode, BlockNode): # code.globalstate.use_utility_code(get_exception_tuple_utility_code) # code.put_trace_exception() - assure_gil('error') + if lenv.nogil and not lenv.has_with_gil_block: + code.putln("{") + code.put_ensure_gil() + code.put_add_traceback(self.entry.qualified_name) + + if lenv.nogil and not lenv.has_with_gil_block: + code.put_release_ensured_gil() + code.putln("}") else: warning(self.entry.pos, "Unraisable exception in function '%s'." % self.entry.qualified_name, 0) - assure_gil('error') - code.put_unraisable(self.entry.qualified_name) + code.put_unraisable(self.entry.qualified_name, lenv.nogil) default_retval = self.return_type.default_value if err_val is None and default_retval: err_val = default_retval @@ -2073,33 +2062,19 @@ class FuncDefNode(StatNode, BlockNode): code.putln("__Pyx_pretend_to_initialize(&%s);" % Naming.retval_cname) if is_getbuffer_slot: - assure_gil('error') self.getbuffer_error_cleanup(code) # If we are using the non-error cleanup section we should # jump past it if we have an error. The if-test below determine # whether this section is used. if buffers_present or is_getbuffer_slot or self.return_type.is_memoryviewslice: - # In the buffer cases, we already called assure_gil('error') and own the GIL. - assert gil_owned['error'] or self.return_type.is_memoryviewslice code.put_goto(code.return_from_error_cleanup_label) - else: - # align error and success GIL state - if gil_owned['success']: - assure_gil('error') - elif gil_owned['error']: - code.put_release_ensured_gil() - gil_owned['error'] = False # ----- Non-error return cleanup code.put_label(code.return_label) - assert gil_owned['error'] == gil_owned['success'], "%s != %s" % (gil_owned['error'], gil_owned['success']) - for entry in used_buffer_entries: - assure_gil('success') Buffer.put_release_buffer_code(code, entry) if is_getbuffer_slot: - assure_gil('success') self.getbuffer_normal_cleanup(code) if self.return_type.is_memoryviewslice: @@ -2109,22 +2084,17 @@ class FuncDefNode(StatNode, BlockNode): cond = code.unlikely(self.return_type.error_condition(Naming.retval_cname)) code.putln( 'if (%s) {' % cond) - if not gil_owned['success']: + if env.nogil: code.put_ensure_gil() code.putln( 'PyErr_SetString(PyExc_TypeError, "Memoryview return value is not initialized");') - if not gil_owned['success']: + if env.nogil: code.put_release_ensured_gil() code.putln( '}') # ----- Return cleanup for both error and no-error return - if code.label_used(code.return_from_error_cleanup_label): - # If we came through the success path, then we took the same GIL decisions as for jumping here. - # If we came through the error path and did not jump, then we aligned both paths above. - # In the end, all paths are aligned from this point on. - assert gil_owned['error'] == gil_owned['success'], "%s != %s" % (gil_owned['error'], gil_owned['success']) - code.put_label(code.return_from_error_cleanup_label) + code.put_label(code.return_from_error_cleanup_label) for entry in lenv.var_entries: if not entry.used or entry.in_closure: @@ -2151,7 +2121,6 @@ class FuncDefNode(StatNode, BlockNode): code.put_xdecref_memoryviewslice(entry.cname, have_gil=not lenv.nogil) if self.needs_closure: - assure_gil('success') code.put_decref(Naming.cur_scope_cname, lenv.scope_class.type) # ----- Return @@ -2167,7 +2136,6 @@ class FuncDefNode(StatNode, BlockNode): if self.entry.is_special and self.entry.name == "__hash__": # Returning -1 for __hash__ is supposed to signal an error # We do as Python instances and coerce -1 into -2. - assure_gil('success') code.putln("if (unlikely(%s == -1) && !PyErr_Occurred()) %s = -2;" % ( Naming.retval_cname, Naming.retval_cname)) @@ -2177,16 +2145,16 @@ class FuncDefNode(StatNode, BlockNode): # generators are traced when iterated, not at creation if self.return_type.is_pyobject: code.put_trace_return( - Naming.retval_cname, nogil=not gil_owned['success']) + Naming.retval_cname, nogil=not code.funcstate.gil_owned) else: code.put_trace_return( - "Py_None", nogil=not gil_owned['success']) + "Py_None", nogil=not code.funcstate.gil_owned) if not lenv.nogil: # GIL holding function - code.put_finish_refcount_context(nogil=not gil_owned['success']) + code.put_finish_refcount_context() - if acquire_gil or (lenv.nogil and gil_owned['success']): + if acquire_gil or (lenv.nogil and lenv.has_with_gil_block): # release the GIL (note that with-gil blocks acquire it on exit in their EnsureGILNode) code.put_release_ensured_gil() code.funcstate.gil_owned = False diff --git a/Cython/Compiler/ParseTreeTransforms.py b/Cython/Compiler/ParseTreeTransforms.py index 24f475120..8e0cbc303 100644 --- a/Cython/Compiler/ParseTreeTransforms.py +++ b/Cython/Compiler/ParseTreeTransforms.py @@ -1859,6 +1859,19 @@ if VALUE is not None: return node + def _handle_nogil_cleanup(self, lenv, node): + "Handle cleanup for 'with gil' blocks in nogil functions." + if lenv.nogil and lenv.has_with_gil_block: + # Acquire the GIL for cleanup in 'nogil' functions, by wrapping + # the entire function body in try/finally. + # The corresponding release will be taken care of by + # Nodes.FuncDefNode.generate_function_definitions() + node.body = Nodes.NogilTryFinallyStatNode( + node.body.pos, + body=node.body, + finally_clause=Nodes.EnsureGILNode(node.body.pos), + finally_except_clause=Nodes.EnsureGILNode(node.body.pos)) + def _handle_fused(self, node): if node.is_generator and node.has_fused_arguments: node.has_fused_arguments = False @@ -1899,6 +1912,7 @@ if VALUE is not None: node = self._create_fused_function(env, node) else: node.body.analyse_declarations(lenv) + self._handle_nogil_cleanup(lenv, node) self._super_visit_FuncDefNode(node) self.seen_vars_stack.pop() diff --git a/Cython/Utility/ModuleSetupCode.c b/Cython/Utility/ModuleSetupCode.c index 87f60e1a2..f7af78bfa 100644 --- a/Cython/Utility/ModuleSetupCode.c +++ b/Cython/Utility/ModuleSetupCode.c @@ -1310,11 +1310,6 @@ static CYTHON_INLINE int __Pyx_Is_Little_Endian(void) #define __Pyx_RefNannySetupContext(name, acquire_gil) \ __pyx_refnanny = __Pyx_RefNanny->SetupContext((name), __LINE__, __FILE__) #endif - #define __Pyx_RefNannyFinishContextNogil() { \ - PyGILState_STATE __pyx_gilstate_save = PyGILState_Ensure(); \ - __Pyx_RefNannyFinishContext(); \ - PyGILState_Release(__pyx_gilstate_save); \ - } #define __Pyx_RefNannyFinishContext() \ __Pyx_RefNanny->FinishContext(&__pyx_refnanny) #define __Pyx_INCREF(r) __Pyx_RefNanny->INCREF(__pyx_refnanny, (PyObject *)(r), __LINE__) @@ -1328,7 +1323,6 @@ static CYTHON_INLINE int __Pyx_Is_Little_Endian(void) #else #define __Pyx_RefNannyDeclarations #define __Pyx_RefNannySetupContext(name, acquire_gil) - #define __Pyx_RefNannyFinishContextNogil() #define __Pyx_RefNannyFinishContext() #define __Pyx_INCREF(r) Py_INCREF(r) #define __Pyx_DECREF(r) Py_DECREF(r) diff --git a/tests/run/trace_nogil.pyx b/tests/run/trace_nogil.pyx deleted file mode 100644 index dee443e5b..000000000 --- a/tests/run/trace_nogil.pyx +++ /dev/null @@ -1,22 +0,0 @@ -# cython: linetrace=True - -cdef void foo(int err) nogil except *: - with gil: - raise ValueError(err) - - -# Test from gh-4637 -def handler(int err): - """ - >>> handler(0) - All good - >>> handler(1) # doctest: +ELLIPSIS - Traceback (most recent call last): - ... - ValueError: 1 - """ - if (err % 2): - with nogil: - foo(err) - else: - print("All good") -- cgit v1.2.1