Memslice nogil tests, better acquisition counting

Cython/Compiler/Code.py

@@ -462,10 +462,11 @@ class FunctionState(object):
 
         A C string referring to the variable is returned.
         """
-        if not type.is_pyobject:
+        if not type.is_pyobject and not type.is_memoryviewslice:
             # Make manage_ref canonical, so that manage_ref will always mean
             # a decref is needed.
             manage_ref = False
+
         freelist = self.temps_free.get((type, manage_ref))
         if freelist is not None and len(freelist) > 0:
             result = freelist.pop()
@@ -508,7 +509,7 @@ class FunctionState(object):
         for name, type, manage_ref in self.temps_allocated:
             freelist = self.temps_free.get((type, manage_ref))
             if freelist is None or name not in freelist:
-                used.append((name, type, manage_ref))
+                used.append((name, type, manage_ref and type.is_pyobject))
         return used
 
     def temps_holding_reference(self):
@@ -518,7 +519,7 @@ class FunctionState(object):
         """
         return [(name, type)
                 for name, type, manage_ref in self.temps_in_use()
-                if manage_ref]
+                if manage_ref  and type.is_pyobject]
 
     def all_managed_temps(self):
         """Return a list of (cname, type) tuples of refcount-managed Python objects.
@@ -1464,6 +1465,8 @@ class CCodeWriter(object):
             decl = type.declaration_code(name)
             if type.is_pyobject:
                 self.putln("%s = NULL;" % decl)
+            elif type.is_memoryviewslice:
+                self.putln("%s = { 0 };" % decl)
             else:
                 self.putln("%s;" % decl)
 
@@ -1544,13 +1547,21 @@ class CCodeWriter(object):
             self.putln("Py_DECREF(%s); %s = 0;" % (
                 typecast(py_object_type, type, cname), cname))
 
-    def put_xdecref(self, cname, type, nanny=True):
+    def put_xdecref(self, cname, type, nanny=True, have_gil=True):
+        if type.is_memoryviewslice:
+            self.put_xdecref_memoryviewslice(cname, have_gil=have_gil)
+            return
+
         if nanny:
             self.putln("__Pyx_XDECREF(%s);" % self.as_pyobject(cname, type))
         else:
             self.putln("Py_XDECREF(%s);" % self.as_pyobject(cname, type))
 
     def put_xdecref_clear(self, cname, type, nanny=True):
+        if type.is_memoryviewslice:
+            self.put_xdecref_memoryviewslice(cname)
+            return
+
         if nanny:
             self.putln("__Pyx_XDECREF(%s); %s = 0;" % (
                 self.as_pyobject(cname, type), cname))

Cython/Compiler/ExprNodes.py

@@ -504,8 +504,12 @@ class ExprNode(Node):
 
     def generate_disposal_code(self, code):
         if self.is_temp:
-            if self.type.is_pyobject and self.result():
+            if self.result():
+                if self.type.is_pyobject:
                     code.put_decref_clear(self.result(), self.ctype())
+                elif self.type.is_memoryviewslice:
+                    code.put_xdecref_memoryviewslice(
+                            self.result(), have_gil=not self.in_nogil_context)
         else:
             # Already done if self.is_temp
             self.generate_subexpr_disposal_code(code)
@@ -520,6 +524,9 @@ class ExprNode(Node):
         if self.is_temp:
             if self.type.is_pyobject:
                 code.putln("%s = 0;" % self.result())
+            elif self.type.is_memoryviewslice:
+                code.putln("%s.memview = NULL;" % self.result())
+                code.putln("%s.data = NULL;" % self.result())
         else:
             self.generate_subexpr_disposal_code(code)
 
@@ -1477,6 +1484,7 @@ class NameNode(AtomicExprNode):
         elif entry.type.is_memoryviewslice:
             self.is_temp = False
             self.is_used_as_rvalue = True
+            self.use_managed_ref = True
 
     def nogil_check(self, env):
         self.nogil = True
@@ -1635,6 +1643,11 @@ class NameNode(AtomicExprNode):
             raise_unbound = (
                 (self.cf_maybe_null or self.cf_is_null) and not self.allow_null)
             null_code = entry.type.check_for_null_code(entry.cname)
+
+            # if entry.type.is_memoryviewslice:
+            #     have_gil = not self.in_nogil_context
+            #     code.put_incref_memoryviewslice(entry.cname, have_gil)
+
             memslice_check = entry.type.is_memoryviewslice and self.initialized_check
 
             if null_code and raise_unbound and (entry.type.is_pyobject or memslice_check):
@@ -1732,12 +1745,15 @@ class NameNode(AtomicExprNode):
                     print("NameNode.generate_assignment_code:")
                     print("...generating post-assignment code for %s" % rhs)
                 rhs.generate_post_assignment_code(code)
+            elif rhs.result_in_temp():
+                rhs.generate_post_assignment_code(code)
+
             rhs.free_temps(code)
 
     def generate_acquire_memoryviewslice(self, rhs, code):
         """
