[Cython] [cython-users] What's up with PyEval_InitThreads() in python 2.7?

[Stefan Behnel]

mark florisson, 28.02.2012 16:35:
> On 28 February 2012 13:50, Stefan Behnel wrote:
>> mark florisson, 28.02.2012 12:20:
>>> On 28 February 2012 10:53, Stefan Behnel wrote:
>>>> mark florisson, 28.02.2012 11:28:
>>>>> On 28 February 2012 10:25, Stefan Behnel wrote:
>>>>>> mark florisson, 28.02.2012 11:16:
>>>>>>> On 28 February 2012 09:54, Stefan Behnel wrote:
>>>>>>>> I'm going to reimplement this, but not for 0.16 anymore, I'd say.
>>>>>>>
>>>>>>> That's ok, I fixed it to not acquire the GIL seeing that control flow
>>>>>>> obsoletes None initialization. So you might as well move it into the
>>>>>>> setup function if you care, the thing is that that would needlessly
>>>>>>> acquire the GIL for the common case (when you have the GIL) in the
>>>>>>> tests, so it might slow them down.

Regarding this bit, we might just use a local temp variable to remember
when we have acquired the GIL already. The C compiler should be able to
figure out its value at each point in the code and drop unnecessary GIL
handling code accordingly.


>>>>>>> It would be better to create a
>>>>>>> __Pyx_RefNannySetupContextNogil() function wrapper. If you're not
>>>>>>> running the code as a test the preprocessor would filter out this
>>>>>>> bloat though, so it's really not a very big deal anyway.
>>>>>>
>>>>>> I was going to pass a constant flag into the macro that would let the C
>>>>>> compiler do the right thing:
>>>>>>
>>>>>> """
>>>>>> #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), ...) \
>>>>>>              PyGILState_Release(__pyx_gilstate_save); \
>>>>>>          } else { \
>>>>>>              __pyx_refnanny = __Pyx_RefNanny->SetupContext((name), ...) \
>>>>>>          }
>>>>>> #else
>>>>>>  #define __Pyx_RefNannySetupContext(name, acquire_gil) \
>>>>>>          __pyx_refnanny = __Pyx_RefNanny->SetupContext((name), ...)
>>>>>> #endif
>>>>>> """
>>>>>>
>>>>>> That also gets rid of the need to declare the "save" variable independently.
>>>>>
>>>>> I don't think that will work, I think the teardown re-uses the save variable.
>>>>
>>>> Well, it doesn't *have* to be the same variable, though.
>>>>
>>>> Speaking of that, BTW, there are a couple of places in the code (Nodes.py,
>>>> from line 1500 on) that release the GIL, and some of them, specifically
>>>> some error cases, look like they are using the wrong conditions to decide
>>>> what they need to do in order to achieve that. I think this is most easily
>>>> fixed by using the above kind of macro also for the refnanny cleanup call.
>>>
>>> If you could be more specific as to how the conditions might be wrong,
>>> that would be helpful.
>>
>> I was thinking about the use of the "acquire_gil_for_refnanny_only" flag at
>> the end, but maybe I got confused by the immense complexity of the function
>> exit generation code, with its several places where the GIL can get
>> acquired. I think this is worth some refactoring...
>>
>> In any case, the "acquire_gil_for_refnanny_only" flag can easily be used to
>> trigger the necessary acquire-release code in the refnanny macros (as in
>> the code above). That's at least a tiny first step to reducing this complexity.
> 
> 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.

Hmm, that sounds a bit fragile, though. I think I'll leave it with the
change of moving the entry code into the macro, just to drop the C code
overhead a little. That was my primary intention anyway.


> Changing the refnanny macro for the cleanup
> code will not avail anything, as the GIL is already ensured.

Right, I noticed that, too.

Stefan