On 28 February 2012 21:22, Stefan Behnel <[email protected]> wrote: > 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.
A cleanup along with any optimizations would be great, +1 from me. > Stefan > _______________________________________________ > cython-devel mailing list > [email protected] > http://mail.python.org/mailman/listinfo/cython-devel _______________________________________________ cython-devel mailing list [email protected] http://mail.python.org/mailman/listinfo/cython-devel
