Fix buffer resource management

msimacek · msimacek · commit 491c23462236 · 2020-10-29T10:18:26.000+01:00
diff --git a/graalpython/com.oracle.graal.python.cext/src/memoryobject.c b/graalpython/com.oracle.graal.python.cext/src/memoryobject.c
@@ -40,6 +40,45 @@
  */
 #include "capi.h"
 
+#if SIZEOF_SIZE_T == 8
+#define polyglot_from_size_array polyglot_from_i64_array
+#elif SIZEOF_SIZE_T == 4
+#define polyglot_from_size_array polyglot_from_i32_array
+#endif
+
+/* Macros taken from CPython */
+/* Memoryview buffer properties */
+#define MV_C_CONTIGUOUS(flags) (flags&(_Py_MEMORYVIEW_SCALAR|_Py_MEMORYVIEW_C))
+#define MV_F_CONTIGUOUS(flags) \
+    (flags&(_Py_MEMORYVIEW_SCALAR|_Py_MEMORYVIEW_FORTRAN))
+#define MV_ANY_CONTIGUOUS(flags) \
+    (flags&(_Py_MEMORYVIEW_SCALAR|_Py_MEMORYVIEW_C|_Py_MEMORYVIEW_FORTRAN))
+
+/* getbuffer() requests */
+#define REQ_INDIRECT(flags) ((flags&PyBUF_INDIRECT) == PyBUF_INDIRECT)
+#define REQ_C_CONTIGUOUS(flags) ((flags&PyBUF_C_CONTIGUOUS) == PyBUF_C_CONTIGUOUS)
+#define REQ_F_CONTIGUOUS(flags) ((flags&PyBUF_F_CONTIGUOUS) == PyBUF_F_CONTIGUOUS)
+#define REQ_ANY_CONTIGUOUS(flags) ((flags&PyBUF_ANY_CONTIGUOUS) == PyBUF_ANY_CONTIGUOUS)
+#define REQ_STRIDES(flags) ((flags&PyBUF_STRIDES) == PyBUF_STRIDES)
+#define REQ_SHAPE(flags) ((flags&PyBUF_ND) == PyBUF_ND)
+#define REQ_WRITABLE(flags) (flags&PyBUF_WRITABLE)
+#define REQ_FORMAT(flags) (flags&PyBUF_FORMAT)
+
+#define BASE_INACCESSIBLE(mv) \
+    (((PyMemoryViewObject *)mv)->flags&_Py_MEMORYVIEW_RELEASED)
+#define CHECK_RELEASED(mv) \
+    if (BASE_INACCESSIBLE(mv)) {                                  \
+        PyErr_SetString(PyExc_ValueError,                         \
+            "operation forbidden on released memoryview object"); \
+        return NULL;                                              \
+    }
+#define CHECK_RELEASED_INT(mv) \
+    if (BASE_INACCESSIBLE(mv)) {                                  \
+        PyErr_SetString(PyExc_ValueError,                         \
+            "operation forbidden on released memoryview object"); \
+        return -1;                                                \
+    }
+
 PyTypeObject PyMemoryView_Type = PY_TRUFFLE_TYPE_WITH_ITEMSIZE("memoryview", &PyType_Type, Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_GC, offsetof(PyMemoryViewObject, ob_array), sizeof(Py_ssize_t));
 PyTypeObject PyBuffer_Type = PY_TRUFFLE_TYPE("buffer", &PyType_Type, Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_GC, sizeof(PyBufferDecorator));
 
