[Cython] GIL handling C code

[Stefan Behnel]

mark florisson, 28.02.2012 22:09:
> On 28 February 2012 21:08, mark florisson wrote:
>> On 28 February 2012 20:19, Stefan Behnel wrote:
>>> Stefan Behnel, 28.02.2012 20:58:
>>>> 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.
>>>
>>> Hmm, guess I got confused again...
>>>
>>> So, the code is this:
>>>
>>> """
>>>       /*finally:*/ {
>>>          int __pyx_why;
>>>          __pyx_why = 0; goto __pyx_L8;
>>>          __pyx_L7: __pyx_why = 4; goto __pyx_L8;
>>>          __pyx_L8:;
>>>          #ifdef WITH_THREAD
>>>          PyGILState_Release(__pyx_gilstate_save);
>>>          #endif
>>>          switch (__pyx_why) {
>>>            case 4: goto __pyx_L4;
>>>          }
>>>        }
>>>    }
>>>  }
>>>  /*finally:*/ {
>>>    int __pyx_why;
>>>    __pyx_why = 0; goto __pyx_L5;
>>>    __pyx_L4: __pyx_why = 4; goto __pyx_L5;
>>>    __pyx_L5:;
>>>    #ifdef WITH_THREAD
>>>    __pyx_gilstate_save = PyGILState_Ensure();
>>>    #endif
>>>    switch (__pyx_why) {
>>>      case 4: goto __pyx_L1_error;
>>>    }
>>>  }
>>>
>>>  goto __pyx_L0;
>>>  __pyx_L1_error:;
>>>  __Pyx_XDECREF(__pyx_t_1);
>>>  __Pyx_XDECREF(__pyx_t_2);
>>>  __Pyx_WriteUnraisable("with_gil.void_nogil_ignore_exception",
>>> __pyx_clineno, __pyx_lineno, __pyx_filename);
>>>  __pyx_L0:;
>>>  #ifdef WITH_THREAD
>>>  PyGILState_Release(__pyx_gilstate_save);
>>>  #endif
>>> }
>>> """
>>>
>>> The first "finally" block is inside of the with-gil block, whereas the
>>> second is outside. In the second, the GIL is reacquired, and I guess that's
>>> for cleaning up temps in the error case, right? I see the problem that it
>>> can't be acquired after jumping to the exit labels (error/return) in the
>>> current code layout, but wouldn't it make sense to generate this code
>>> instead (starting at the second finally clause):
>>
>> This code could certainly be optimized, I just never got around to
>> implementing it.
>>
>>> """
>>>  /*finally:*/ {
>>>    int __pyx_why;
>>>    __pyx_why = 0; goto __pyx_L5;
>>>    __pyx_L4: __pyx_why = 4; goto __pyx_L5;
>>>    __pyx_L5:;
>>>    switch (__pyx_why) {
>>>      case 4: goto __pyx_L1_error;
>>>    }
>>>  }
>>>
>>>  goto __pyx_L0;
>>>  __pyx_L1_error:;
>>>  #ifdef WITH_THREAD
>>>  __pyx_gilstate_save = PyGILState_Ensure();
>>>  #endif
>>>  __Pyx_XDECREF(__pyx_t_1);
>>>  __Pyx_XDECREF(__pyx_t_2);
>>>  __Pyx_WriteUnraisable("with_gil.void_nogil_ignore_exception",
>>> __pyx_clineno, __pyx_lineno, __pyx_filename);
>>>  #ifdef WITH_THREAD
>>>  PyGILState_Release(__pyx_gilstate_save);
>>>  #endif
>>>  return;   /* <- error return here! */
>>>  __pyx_L0:;
>>> }
>>> """
>>>
>>> Meaning: split the return code paths and let them independently acquire the
>>> GIL if needed? That would at least relieve the outer finally from having to
>>> care about the GIL and having to rely on "someone" to declare the GIL
>>> variable and eventually release it.
>>
>> In this case, that would be valid. Normally though,
>> the exception case should fall through to the normal case, so I think
>> what you'd want is
>>
>>    ...
>>    goto L0;
>> L1:
>>    PyGILState_Ensure();
>>    /* error code */
>>    __pyx_r = ...;
>>    goto L2;
>> L0:
>>    PyGILState_Ensure();
>> L2:
>>    /* normal code */
>>    return __pyx_r;
>> }
> 
> And there should be a release just before the return of course :)

Yes, I think that's much better than acquiring the GIL in some node in the
syntax tree and then releasing it in a completely different place at
function exit.

(IIRC, the C compiler won't allow the definition of the GIL state variable
to occur at the end in this case because there'd be code paths that use it
that start at a label after the declaration, so it would still have to be
declared at the beginning. Fine with me.)


>> If the entire body is a 'with gil' block, it'd probably be easiest to
>> transform the nogil function into a 'with gil' function.

Right.


>> If you want
>> to optimize for "paths going from with gil blocks directly into
>> function cleanup code" (i.e., no intermediate nogil try/finally or
>> nested with gil blocks), then you could use two more labels in the
>> code above that bypass the acquire.

In any case, jumping through labels is the normal way we do these things in
Cython, so this is worth cleaning up. And if we do all of this in the main
function code generation, it's much easier to figure out on what code paths
(i.e. at what labels) the GIL is really required for cleanup.

Stefan