Re: [Cython] [cython-users] What's up with PyEval_InitThreads() in python 2.7?
I'm going to reimplement this, but not for 0.16 anymore, I'd say. -------- Original-Message -------- Betreff: Re: [cython-users] What's up with PyEval_InitThreads() in python 2.7? Mike Cui, 28.02.2012 10:18:
Thanks for the test code, you hadn't mentioned that you use a "with gil" block. Could you try the latest github version of Cython?
Ahh, much better!
#if CYTHON_REFNANNY #ifdef WITH_THREAD __pyx_gilstate_save = PyGILState_Ensure(); #endif #endif /* CYTHON_REFNANNY */ __Pyx_RefNannySetupContext("callback"); #if CYTHON_REFNANNY #ifdef WITH_THREAD PyGILState_Release(__pyx_gilstate_save); #endif #endif /* CYTHON_REFNANNY */
Hmm, thanks for posting this - it can be further improved. There's no reason for code bloat here, it should all just go into the __Pyx_RefNannySetupContext() macro. Stefan
On 28 February 2012 09:54, Stefan Behnel <stefan_ml@behnel.de> 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. 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.
-------- Original-Message -------- Betreff: Re: [cython-users] What's up with PyEval_InitThreads() in python 2.7?
Mike Cui, 28.02.2012 10:18:
Thanks for the test code, you hadn't mentioned that you use a "with gil" block. Could you try the latest github version of Cython?
Ahh, much better!
#if CYTHON_REFNANNY #ifdef WITH_THREAD __pyx_gilstate_save = PyGILState_Ensure(); #endif #endif /* CYTHON_REFNANNY */ __Pyx_RefNannySetupContext("callback"); #if CYTHON_REFNANNY #ifdef WITH_THREAD PyGILState_Release(__pyx_gilstate_save); #endif #endif /* CYTHON_REFNANNY */
Hmm, thanks for posting this - it can be further improved. There's no reason for code bloat here, it should all just go into the __Pyx_RefNannySetupContext() macro.
Stefan _______________________________________________ cython-devel mailing list cython-devel@python.org http://mail.python.org/mailman/listinfo/cython-devel
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. 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. Stefan
On 28 February 2012 10:25, Stefan Behnel <stefan_ml@behnel.de> 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. 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.
Stefan _______________________________________________ cython-devel mailing list cython-devel@python.org http://mail.python.org/mailman/listinfo/cython-devel
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. 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. Stefan
On 28 February 2012 10:53, Stefan Behnel <stefan_ml@behnel.de> 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. 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,
Sorry, do you mean acquire?
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. The conditions don't just pertain to refnanny, nogil functions with with gil blocks and object arguments may also incref their arguments (admittedly, it doesn't check for the borrowed case, and always assumes the "incref to obtain a new reference" case). The same goes for cleanup, it acquires the GIL to clean up any temporaries and new references to function arguments. But there is indeed room for optimization, e.g. if there are no new references to object arguments and no error, then the GIL doesn't need to be acquired. It's all rather subtle and tricky, but I think I wrote decent tests at the time (but could always use more if the behaviour were to be changed). The cleanup could be further improved if each statement or each block would do its own local cleanup. So any object temporaries would be cleaned up before leaving the with gil block, etc. Anyway, changing anything more than just the refnanny macros or functions would probably have to wait until 0.16.1.
Stefan _______________________________________________ cython-devel mailing list cython-devel@python.org http://mail.python.org/mailman/listinfo/cython-devel
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. 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,
Sorry, do you mean acquire?
No, I meant release, at the exit points.
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.
The conditions don't just pertain to refnanny, nogil functions with with gil blocks and object arguments may also incref their arguments
I'm well aware of that.
The same goes for cleanup, it acquires the GIL to clean up any temporaries and new references to function arguments. But there is indeed room for optimization, e.g. if there are no new references to object arguments and no error, then the GIL doesn't need to be acquired. It's all rather subtle and tricky, but I think I wrote decent tests at the time (but could always use more if the behaviour were to be changed).
I'm not currently intending to change the behaviour, just the generated code.
The cleanup could be further improved if each statement or each block would do its own local cleanup. So any object temporaries would be cleaned up before leaving the with gil block, etc. Anyway, changing anything more than just the refnanny macros or functions would probably have to wait until 0.16.1.
Agreed. I'll see how my changes turn out, and then we can decide when to release them. Given that they don't really fix anything, just simplify the C code, I don't see a compelling enough reason to get them in for 0.16. Stefan
On 28 February 2012 13:50, Stefan Behnel <stefan_ml@behnel.de> 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. 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,
Sorry, do you mean acquire?
No, I meant release, at the exit points.
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. Changing the refnanny macro for the cleanup code will not avail anything, as the GIL is already ensured.
The conditions don't just pertain to refnanny, nogil functions with with gil blocks and object arguments may also incref their arguments
I'm well aware of that.
The same goes for cleanup, it acquires the GIL to clean up any temporaries and new references to function arguments. But there is indeed room for optimization, e.g. if there are no new references to object arguments and no error, then the GIL doesn't need to be acquired. It's all rather subtle and tricky, but I think I wrote decent tests at the time (but could always use more if the behaviour were to be changed).
I'm not currently intending to change the behaviour, just the generated code.
The cleanup could be further improved if each statement or each block would do its own local cleanup. So any object temporaries would be cleaned up before leaving the with gil block, etc. Anyway, changing anything more than just the refnanny macros or functions would probably have to wait until 0.16.1.
Agreed. I'll see how my changes turn out, and then we can decide when to release them. Given that they don't really fix anything, just simplify the C code, I don't see a compelling enough reason to get them in for 0.16.
Sure, good luck :) I'll be moving for the next couple of days, so I'll be rather preoccupied.
Stefan _______________________________________________ cython-devel mailing list cython-devel@python.org http://mail.python.org/mailman/listinfo/cython-devel
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
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. Looks like a bug to me. Stefan
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): """ /*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. Or, alternatively, add an additional exit path to the finally block that jumps to a label that acquires the GIL before going on to the original label. Something along those lines... Stefan
On 28 February 2012 20:19, Stefan Behnel <stefan_ml@behnel.de> 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.
(Sorry, dinner). 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(); goto L2: /* normal code */ return __pyx_r; } If the entire body is a 'with gil' block, it'd probably be easiest to transform the nogil function into a 'with gil' function. 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.
Or, alternatively, add an additional exit path to the finally block that jumps to a label that acquires the GIL before going on to the original label. Something along those lines...
Stefan _______________________________________________ cython-devel mailing list cython-devel@python.org http://mail.python.org/mailman/listinfo/cython-devel
On 28 February 2012 21:08, mark florisson <markflorisson88@gmail.com> wrote:
On 28 February 2012 20:19, Stefan Behnel <stefan_ml@behnel.de> 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.
(Sorry, dinner). 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(); goto L2: /* normal code */ return __pyx_r; }
And there should be a release just before the return of course :)
If the entire body is a 'with gil' block, it'd probably be easiest to transform the nogil function into a 'with gil' function. 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.
Or, alternatively, add an additional exit path to the finally block that jumps to a label that acquires the GIL before going on to the original label. Something along those lines...
Stefan _______________________________________________ cython-devel mailing list cython-devel@python.org http://mail.python.org/mailman/listinfo/cython-devel
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
On 28 February 2012 21:22, Stefan Behnel <stefan_ml@behnel.de> 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 cython-devel@python.org http://mail.python.org/mailman/listinfo/cython-devel
On 28 February 2012 19:58, Stefan Behnel <stefan_ml@behnel.de> wrote:
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.
Looks like a bug to me.
That's not a bug, that's how it is implemented. At setup, a variable __pyx_gilstate_save is declared, which is also used for teardown. Any with GIL blocks use C block scoping to do the same thing. The with block is itself a try/finally, so you get a try/finally wrapped in a try/finally. The code uses try/finally for it's way of trapping control flow, allowing some code to execute and resuming control flow afterwards.
Stefan _______________________________________________ cython-devel mailing list cython-devel@python.org http://mail.python.org/mailman/listinfo/cython-devel
mark florisson, 28.02.2012 21:20:
On 28 February 2012 19:58, Stefan Behnel wrote:
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.
Looks like a bug to me.
That's not a bug, that's how it is implemented. At setup, a variable __pyx_gilstate_save is declared, which is also used for teardown. Any with GIL blocks use C block scoping to do the same thing. The with block is itself a try/finally, so you get a try/finally wrapped in a try/finally. The code uses try/finally for it's way of trapping control flow, allowing some code to execute and resuming control flow afterwards.
Ok, so what really got me confused is that the code used the variable "acquire_gil_for_refnanny_only" in places that didn't have anything to do with the refnanny. Similarly, "acquire_gil_for_var_decls_only" was used for cleanup even though the GIL had already been released if this flag was set, way before the end of the function. I think I fixed both issues in the patch I attached. At least, it still passes the gil related tests and doesn't raise any C compiler warnings about the GIL state variable being unused. Does this look about right? Stefan
On 28 February 2012 21:08, Stefan Behnel <stefan_ml@behnel.de> wrote:
mark florisson, 28.02.2012 21:20:
On 28 February 2012 19:58, Stefan Behnel wrote:
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.
Looks like a bug to me.
That's not a bug, that's how it is implemented. At setup, a variable __pyx_gilstate_save is declared, which is also used for teardown. Any with GIL blocks use C block scoping to do the same thing. The with block is itself a try/finally, so you get a try/finally wrapped in a try/finally. The code uses try/finally for it's way of trapping control flow, allowing some code to execute and resuming control flow afterwards.
Ok, so what really got me confused is that the code used the variable "acquire_gil_for_refnanny_only" in places that didn't have anything to do with the refnanny. Similarly, "acquire_gil_for_var_decls_only" was used for cleanup even though the GIL had already been released if this flag was set, way before the end of the function.
I think I fixed both issues in the patch I attached. At least, it still passes the gil related tests and doesn't raise any C compiler warnings about the GIL state variable being unused.
Does this look about right?
It looks right to me, yeah. (<pedantic>I prefer to format bools directly with %d, as they are a subclass of int anyway</pedantic>). I think in general the EnsureGILNode should have been mentioned in the code generation function of FuncDefNode, which makes it easier to figure out what is going on. The documentation is currently at the wrapping site in AnalyseDeclarationsTransform.
Stefan
_______________________________________________ cython-devel mailing list cython-devel@python.org http://mail.python.org/mailman/listinfo/cython-devel
mark florisson, 28.02.2012 22:19:
On 28 February 2012 21:08, Stefan Behnel wrote:
mark florisson, 28.02.2012 21:20:
On 28 February 2012 19:58, Stefan Behnel wrote:
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.
Looks like a bug to me.
That's not a bug, that's how it is implemented. At setup, a variable __pyx_gilstate_save is declared, which is also used for teardown. Any with GIL blocks use C block scoping to do the same thing. The with block is itself a try/finally, so you get a try/finally wrapped in a try/finally. The code uses try/finally for it's way of trapping control flow, allowing some code to execute and resuming control flow afterwards.
Ok, so what really got me confused is that the code used the variable "acquire_gil_for_refnanny_only" in places that didn't have anything to do with the refnanny. Similarly, "acquire_gil_for_var_decls_only" was used for cleanup even though the GIL had already been released if this flag was set, way before the end of the function.
I think I fixed both issues in the patch I attached. At least, it still passes the gil related tests and doesn't raise any C compiler warnings about the GIL state variable being unused.
Does this look about right?
It looks right to me, yeah.
Ok. I think this looks simple enough to go into the release, whereas any more advanced cleanup and optimisations should have their time to mature. Would you object?
(<pedantic>I prefer to format bools directly with %d, as they are a subclass of int anyway</pedantic>).
I don't. It works when they are really booleans, but in Python, many things that are "true" or "false" aren't actually of type bool. When it comes to writing out (user visible) data, it's always best to make it clear what the intended output is, instead of relying on 'implementation details' and data of unknown sources (or different purposes, as in this case).
I think in general the EnsureGILNode should have been mentioned in the code generation function of FuncDefNode, which makes it easier to figure out what is going on. The documentation is currently at the wrapping site in AnalyseDeclarationsTransform.
I'll add a comment. Stefan
On 28 February 2012 21:38, Stefan Behnel <stefan_ml@behnel.de> wrote:
mark florisson, 28.02.2012 22:19:
On 28 February 2012 21:08, Stefan Behnel wrote:
mark florisson, 28.02.2012 21:20:
On 28 February 2012 19:58, Stefan Behnel wrote:
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.
Looks like a bug to me.
That's not a bug, that's how it is implemented. At setup, a variable __pyx_gilstate_save is declared, which is also used for teardown. Any with GIL blocks use C block scoping to do the same thing. The with block is itself a try/finally, so you get a try/finally wrapped in a try/finally. The code uses try/finally for it's way of trapping control flow, allowing some code to execute and resuming control flow afterwards.
Ok, so what really got me confused is that the code used the variable "acquire_gil_for_refnanny_only" in places that didn't have anything to do with the refnanny. Similarly, "acquire_gil_for_var_decls_only" was used for cleanup even though the GIL had already been released if this flag was set, way before the end of the function.
I think I fixed both issues in the patch I attached. At least, it still passes the gil related tests and doesn't raise any C compiler warnings about the GIL state variable being unused.
Does this look about right?
It looks right to me, yeah.
Ok. I think this looks simple enough to go into the release, whereas any more advanced cleanup and optimisations should have their time to mature. Would you object?
Sure, that's fine with me.
(<pedantic>I prefer to format bools directly with %d, as they are a subclass of int anyway</pedantic>).
I don't. It works when they are really booleans, but in Python, many things that are "true" or "false" aren't actually of type bool. When it comes to writing out (user visible) data, it's always best to make it clear what the intended output is, instead of relying on 'implementation details' and data of unknown sources (or different purposes, as in this case).
I think in general the EnsureGILNode should have been mentioned in the code generation function of FuncDefNode, which makes it easier to figure out what is going on. The documentation is currently at the wrapping site in AnalyseDeclarationsTransform.
I'll add a comment.
Stefan _______________________________________________ cython-devel mailing list cython-devel@python.org http://mail.python.org/mailman/listinfo/cython-devel
On 28 February 2012 21:08, Stefan Behnel <stefan_ml@behnel.de> wrote:
mark florisson, 28.02.2012 21:20:
On 28 February 2012 19:58, Stefan Behnel wrote:
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.
Looks like a bug to me.
That's not a bug, that's how it is implemented. At setup, a variable __pyx_gilstate_save is declared, which is also used for teardown. Any with GIL blocks use C block scoping to do the same thing. The with block is itself a try/finally, so you get a try/finally wrapped in a try/finally. The code uses try/finally for it's way of trapping control flow, allowing some code to execute and resuming control flow afterwards.
Ok, so what really got me confused is that the code used the variable "acquire_gil_for_refnanny_only" in places that didn't have anything to do with the refnanny. Similarly, "acquire_gil_for_var_decls_only" was used for cleanup even though the GIL had already been released if this flag was set, way before the end of the function.
Yeah those names weren't very apt, they are useful for the setup code but don't make sense for the teardown code :)
I think I fixed both issues in the patch I attached. At least, it still passes the gil related tests and doesn't raise any C compiler warnings about the GIL state variable being unused.
Does this look about right?
Stefan
_______________________________________________ cython-devel mailing list cython-devel@python.org http://mail.python.org/mailman/listinfo/cython-devel
participants (2)
-
mark florisson -
Stefan Behnel