@@ -59,13 +98,36 @@ PyObject* PyMemoryView_FromObject(PyObject *v) {
 
 /* called back from the above upcall only if the object was native */
 PyObject* PyTruffle_MemoryViewFromObject(PyObject *v) {
-    Py_buffer buffer;
     if (PyObject_CheckBuffer(v)) {
-        PyObject *ret;
-        if (PyObject_GetBuffer(v, &buffer, PyBUF_FULL_RO) < 0) {
+        Py_buffer* buffer = malloc(sizeof(Py_buffer));
+        if (PyObject_GetBuffer(v, buffer, PyBUF_FULL_RO) < 0) {
             return NULL;
         }
-        return PyMemoryView_FromBuffer(&buffer);
+        Py_ssize_t ndim = buffer->ndim;
+        int needs_release = 0;
+        if (buffer->obj != NULL) {
+            PyBufferProcs *pb;
+            pb = Py_TYPE(buffer->obj)->tp_as_buffer;
+            if (pb) {
+                needs_release = pb->bf_releasebuffer != NULL;
+            }
+        }
+        PyObject *mv = polyglot_invoke(PY_TRUFFLE_CEXT, "PyTruffle_MemoryViewFromBuffer",
+                needs_release ? buffer : NULL, /* We only need the ptr for the release */
+                native_to_java(buffer->obj),
+                buffer->len,
+                buffer->readonly,
+                buffer->itemsize,
+                polyglot_from_string(buffer->format ? buffer->format : "B", "ascii"),
+                buffer->ndim,
+                polyglot_from_i8_array(buffer->buf, buffer->len),
+                buffer->shape ? polyglot_from_size_array(buffer->shape, ndim) : NULL,
+                buffer->strides ? polyglot_from_size_array(buffer->strides, ndim) : NULL,
+                buffer->suboffsets ? polyglot_from_size_array(buffer->suboffsets, ndim) : NULL);
+        if (!needs_release) {
+            free(buffer);
+        }
+        return mv;
     }
 
     PyErr_Format(PyExc_TypeError,
@@ -74,27 +136,23 @@ PyObject* PyTruffle_MemoryViewFromObject(PyObject *v) {
     return NULL;
 }
 
-#if SIZEOF_SIZE_T == 8
-#define polyglot_from_size_array polyglot_from_i64_array
-#elif SIZEOF_SIZE_T == 4
-#define polyglot_from_size_array polyglot_from_i32_array
-#endif
-
-
-PyObject* PyMemoryView_FromBuffer(Py_buffer *buffer) {
-    Py_ssize_t ndim = buffer->ndim;
-    releasebufferproc releasefn = NULL;
+/* Release buffer struct allocated in PyTruffle_MemoryViewFromObject */
+void PyTruffle_ReleaseBuffer(Py_buffer* buffer) {
     if (buffer->obj != NULL) {
         PyBufferProcs *pb;
         pb = Py_TYPE(buffer->obj)->tp_as_buffer;
         if (pb) {
-            releasefn = pb->bf_releasebuffer;
+            pb->bf_releasebuffer(buffer->obj, buffer);
         }
     }
+    free(buffer);
+}
+
+PyObject* PyMemoryView_FromBuffer(Py_buffer *buffer) {
+    Py_ssize_t ndim = buffer->ndim;
     return polyglot_invoke(PY_TRUFFLE_CEXT, "PyTruffle_MemoryViewFromBuffer",
-            buffer,
-            native_to_java(buffer->obj),
-            releasefn,
+            NULL,
+            NULL,
             buffer->len,
             buffer->readonly,
             buffer->itemsize,
@@ -111,47 +169,14 @@ PyObject *PyMemoryView_FromMemory(char *mem, Py_ssize_t size, int flags) {
     assert(flags == PyBUF_READ || flags == PyBUF_WRITE);
     int readonly = (flags == PyBUF_WRITE) ? 0 : 1;
     return polyglot_invoke(PY_TRUFFLE_CEXT, "PyTruffle_MemoryViewFromBuffer",
-            NULL, NULL, NULL, size, readonly, 1, polyglot_from_string("B", "ascii"), 1, polyglot_from_i8_array((int8_t*)mem, size), NULL, NULL, NULL);
+            NULL, NULL, size, readonly, 1, polyglot_from_string("B", "ascii"), 1, polyglot_from_i8_array((int8_t*)mem, size), NULL, NULL, NULL);
 }
 
 UPCALL_ID(PyMemoryView_GetContiguous)
 PyObject* PyMemoryView_GetContiguous(PyObject *obj, int buffertype, char order) {
     return UPCALL_CEXT_O(_jls_PyMemoryView_GetContiguous, native_to_java(obj), buffertype, (int)order);
 }
 
-/* Macros taken from CPython */
-/* Memoryview buffer properties */
-#define MV_C_CONTIGUOUS(flags) (flags&(_Py_MEMORYVIEW_SCALAR|_Py_MEMORYVIEW_C))
-#define MV_F_CONTIGUOUS(flags) \
-    (flags&(_Py_MEMORYVIEW_SCALAR|_Py_MEMORYVIEW_FORTRAN))
-#define MV_ANY_CONTIGUOUS(flags) \
-    (flags&(_Py_MEMORYVIEW_SCALAR|_Py_MEMORYVIEW_C|_Py_MEMORYVIEW_FORTRAN))
-
-/* getbuffer() requests */
-#define REQ_INDIRECT(flags) ((flags&PyBUF_INDIRECT) == PyBUF_INDIRECT)
-#define REQ_C_CONTIGUOUS(flags) ((flags&PyBUF_C_CONTIGUOUS) == PyBUF_C_CONTIGUOUS)
-#define REQ_F_CONTIGUOUS(flags) ((flags&PyBUF_F_CONTIGUOUS) == PyBUF_F_CONTIGUOUS)
-#define REQ_ANY_CONTIGUOUS(flags) ((flags&PyBUF_ANY_CONTIGUOUS) == PyBUF_ANY_CONTIGUOUS)
-#define REQ_STRIDES(flags) ((flags&PyBUF_STRIDES) == PyBUF_STRIDES)
-#define REQ_SHAPE(flags) ((flags&PyBUF_ND) == PyBUF_ND)
-#define REQ_WRITABLE(flags) (flags&PyBUF_WRITABLE)
-#define REQ_FORMAT(flags) (flags&PyBUF_FORMAT)
-
-#define BASE_INACCESSIBLE(mv) \
-    (((PyMemoryViewObject *)mv)->flags&_Py_MEMORYVIEW_RELEASED)
-#define CHECK_RELEASED(mv) \
-    if (BASE_INACCESSIBLE(mv)) {                                  \
-        PyErr_SetString(PyExc_ValueError,                         \
-            "operation forbidden on released memoryview object"); \
-        return NULL;                                              \
-    }
-#define CHECK_RELEASED_INT(mv) \
-    if (BASE_INACCESSIBLE(mv)) {                                  \
-        PyErr_SetString(PyExc_ValueError,                         \
-            "operation forbidden on released memoryview object"); \
-        return -1;                                                \
-    }
-
 /* Taken from CPython memoryobject.c: memory_getbuf */
 int memoryview_getbuffer(PyMemoryViewObject *self, Py_buffer *view, int flags)
 {
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/BuiltinConstructors.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/BuiltinConstructors.java
@@ -3377,7 +3377,7 @@ private static IntrinsifiedPMemoryView fromManaged(Object object, int itemsize,
             ManagedBuffer managedBuffer = null;
             if (needsRelease) {
                 // TODO We should lock the underlying storage for resizing
-                managedBuffer = new ManagedBuffer(object);
+                managedBuffer = ManagedBuffer.createForManaged(object);
             }
             return new IntrinsifiedPMemoryView(PythonBuiltinClassType.PMemoryView, PythonBuiltinClassType.PMemoryView.getInstanceShape(),
                             refQueue, managedBuffer, object, length * itemsize, readonly, itemsize, format, 1,
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/PythonCextBuiltins.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/PythonCextBuiltins.java
@@ -1563,12 +1563,12 @@ Object wrap(VirtualFrame frame, Object object,
     }
 
     // Called without landing node
-    @Builtin(name = NativeCAPISymbols.FUN_PY_TRUFFLE_MEMORYVIEW_FROM_BUFFER, minNumOfPositionalArgs = 12)
+    @Builtin(name = NativeCAPISymbols.FUN_PY_TRUFFLE_MEMORYVIEW_FROM_BUFFER, minNumOfPositionalArgs = 11)
     @GenerateNodeFactory
     abstract static class PyTruffle_MemoryViewFromBuffer extends NativeBuiltin {
 
         @Specialization
-        Object wrap(VirtualFrame frame, Object bufferStructPointer, Object ownerObj, Object releasefn, Object lenObj,
+        Object wrap(VirtualFrame frame, Object bufferStructPointer, Object ownerObj, Object lenObj,
                         Object readonlyObj, Object itemsizeObj, Object formatObj,
                         Object ndimObj, Object bufPointer, Object shapePointer, Object stridesPointer, Object suboffsetsPointer,
                         @Cached MemoryViewNodes.InitFlagsNode initFlagsNode,
@@ -1618,8 +1618,8 @@ Object wrap(VirtualFrame frame, Object bufferStructPointer, Object ownerObj, Obj
                 int flags = initFlagsNode.execute(ndim, itemsize, shape, strides, suboffsets);
                 // TODO factory
                 ManagedBuffer managedBuffer = null;
-                if (!lib.isNull(releasefn)) {
-                    managedBuffer = new ManagedBuffer(owner, bufferStructPointer, releasefn);
+                if (!lib.isNull(bufferStructPointer)) {
+                    managedBuffer = ManagedBuffer.createForNative(bufferStructPointer);
                 }
                 IntrinsifiedPMemoryView memoryview = new IntrinsifiedPMemoryView(PythonBuiltinClassType.PMemoryView, PythonBuiltinClassType.PMemoryView.getInstanceShape(),
                                 getQueue.execute(), managedBuffer, owner, len, readonly, itemsize, format, ndim, bufPointer, 0, shape, strides, suboffsets, flags);
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/cext/capi/NativeCAPISymbols.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/cext/capi/NativeCAPISymbols.java
@@ -109,6 +109,7 @@ public abstract class NativeCAPISymbols {
     public static final String FUN_TRUFFLE_ADD_SUBOFFSET = "truffle_add_suboffset";
     public static final String FUN_PY_TRUFFLE_MEMORYVIEW_FROM_BUFFER = "PyTruffle_MemoryViewFromBuffer";
     public static final String FUN_PY_TRUFFLE_MEMORYVIEW_FROM_OBJECT = "PyTruffle_MemoryViewFromObject";
+    public static final String FUN_PY_TRUFFLE_RELEASE_BUFFER = "PyTruffle_ReleaseBuffer";
     private static final String FUN_GET_INT8_T_TYPEID = "get_int8_t_typeid";
     private static final String FUN_GET_INT16_T_TYPEID = "get_int16_t_typeid";
     private static final String FUN_GET_INT32_T_TYPEID = "get_int32_t_typeid";
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/memoryview/ManagedBuffer.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/memoryview/ManagedBuffer.java
@@ -1,5 +1,6 @@
 package com.oracle.graal.python.builtins.objects.memoryview;
 
+import java.lang.ref.WeakReference;
 import java.util.concurrent.atomic.AtomicInteger;
 
 /**
@@ -10,39 +11,40 @@
  * Rough equivalent of CPython's {@code _PyManagedBuffer_Type}
  */
 public class ManagedBuffer {
-    // Buffer owner, never null
-    final Object owner;
+    // Buffer owner, null for native objects
+    final WeakReference<Object> owner;
     // Pointer to native Py_buffer if any, null for managed objects
     final Object bufferStructPointer;
-    // Pointer to native bf_releasebuffer C function, null for managed objects
-    final Object releasefn;
 
     final AtomicInteger exports = new AtomicInteger();
 
-    public ManagedBuffer(Object owner, Object bufferStructPointer, Object releasefn) {
-        assert owner != null : "Buffers without an owner object shouldn't create a ManagedBuffer";
-        assert releasefn == null || bufferStructPointer != null;
+    private ManagedBuffer(WeakReference<Object> owner, Object bufferStructPointer) {
         this.owner = owner;
         this.bufferStructPointer = bufferStructPointer;
-        this.releasefn = releasefn;
     }
 
-    public ManagedBuffer(Object owner) {
-        this(owner, null, null);
+    public static ManagedBuffer createForManaged(Object owner) {
+        assert owner != null;
+        return new ManagedBuffer(new WeakReference<>(owner), null);
+    }
+
+    public static ManagedBuffer createForNative(Object bufferStructPointer) {
+        assert bufferStructPointer != null;
+        return new ManagedBuffer(null, bufferStructPointer);
+    }
+
+    public boolean isForNative() {
+        return bufferStructPointer != null;
     }
 
     public Object getOwner() {
-        return owner;
+        return owner.get();
     }
 
     public Object getBufferStructPointer() {
         return bufferStructPointer;
     }
 
-    public Object getReleaseFunction() {
-        return releasefn;
-    }
-
     public AtomicInteger getExports() {
         return exports;
     }
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/memoryview/MemoryViewBuiltins.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/memoryview/MemoryViewBuiltins.java
@@ -68,10 +68,6 @@
 import com.oracle.truffle.api.dsl.NodeFactory;
 import com.oracle.truffle.api.dsl.Specialization;
 import com.oracle.truffle.api.frame.VirtualFrame;
-import com.oracle.truffle.api.interop.ArityException;
-import com.oracle.truffle.api.interop.InteropLibrary;
-import com.oracle.truffle.api.interop.UnsupportedMessageException;
-import com.oracle.truffle.api.interop.UnsupportedTypeException;
 import com.oracle.truffle.api.library.CachedLibrary;
 import com.oracle.truffle.api.object.HiddenKey;
 import com.oracle.truffle.api.profiles.ConditionProfile;
@@ -99,13 +95,8 @@ public void execute(PythonContext context) {
             }
             ManagedBuffer buffer = reference.getManagedBuffer();
             // Managed buffers should be released directly in the reference queue thread
-            assert buffer.getBufferStructPointer() != null;
-            assert buffer.getReleaseFunction() != null;
-            try {
-                InteropLibrary.getUncached().execute(buffer.getReleaseFunction(), buffer.getOwner(), buffer.getBufferStructPointer());
-            } catch (UnsupportedTypeException | ArityException | UnsupportedMessageException e) {
-                throw CompilerDirectives.shouldNotReachHere("Failed to invoke bf_releasebuffer", e);
-            }
+            assert buffer.isForNative();
+            CExtNodes.PCallCapiFunction.getUncached().call(NativeCAPISymbols.FUN_PY_TRUFFLE_RELEASE_BUFFER, buffer.getBufferStructPointer());
         }
     }
 
@@ -129,10 +120,15 @@ public void postInitialize(PythonCore core) {
                 }
                 ManagedBuffer buffer = bufferReference.getManagedBuffer();
                 if (buffer.decrementExports() == 0) {
-                    if (buffer.getBufferStructPointer() != null) {
+                    if (buffer.isForNative()) {
                         return new NativeBufferReleaseCallback(bufferReference);
                     } else {
-                        MemoryViewNodes.ReleaseBufferOfManagedObjectNode.getUncached().execute(buffer.getOwner());
+                        Object owner = buffer.getOwner();
+                        // It's a weakref, it may go away and in that case we don't have to do
+                        // anything
+                        if (owner != null) {
+                            MemoryViewNodes.ReleaseBufferOfManagedObjectNode.getUncached().execute(owner);
+                        }
                         return null;
                     }
                 }
@@ -719,28 +715,26 @@ Object releaseSimple(IntrinsifiedPMemoryView self) {
             return PNone.NONE;
         }
 
-        @Specialization(guards = {"self.getReference() != null", "self.getManagedBuffer().getBufferStructPointer() == null"})
+        @Specialization(guards = {"self.getReference() != null", "!self.getManagedBuffer().isForNative()"})
         Object releaseManaged(IntrinsifiedPMemoryView self,
                         @Cached MemoryViewNodes.ReleaseBufferOfManagedObjectNode release) {
             checkExports(self);
             if (checkShouldReleaseBuffer(self)) {
-                release.execute(self.getManagedBuffer().getOwner());
+                release.execute(self.getOwner());
             }
             self.setReleased();
             return PNone.NONE;
         }
 
-        @Specialization(guards = {"self.getReference() != null", "self.getManagedBuffer().getBufferStructPointer() != null"})
+        @Specialization(guards = {"self.getReference() != null", "self.getManagedBuffer().isForNative()"})
         Object releaseNative(VirtualFrame frame, IntrinsifiedPMemoryView self,
-                        @CachedLibrary(limit = "1") InteropLibrary lib) {
+                        @Cached CExtNodes.PCallCapiFunction callRelease) {
             checkExports(self);
             if (checkShouldReleaseBuffer(self)) {
                 Object state = IndirectCallContext.enter(frame, getContext(), this);
                 ManagedBuffer buffer = self.getManagedBuffer();
                 try {
-                    lib.execute(buffer.getReleaseFunction(), buffer.getOwner(), buffer.getBufferStructPointer());
-                } catch (UnsupportedTypeException | ArityException | UnsupportedMessageException e) {
-                    throw CompilerDirectives.shouldNotReachHere("Failed to invoke bf_releasebuffer", e);
+                    callRelease.call(NativeCAPISymbols.FUN_PY_TRUFFLE_RELEASE_BUFFER, buffer.getBufferStructPointer());
                 } finally {
                     IndirectCallContext.exit(frame, getContext(), state);
                 }
diff --git a/graalpython/lib-python/3/test/test_memoryview.py b/graalpython/lib-python/3/test/test_memoryview.py
@@ -242,8 +242,11 @@ class MyObject:
             b.o = o
             wr = weakref.ref(o)
             b = m = o = None
+            # XXX GraalVM change - add more collect calls
+            for i in range(10):
+                test.support.gc_collect()
+                time.sleep(0.1)
             # The cycle must be broken
-            gc.collect()
             self.assertTrue(wr() is None, wr())
 
             # This exercises memory_clear().
@@ -253,8 +256,11 @@ class MyObject:
             m.o = o
             wr = weakref.ref(o)
             m = o = None
+            # XXX GraalVM change - add more collect calls
+            for i in range(10):
+                test.support.gc_collect()
+                time.sleep(0.1)
             # The cycle must be broken
-            gc.collect()
             self.assertTrue(wr() is None, wr())
 
     def _check_released(self, m, tp):
@@ -356,7 +362,7 @@ def callback(wr, b=b):
             self.assertIs(wr(), m)
             del m
             # XXX GraalVM change - add collect call
-            for i in range(5):
+            for i in range(10):
                 test.support.gc_collect()
                 time.sleep(0.1)
             self.assertIs(wr(), None)

Original file line number	Diff line number	Diff line change
`@@ -3377,7 +3377,7 @@ private static IntrinsifiedPMemoryView fromManaged(Object object, int itemsize,`
`3377`	`3377`	`ManagedBuffer managedBuffer = null;`
`3378`	`3378`	`if (needsRelease) {`
`3379`	`3379`	`// TODO We should lock the underlying storage for resizing`
`3380`		`- managedBuffer = new ManagedBuffer(object);`
	`3380`	`+ managedBuffer = ManagedBuffer.createForManaged(object);`
`3381`	`3381`	`}`
`3382`	`3382`	`return new IntrinsifiedPMemoryView(PythonBuiltinClassType.PMemoryView, PythonBuiltinClassType.PMemoryView.getInstanceShape(),`
`3383`	`3383`	`refQueue, managedBuffer, object, length * itemsize, readonly, itemsize, format, 1,`