Conditional with gil and threading

[rth]

A nogil block with a loop that uses an unlikely with gil statement e.g. approximately,

cpdef float test_gil(float[:] x):
    cdef int i = 0
    cdef int size = x1.shape[0]
    cdef float out = 0
    with nogil:
        while i < size:
            out_sum += x[i]
            if i > size +1:
                with gil:
                    raise ValueError
   return out 

appears work similarly to the while function not releasing gil, when using in multi-threaded code.

See more complete minimal example in scikit-learn/scikit-learn#17038 (comment)

Naively I though that since the with gil is never executed, performance wise this would be equivalent to the same function without the with gil block (or roughly that threading performance would be directly impacted by the fraction of run time where the gil is released). However it does not seem to be the case.

Is this behavior expected? If so maybe the documentation section https://cython.readthedocs.io/en/latest/src/userguide/external_C_code.html#acquiring-and-releasing-the-gil could be updated to mention conditional acquiring of GIL.

[scoder]

Note that you do not need to write the with gil in this specific example since it's implicit in the raise statement (i.e. you can raise exceptions also from within nogil sections). That generates the same code as with gil, though, so will run into the same problem.

Looking at the C code that Cython generates from the example that you linked to, the difference is that the function that uses with gil has a Python temp variable, which needs to be cleaned up at the end in the exception case. For that, it needs to acquire the GIL. However, it wouldn't need the GIL in the success case, and still acquires it. That makes it uselessly hammer on the GIL in very fast functions like this.

The GIL acquisition is unconditionally inserted here, as a try-finally statement wrapping the whole function body:

cython/Cython/Compiler/ParseTreeTransforms.py

Lines 1861 to 1872 in f09e61a

	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))

and gets released here, after generating the function code:

cython/Cython/Compiler/Nodes.py

Lines 2145 to 2148 in f09e61a

	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

I don't know how difficult it would be to detect the needless acquire and avoid it, but in any case, I consider it a bug that this happens unconditionally. in the success case, it should run through the function without touching the GIL at all.

[rth]

Thanks for the detailed answer and investigation @scoder !

[scoder]

I think the best solution is to get rid of the try-finally node and do the cleanup in FuncDefNode.generate_function_definitions(), acquiring the GIL only when necessary for a given exit case (error or success).

[scoder]

I think I found a better way to do this. See #3556.

[scoder]

@rth @webber26232 could you try #3556 to see if it resolves the performance issue for you? I looked at the generated code and the success code path is free of GIL usage now and should be fast.

[rth]

Sorry for slow response @scoder. Thank you for finding a solution and for your work on Cython in general! I can confirm that 3.0a3 which includes this fix does resolve the problem on the example in scikit-learn/scikit-learn#17038 (comment)