memoryview slice transpose
I wanted to re-offer for discussion a fix for a bug I reported a while back. The code that breaks is this: import numpy cimport numpy def testfunc(): a = numpy.arange(12).reshape([3,4]) cdef numpy.int_t[:,:] a_view = a cdef numpy.int_t[:,:] b = a_view[:,:].T One problem is that when a memoryview slice is decremented, the .memview and .data fields are not set to NULL, and any error that causes a jump to cleanup code will attempt to decref it again. The second problem (I think) is that assignment of a transpose of memoryview slices to a variable is not resulting in an incref on the underlying slice, even though the variable is not a temporary and continues to persist. Here is a patch that solves the problem for me. https://github.com/SyamGadde/cython/commit/5739d8b908f18c4fc9103ef04e39964d6... The part I am least sure about is removing the "if" qualifications on the incref and decref: if self.obj.is_name or self.obj.is_attribute and self.obj.is_memslice_transpose: that was obviously there for a reason, but it causes problems in the above case. If there is a less extreme solution I'd be happy to take it. Thanks, -syam
Syam Gadde, 27.03.2014 21:29:
I wanted to re-offer for discussion a fix for a bug I reported a while back. The code that breaks is this:
import numpy cimport numpy
def testfunc(): a = numpy.arange(12).reshape([3,4]) cdef numpy.int_t[:,:] a_view = a cdef numpy.int_t[:,:] b = a_view[:,:].T
One problem is that when a memoryview slice is decremented, the .memview and .data fields are not set to NULL, and any error that causes a jump to cleanup code will attempt to decref it again.
The second problem (I think) is that assignment of a transpose of memoryview slices to a variable is not resulting in an incref on the underlying slice, even though the variable is not a temporary and continues to persist.
Here is a patch that solves the problem for me.
https://github.com/SyamGadde/cython/commit/5739d8b908f18c4fc9103ef04e39964d6...
Hmm, I can't see why the additional NULL setting would change anything. That's what the call to put_xdecref_memoryviewslice() already did that immediately precedes them (it actually calls "__Pyx_XDEC_MEMVIEW()" in MemoryView_C.c).
The part I am least sure about is removing the "if" qualifications on the incref and decref:
if self.obj.is_name or self.obj.is_attribute and self.obj.is_memslice_transpose:
that was obviously there for a reason, but it causes problems in the above case. If there is a less extreme solution I'd be happy to take it.
It's more likely that this was the change that made the difference. I can't say why the original code behaved that way, but it may well be that it misbehaves in an error case. Essentially, it doesn't incref the slice in certain cases (as an optimisation?), and if an error occurs, it might still end up trying to decref it. I think it's worth investigating that a bit further. Stefan
My patch actually addressed two slightly different issues, both of which have to do with refcounts of transposed slices. I'll address the first one below since that's the one that's not so obvious. Here's the code that causes an "Acquisition count is 0" error: import numpy cimport numpy def testfunc(): a = numpy.arange(12).reshape([3,4]) cdef numpy.int_t[:,:] a_view = a cdef numpy.int_t[:,:] b = a_view[:,:].T raise Exception() # comment this out and you still get a refcount error at a different line testfunc() On 04/06/2014 06:12 AM, Stefan Behnel wrote:
Syam Gadde, 27.03.2014 21:29:
One problem is that when a memoryview slice is decremented, the .memview and .data fields are not set to NULL, and any error that causes a jump to cleanup code will attempt to decref it again. Hmm, I can't see why the additional NULL setting would change anything. That's what the call to put_xdecref_memoryviewslice() already did that immediately precedes them (it actually calls "__Pyx_XDEC_MEMVIEW()" in MemoryView_C.c).
But only if the refcount of the memview goes to zero.Maybe that's what needs to change? See the case below (look for the *****'s). memviewslice objects are defined at the top of the function and on the stack, so they persist throughout the function. Several memviewslice objects may share the same base memview object. So if a memviewslice object finished doing what it needs to do, it needs to decrement the refcount of the memview object it's pointing to using __PYX_XDEC_MEMVIEW, *and* detach itself from the memview (that's the setting .memview = NULL part). The reason the explicit detaching is important is that there are extra __PYX_XDEC_MEMVIEW() calls in the error cleanup code, presumably to decrement the reference counts for still attached memviews in the case there is an error/exception. If the memview field is set to NULL whenever a slice is done with it, the extra calls to __PYX_XDEC_MEMVIEW are OK because it checks to see whether .memview is NULL before going forward. But currently, if an error/exception occurs while the slice is still attached to the memview, you can end up with a refcount < 0. If __PYX_XDEC_MEMVIEW did indeed set .memview to NULL on every call, then this wouldn't be necessary. But I have not tried that to see if it breaks something else. /* "testtranspose.pyx":7 * a = numpy.arange(12).reshape([3,4]) * cdef numpy.int_t[:,:] a_view = a * cdef numpy.int_t[:,:] b = a_view[:,:].T # <<<<<<<<<<<<<< * raise Exception() * */ __pyx_t_6 = -1; __pyx_t_5.data = __pyx_v_a_view.data; __pyx_t_5.memview = __pyx_v_a_view.memview; __PYX_INC_MEMVIEW(&__pyx_t_5, 0); __pyx_t_5.shape[0] = __pyx_v_a_view.shape[0]; __pyx_t_5.strides[0] = __pyx_v_a_view.strides[0]; __pyx_t_5.suboffsets[0] = -1; __pyx_t_5.shape[1] = __pyx_v_a_view.shape[1]; __pyx_t_5.strides[1] = __pyx_v_a_view.strides[1]; __pyx_t_5.suboffsets[1] = -1; __pyx_t_7 = __pyx_t_5; if (unlikely(__pyx_memslice_transpose(&__pyx_t_7) == 0)) {__pyx_filename = __p yx_f[0]; __pyx_lineno = 7; __pyx_clineno = __LINE__; goto __pyx_L1_error;} __PYX_XDEC_MEMVIEW(&__pyx_t_5, 1);//************************** __pyx_v_b = __pyx_t_7; __pyx_t_7.memview = NULL; __pyx_t_7.data = NULL; /* "testtranspose.pyx":8 * cdef numpy.int_t[:,:] a_view = a * cdef numpy.int_t[:,:] b = a_view[:,:].T * raise Exception() # <<<<<<<<<<<<<< * * testfunc() */ __pyx_t_1 = __Pyx_PyObject_Call(__pyx_builtin_Exception, __pyx_empty_tuple, NULL); if (unlikely(!__pyx_t_1)) {__pyx_filename = __pyx_f[0]; __pyx_lineno = 8; __pyx_clineno = __LINE__; goto __pyx_L1_error;} __Pyx_GOTREF(__pyx_t_1); __Pyx_Raise(__pyx_t_1, 0, 0, 0); __Pyx_DECREF(__pyx_t_1); __pyx_t_1 = 0; {__pyx_filename = __pyx_f[0]; __pyx_lineno = 8; __pyx_clineno = __LINE__; goto __pyx_L1_error;} /* "testtranspose.pyx":4 * cimport numpy * * def testfunc(): # <<<<<<<<<<<<<< * a = numpy.arange(12).reshape([3,4]) * cdef numpy.int_t[:,:] a_view = a */ /* function exit code */ __pyx_L1_error:; // ****** exception causes a jump to here ***** __Pyx_XDECREF(__pyx_t_1); __Pyx_XDECREF(__pyx_t_2); __Pyx_XDECREF(__pyx_t_3); __PYX_XDEC_MEMVIEW(&__pyx_t_4, 1); __PYX_XDEC_MEMVIEW(&__pyx_t_5, 1); //**************************** __PYX_XDEC_MEMVIEW(&__pyx_t_7, 1); __Pyx_AddTraceback("testtranspose.testfunc", __pyx_clineno, __pyx_lineno, __pyx_filename); __pyx_r = NULL; __Pyx_XDECREF(__pyx_v_a); __PYX_XDEC_MEMVIEW(&__pyx_v_a_view, 1); __PYX_XDEC_MEMVIEW(&__pyx_v_b, 1); __Pyx_XGIVEREF(__pyx_r); __Pyx_RefNannyFinishContext(); return __pyx_r;
participants (2)
-
Stefan Behnel -
Syam Gadde