Skip to content

Commit d5aa5c5

Browse files
msimacekmsimacek
committed
Don't lock managed objects when acquiring memoryview
1 parent b2d393d commit d5aa5c5

File tree

6 files changed

+19
-85
lines changed

6 files changed

+19
-85
lines changed

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/BuiltinConstructors.java

Lines changed: 5 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,6 @@
156156
import com.oracle.graal.python.builtins.objects.iterator.PZip;
157157
import com.oracle.graal.python.builtins.objects.list.PList;
158158
import com.oracle.graal.python.builtins.objects.map.PMap;
159-
import com.oracle.graal.python.builtins.objects.memoryview.ManagedBuffer;
160159
import com.oracle.graal.python.builtins.objects.memoryview.MemoryViewNodes;
161160
import com.oracle.graal.python.builtins.objects.memoryview.PBuffer;
162161
import com.oracle.graal.python.builtins.objects.memoryview.PMemoryView;
@@ -3392,34 +3391,28 @@ public final PMemoryView execute(VirtualFrame frame, Object object) {
33923391

33933392
@Specialization
33943393
PMemoryView fromBytes(@SuppressWarnings("unused") Object cls, PBytes object,
3395-
@Shared("getQueue") @Cached MemoryViewNodes.GetBufferReferences getQueue,
33963394
@Cached SequenceNodes.GetSequenceStorageNode getSequenceStorageNode,
33973395
@Cached SequenceStorageNodes.LenNode lenNode) {
33983396
SequenceStorage storage = getSequenceStorageNode.execute(object);
3399-
return fromManaged(object, 1, lenNode.execute(storage), true, "B", false, getQueue.execute());
3397+
return factory().createMemoryViewForManagedObject(object, 1, lenNode.execute(storage), true, "B");
34003398
}
34013399

34023400
@Specialization
34033401
PMemoryView fromByteArray(@SuppressWarnings("unused") Object cls, PByteArray object,
3404-
@Shared("getQueue") @Cached MemoryViewNodes.GetBufferReferences getQueue,
34053402
@Cached SequenceNodes.GetSequenceStorageNode getSequenceStorageNode,
34063403
@Cached SequenceStorageNodes.LenNode lenNode) {
3407-
object.incrementExports();
34083404
SequenceStorage storage = getSequenceStorageNode.execute(object);
3409-
return fromManaged(object, 1, lenNode.execute(storage), false, "B", true, getQueue.execute());
3405+
return factory().createMemoryViewForManagedObject(object, 1, lenNode.execute(storage), false, "B");
34103406
}
34113407

34123408
@Specialization
3413-
PMemoryView fromArray(@SuppressWarnings("unused") Object cls, PArray object,
3414-
@Shared("getQueue") @Cached MemoryViewNodes.GetBufferReferences getQueue) {
3415-
int itemsize = object.getFormat().bytesize;
3416-
object.incrementExports();
3417-
return fromManaged(object, itemsize, object.getLength(), false, object.getFormatStr(), true, getQueue.execute());
3409+
PMemoryView fromArray(@SuppressWarnings("unused") Object cls, PArray object) {
3410+
return factory().createMemoryViewForManagedObject(object, object.getFormat().bytesize, object.getLength(), false, object.getFormatStr());
34183411
}
34193412

34203413
@Specialization
34213414
PMemoryView fromMemoryView(@SuppressWarnings("unused") Object cls, PMemoryView object,
3422-
@Shared("getQueue") @Cached MemoryViewNodes.GetBufferReferences getQueue) {
3415+
@Cached MemoryViewNodes.GetBufferReferences getQueue) {
34233416
object.checkReleased(this);
34243417
return factory().createMemoryView(getQueue.execute(), object.getManagedBuffer(), object.getOwner(), object.getLength(),
34253418
object.isReadOnly(), object.getItemSize(), object.getFormat(), object.getFormatString(), object.getDimensions(),
@@ -3449,17 +3442,6 @@ PMemoryView error(@SuppressWarnings("unused") Object cls, Object object) {
34493442
throw raise(TypeError, ErrorMessages.MEMORYVIEW_A_BYTES_LIKE_OBJECT_REQUIRED_NOT_P, object);
34503443
}
34513444

3452-
private PMemoryView fromManaged(Object object, int itemsize, int length, boolean readonly, String format, boolean needsRelease,
3453-
MemoryViewNodes.BufferReferences refQueue) {
3454-
ManagedBuffer managedBuffer = null;
3455-
if (needsRelease) {
3456-
managedBuffer = ManagedBuffer.createForManaged(object);
3457-
}
3458-
return factory().createMemoryView(refQueue, managedBuffer, object, length * itemsize, readonly, itemsize, format, 1,
3459-
null, 0, new int[]{length}, new int[]{itemsize}, null,
3460-
PMemoryView.FLAG_C | PMemoryView.FLAG_FORTRAN);
3461-
}
3462-
34633445
public static MemoryViewNode create() {
34643446
return BuiltinConstructorsFactory.MemoryViewNodeFactory.create(null);
34653447
}

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/PythonCextBuiltins.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1625,7 +1625,7 @@ Object wrap(VirtualFrame frame, Object bufferStructPointer, Object ownerObj, Obj
16251625
int flags = initFlagsNode.execute(ndim, itemsize, shape, strides, suboffsets);
16261626
ManagedBuffer managedBuffer = null;
16271627
if (!lib.isNull(bufferStructPointer)) {
1628-
managedBuffer = ManagedBuffer.createForNative(bufferStructPointer);
1628+
managedBuffer = new ManagedBuffer(bufferStructPointer);
16291629
}
16301630
PMemoryView memoryview = factory().createMemoryView(getQueue.execute(), managedBuffer, owner, len, readonly, itemsize,
16311631
BufferFormat.forMemoryView(format),

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/memoryview/ManagedBuffer.java

Lines changed: 4 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -40,45 +40,24 @@
4040
*/
4141
package com.oracle.graal.python.builtins.objects.memoryview;
4242

43-
import java.lang.ref.WeakReference;
4443
import java.util.concurrent.atomic.AtomicInteger;
4544

4645
/**
4746
* Object for tracking lifetime of buffers inside memoryviews. The only purpose is to release the
4847
* underlying buffer when this object's export count goes to 0 or it gets garbage collected. Should
49-
* only be created for buffers that actually need to be released.
48+
* only be created for native buffers that actually need to be released (have a release function).
5049
*
5150
* Rough equivalent of CPython's {@code _PyManagedBuffer_Type}
5251
*/
5352
public class ManagedBuffer {
54-
// Buffer owner, null for native objects
55-
final WeakReference<Object> owner;
56-
// Pointer to native Py_buffer if any, null for managed objects
53+
// Pointer to native Py_buffer
5754
final Object bufferStructPointer;
5855

5956
final AtomicInteger exports = new AtomicInteger();
6057

61-
private ManagedBuffer(WeakReference<Object> owner, Object bufferStructPointer) {
62-
this.owner = owner;
63-
this.bufferStructPointer = bufferStructPointer;
64-
}
65-
66-
public static ManagedBuffer createForManaged(Object owner) {
67-
assert owner != null;
68-
return new ManagedBuffer(new WeakReference<>(owner), null);
69-
}
70-
71-
public static ManagedBuffer createForNative(Object bufferStructPointer) {
58+
public ManagedBuffer(Object bufferStructPointer) {
7259
assert bufferStructPointer != null;
73-
return new ManagedBuffer(null, bufferStructPointer);
74-
}
75-
76-
public boolean isForNative() {
77-
return bufferStructPointer != null;
78-
}
79-
80-
public Object getOwner() {
81-
return owner.get();
60+
this.bufferStructPointer = bufferStructPointer;
8261
}
8362

8463
public Object getBufferStructPointer() {

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/memoryview/MemoryViewBuiltins.java

Lines changed: 2 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -67,11 +67,9 @@
6767
import com.oracle.graal.python.builtins.objects.PEllipsis;
6868
import com.oracle.graal.python.builtins.objects.PNone;
6969
import com.oracle.graal.python.builtins.objects.PNotImplemented;
70-
import com.oracle.graal.python.builtins.objects.array.PArray;
7170
import com.oracle.graal.python.builtins.objects.bytes.BytesBuiltins.ExpectIntNode;
7271
import com.oracle.graal.python.builtins.objects.bytes.BytesBuiltins.SepExpectByteNode;
7372
import com.oracle.graal.python.builtins.objects.bytes.BytesNodes;
74-
import com.oracle.graal.python.builtins.objects.bytes.PByteArray;
7573
import com.oracle.graal.python.builtins.objects.bytes.PBytes;
7674
import com.oracle.graal.python.builtins.objects.cext.capi.CExtNodes;
7775
import com.oracle.graal.python.builtins.objects.cext.capi.NativeCAPISymbols;
@@ -111,7 +109,6 @@
111109
import com.oracle.truffle.api.library.CachedLibrary;
112110
import com.oracle.truffle.api.object.HiddenKey;
113111
import com.oracle.truffle.api.profiles.ConditionProfile;
114-
import com.oracle.truffle.api.profiles.ValueProfile;
115112

116113
@CoreFunctions(extendClasses = PythonBuiltinClassType.PMemoryView)
117114
public class MemoryViewBuiltins extends PythonBuiltins {
@@ -135,8 +132,6 @@ public void execute(PythonContext context) {
135132
return;
136133
}
137134
ManagedBuffer buffer = reference.getManagedBuffer();
138-
// Managed buffers should be released directly in the reference queue thread
139-
assert buffer.isForNative();
140135
boolean shouldLock = !context.getSingleThreadedAssumption().isValid();
141136
if (shouldLock) {
142137
context.acquireInteropLock();
@@ -171,17 +166,7 @@ public void postInitialize(PythonCore core) {
171166
}
172167
ManagedBuffer buffer = bufferReference.getManagedBuffer();
173168
if (buffer.decrementExports() == 0) {
174-
if (buffer.isForNative()) {
175-
return new NativeBufferReleaseCallback(bufferReference);
176-
} else {
177-
Object owner = buffer.getOwner();
178-
// It's a weakref, it may go away and in that case we don't have to do
179-
// anything
180-
if (owner != null) {
181-
releaseBufferOfManagedObject(owner);
182-
}
183-
return null;
184-
}
169+
return new NativeBufferReleaseCallback(bufferReference);
185170
}
186171
}
187172
return null;
@@ -777,18 +762,7 @@ Object releaseSimple(PMemoryView self) {
777762
return PNone.NONE;
778763
}
779764

780-
@Specialization(guards = {"self.getReference() != null", "!self.getManagedBuffer().isForNative()"})
781-
Object releaseManaged(PMemoryView self,
782-
@Cached("createClassProfile()") ValueProfile bufferClassProfile) {
783-
checkExports(self);
784-
if (checkShouldReleaseBuffer(self)) {
785-
releaseBufferOfManagedObject(bufferClassProfile.profile(self.getOwner()));
786-
}
787-
self.setReleased();
788-
return PNone.NONE;
789-
}
790-
791-
@Specialization(guards = {"self.getReference() != null", "self.getManagedBuffer().isForNative()"})
765+
@Specialization(guards = {"self.getReference() != null"})
792766
Object releaseNative(VirtualFrame frame, PMemoryView self,
793767
@Cached ExecutionContext.ForeignCallContext foreignCallContext,
794768
@Cached CExtNodes.PCallCapiFunction callRelease) {
@@ -964,12 +938,4 @@ boolean get(PMemoryView self) {
964938
return self.isCContiguous() || self.isFortranContiguous();
965939
}
966940
}
967-
968-
private static void releaseBufferOfManagedObject(Object object) {
969-
if (object instanceof PByteArray) {
970-
((PByteArray) object).decrementExports();
971-
} else if (object instanceof PArray) {
972-
((PArray) object).decrementExports();
973-
}
974-
}
975941
}

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/runtime/object/PythonObjectFactory.java

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -419,6 +419,12 @@ public PMemoryView createMemoryView(MemoryViewNodes.BufferReferences references,
419419
BufferFormat.forMemoryView(formatString), formatString, ndim, bufPointer, offset, shape, strides, suboffsets, flags));
420420
}
421421

422+
public PMemoryView createMemoryViewForManagedObject(Object object, int itemsize, int length, boolean readonly, String format) {
423+
return createMemoryView(null, null, object, length * itemsize, readonly, itemsize, format, 1,
424+
null, 0, new int[]{length}, new int[]{itemsize}, null,
425+
PMemoryView.FLAG_C | PMemoryView.FLAG_FORTRAN);
426+
}
427+
422428
public final PMethod createMethod(Object cls, Object self, Object function) {
423429
return trace(new PMethod(cls, getShape(cls), self, function));
424430
}

‎graalpython/lib-python/3/test/test_bytes.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1511,6 +1511,7 @@ def test_partition_bytearray_doesnt_share_nullstring(self):
15111511
self.assertEqual(b, b"")
15121512
self.assertEqual(c, b"")
15131513

1514+
@support.impl_detail("buffer locking", graalvm=False)
15141515
def test_resize_forbidden(self):
15151516
# #4509: can't resize a bytearray when there are buffer exports, even
15161517
# if it wouldn't reallocate the underlying buffer.

0 commit comments

Comments
 (0)