-        If the value was coerced to a memoryviewslice, its a new reference.
-        Otherwise we're simply using a borrowed reference from another slice
+        Slices, coercions from objects, return values etc are new references.
+        We have a borrowed reference in case of dst = src
         """
         import MemoryView
 
@@ -1747,7 +1763,8 @@ class NameNode(AtomicExprNode):
             lhs_pos=self.pos,
             rhs=rhs,
             code=code,
-            incref_rhs=not isinstance(rhs, CoerceToMemViewSliceNode))
+            incref_rhs=rhs.is_name,
+            have_gil=not self.nogil)
 
     def generate_acquire_buffer(self, rhs, code):
         # rhstmp is only used in case the rhs is a complicated expression leading to
@@ -2469,9 +2486,7 @@ class IndexNode(ExprNode):
 
                 elif index.type.is_int:
                     self.memslice_index = True
-                    index = index.coerce_to(index_type, env)\
-                    #.coerce_to_temp(
-                    #                                             index_type)
+                    index = index.coerce_to(index_type, env)
                     indices[i] = index
                     new_indices.append(index)
 
@@ -2488,6 +2503,8 @@ class IndexNode(ExprNode):
 
             self.memslice_index = self.memslice_index and not self.memslice_slice
             self.original_indices = indices
+            # All indices with all start/stop/step for slices.
+            # We need to keep this around
             self.indices = new_indices
 
             self.env = env
@@ -2540,7 +2557,9 @@ class IndexNode(ExprNode):
                 error(self.pos, "memoryviews currently support setting only.")
 
         elif self.memslice_slice:
+            self.index = None
             self.is_temp = True
+            self.use_managed_ref = True
             self.type = PyrexTypes.MemoryViewSliceType(
                             self.base.type.dtype, axes)
 
@@ -2618,8 +2637,8 @@ class IndexNode(ExprNode):
     gil_message = "Indexing Python object"
 
     def nogil_check(self, env):
-        if self.is_buffer_access or self.memslice_index:
-            if env.directives['boundscheck']:
+        if self.is_buffer_access or self.memslice_index or self.memslice_slice:
+            if not self.memslice_slice and env.directives['boundscheck']:
                 error(self.pos, "Cannot check buffer index bounds without gil; use boundscheck(False) directive")
                 return
             elif self.type.is_pyobject:
@@ -2887,12 +2906,13 @@ class IndexNode(ExprNode):
 
     def put_memoryviewslice_slice_code(self, code):
         buffer_entry = self.buffer_entry()
+        have_gil = not self.in_nogil_context
         buffer_entry.generate_buffer_slice_code(code,
                                                 self.original_indices,
                                                 self.base.type,
                                                 self.type,
                                                 self.result(),
-                                                have_gil = not self.env.nogil)
+                                                have_gil=have_gil)
 
     def put_nonecheck(self, code):
         code.globalstate.use_utility_code(raise_noneindex_error_utility_code)
@@ -6206,8 +6226,6 @@ class CythonArrayNode(ExprNode):
         import MemoryView
 
         self.type = error_type
-        self.env = env
-
         self.shapes = []
 
         for axis_no, axis in enumerate(self.base_type_node.axes):
@@ -8004,8 +8022,9 @@ class CoerceToMemViewSliceNode(CoercionNode):
         assert not arg.type.is_memoryviewslice
         CoercionNode.__init__(self, arg)
         self.type = dst_type
-        self.env = env
         self.is_temp = 1
+        self.env = env
+        self.use_managed_ref = True
         self.arg = arg
 
     def generate_result_code(self, code):
@@ -8017,10 +8036,6 @@ class CoerceToMemViewSliceNode(CoercionNode):
         error_cond = self.type.error_condition(self.result())
         code.putln(code.error_goto_if(error_cond, self.pos))
 
-    def generate_disposal_code(self, code):
-        code.put_xdecref_memoryviewslice(self.result(),
-                                         have_gil=not self.env.nogil)
-
 
 class CastNode(CoercionNode):
     #  Wrap a node in a C type cast.

Cython/Compiler/MemoryView.py

@@ -79,7 +79,7 @@ def mangle_dtype_name(dtype):
 #    return "".join([access[0].upper()+packing[0] for (access, packing) in axes])
 
 def put_acquire_memoryviewslice(lhs_cname, lhs_type, lhs_pos, rhs, code,
-                                incref_rhs=True, have_gil=False):
+                                incref_rhs=False, have_gil=False):
     assert rhs.type.is_memoryviewslice
 
     pretty_rhs = isinstance(rhs, NameNode) or rhs.result_in_temp()
@@ -97,18 +97,20 @@ def put_acquire_memoryviewslice(lhs_cname, lhs_type, lhs_pos, rhs, code,
         code.funcstate.release_temp(rhstmp)
 
 def put_assign_to_memviewslice(lhs_cname, rhs_cname, memviewslicetype, code,
-                               incref_rhs=True):
+                               incref_rhs=False):
     code.put_xdecref_memoryviewslice(lhs_cname)
     if incref_rhs:
         code.put_incref_memoryviewslice(rhs_cname)
 
-    code.putln("%s.memview = %s.memview;" % (lhs_cname, rhs_cname))
-    code.putln("%s.data = %s.data;" % (lhs_cname, rhs_cname))
-    for i in range(memviewslicetype.ndim):
-        tup = (lhs_cname, i, rhs_cname, i)
-        code.putln("%s.shape[%d] = %s.shape[%d];" % tup)
-        code.putln("%s.strides[%d] = %s.strides[%d];" % tup)
-        code.putln("%s.suboffsets[%d] = %s.suboffsets[%d];" % tup)
+    code.putln("%s = %s;" % (lhs_cname, rhs_cname))
+
+    #code.putln("%s.memview = %s.memview;" % (lhs_cname, rhs_cname))
+    #code.putln("%s.data = %s.data;" % (lhs_cname, rhs_cname))
+    #for i in range(memviewslicetype.ndim):
+    #    tup = (lhs_cname, i, rhs_cname, i)
+    #    code.putln("%s.shape[%d] = %s.shape[%d];" % tup)
+    #    code.putln("%s.strides[%d] = %s.strides[%d];" % tup)
+    #    code.putln("%s.suboffsets[%d] = %s.suboffsets[%d];" % tup)
 
 def get_buf_flags(specs):
     is_c_contig, is_f_contig = is_cf_contig(specs)
@@ -264,9 +266,9 @@ class MemoryViewSliceBufferEntry(Buffer.BufferEntry):
                 code.putln(    "if (unlikely(__pyx_t_result)) {")
                 code.put_ensure_gil()
                 code.putln(        "PyErr_Format(PyExc_IndexError, "
-                                                "__pyx_t_result, %d)" % dim)
+                                                "__pyx_t_result, %d);" % dim)
                 code.put_release_ensured_gil()
-                code.putln(code.goto_error(pos))
+                code.putln(code.error_goto(pos))
                 code.putln(    "}")
 
                 code.putln("}")
@@ -274,6 +276,7 @@ class MemoryViewSliceBufferEntry(Buffer.BufferEntry):
         code.putln("%s = -1;" % suboffset_dim)
         code.putln("%(dst)s.data = %(cname)s.data;" % locals())
         code.putln("%(dst)s.memview = %(cname)s.memview;" % locals())
+        code.put_incref_memoryviewslice(dst)
 
         for dim, index in enumerate(indices):
             if not isinstance(index, ExprNodes.SliceNode):
@@ -295,6 +298,7 @@ class MemoryViewSliceBufferEntry(Buffer.BufferEntry):
 
         code.funcstate.release_temp(suboffset_dim)
 
+
 def empty_slice(pos):
     none = ExprNodes.NoneNode(pos)
     return ExprNodes.SliceNode(pos, start=none,

Cython/Compiler/Nodes.py

@@ -1495,7 +1495,7 @@ class FuncDefNode(StatNode, BlockNode):
             code.put_goto(code.return_label)
             code.put_label(code.error_label)
             for cname, type in code.funcstate.all_managed_temps():
-                code.put_xdecref(cname, type)
+                code.put_xdecref(cname, type, have_gil=not lenv.nogil)
 
             # Clean up buffers -- this calls a Python function
             # so need to save and restore error state
@@ -4347,8 +4347,8 @@ class ReturnStatNode(StatNode):
         if self.return_type.is_pyobject:
             code.put_xdecref(Naming.retval_cname,
                              self.return_type)
-        elif self.return_type.is_memoryviewslice:
-            code.put_xdecref_memoryviewslice(Naming.retval_cname)
+        #elif self.return_type.is_memoryviewslice:
+        #    code.put_xdecref_memoryviewslice(Naming.retval_cname)
 
         if self.value:
             self.value.generate_evaluation_code(code)
@@ -4359,7 +4359,9 @@ class ReturnStatNode(StatNode):
                         lhs_type=self.return_type,
                         lhs_pos=self.value.pos,
                         rhs=self.value,
-                        code=code)
+                        code=code,
+                        incref_rhs=True,
+                        have_gil=self.in_nogil_context)
             else:
                 self.value.make_owned_reference(code)
                 code.putln(

Cython/Compiler/ParseTreeTransforms.py

@@ -2223,6 +2223,7 @@ class GilCheck(VisitorTransform):
         if self.env_stack and self.nogil and node.nogil_check:
             node.nogil_check(self.env_stack[-1])
         self.visitchildren(node)
+        node.in_nogil_context = self.nogil
         return node
 
 

Cython/Utility/MemoryView.pyx

@@ -460,16 +460,6 @@ cdef extern from "pystate.h":
     void PyErr_SetString(PyObject *type, char *msg) nogil
     PyObject *PyErr_Format(PyObject *exc, char *msg, ...) nogil
 
-
-cdef:
-    char *ERR_OOB = "Index out of bounds (axis %d)"
-    char *ERR_STEP = "Step must not be zero (axis %d)"
-    char *ERR_INDIRECT_GIL = ("Cannot make indirect dimension %d disappear "
-                              "through indexing, consider slicing with %d:%d")
-    char *ERR_INDIRECT_NOGIL = ("Cannot make indirect dimension %d disappear "
-                                "through indexing")
-    PyObject *exc = <PyObject *> IndexError
-
 @cname('__pyx_memoryview_slice_memviewslice')
 cdef char *slice_memviewslice({{memviewslice_name}} *src,
                               {{memviewslice_name}} *dst,
@@ -500,6 +490,15 @@ cdef char *slice_memviewslice({{memviewslice_name}} *src,
         Py_ssize_t new_shape
         bint negative_step
 
+        # Somehow these pointers are NULL when set as globals... this needs investigation
+        char *ERR_OOB = "Index out of bounds (axis %d)"
+        char *ERR_STEP = "Step must not be zero (axis %d)"
+        char *ERR_INDIRECT_GIL = ("Cannot make indirect dimension %d disappear "
+                              "through indexing, consider slicing with %d:%d")
+        char *ERR_INDIRECT_NOGIL = ("Cannot make indirect dimension %d disappear "
+                                "through indexing")
+        PyObject *exc = <PyObject *> IndexError
+
     if have_gil:
         # Assert the GIL
         PyThreadState_Get()

tests/run/memslice.pyx

@@ -1330,3 +1330,73 @@ def test_oob():
     """
     cdef int[:, :] a = IntMockBuffer("A", range(4 * 9), shape=(4, 9))
     print a[:, 20]
+
+
+cdef int nogil_oob(int[:, :] a) nogil except 0:
+    a[100, 9:]
+    return 1
+
+@testcase
+def test_nogil_oob1():
+    """
+    A is acquired at the beginning of the function and released at the end.
+    B is acquired as a temporary and as such is immediately released in the
+    except clause.
+    >>> test_nogil_oob1()
+    acquired A
+    acquired B
+    released B
+    Index out of bounds (axis 0)
+    Index out of bounds (axis 0)
+    released A
+    """
+    cdef int[:, :] a = IntMockBuffer("A", range(4 * 9), shape=(4, 9))
+
+    try:
+        nogil_oob(IntMockBuffer("B", range(4 * 9), shape=(4, 9)))
+    except IndexError, e:
+        print e.args[0]
+
+    try:
+        # Enable when the nogil exception propagation fix is merged
+        #with nogil:
+        nogil_oob(a)
+    except IndexError, e:
+        print e.args[0]
+
+@testcase
+def test_nogil_oob2():
+    """
+    >>> test_nogil_oob2()
+    Traceback (most recent call last):
+       ...
+    IndexError: Index out of bounds (axis 0)
+    """
+    cdef int[:, :] a = IntMockBuffer("A", range(4 * 9), shape=(4, 9))
+    with nogil:
+        a[100, 9:]
+
+@cython.boundscheck(False)
+cdef int cdef_nogil(int[:, :] a) nogil except 0:
+    cdef int i, j
+    cdef int[:, :] b = a[::-1, 3:10:2]
+    for i in range(b.shape[0]):
+        for j in range(b.shape[1]):
+            b[i, j] = -b[i, j]
+
+    return 1
+
+@testcase
+def test_nogil():
+    """
+    >>> test_nogil()
+    acquired A
+    released A
+    acquired A
+    -25
+    released A
+    """
+    _a = IntMockBuffer("A", range(4 * 9), shape=(4, 9))
+    cdef_nogil(_a)
+    cdef int[:, :] a = _a
+    print a[2, 7]