Skip to content

Commit 50792fe

Browse files
timfeltimfel
committed
Clean up code around registering replacement wrappers
All replacements must be freed somehow, and they can only be free'd when the Java side says it's ok to do so, either when the managed object dies or the context shuts down. So any replacement wrapper that is not simply free'd directly (like handles) needs to be free'd by calling its type's tp_dealloc.
1 parent a0a0db5 commit 50792fe

File tree

8 files changed

+73
-52
lines changed

8 files changed

+73
-52
lines changed

‎graalpython/com.oracle.graal.python.cext/src/capi.c‎

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -408,10 +408,25 @@ PyAPI_FUNC(Py_ssize_t)
408408
GraalPyPrivate_BulkDealloc(intptr_t ptrArray[], int64_t len)
409409
{
410410
for (int i = 0; i < len; i++) {
411-
PyObject *obj = (PyObject*)ptrArray[i];
412-
GraalPyPrivate_Log(PY_TRUFFLE_LOG_FINER, "%s: _Py_Dealloc(0x%zx)",
413-
__func__, obj);
414-
_Py_Dealloc(obj);
411+
PyObject *obj = (PyObject *)ptrArray[i];
412+
if (!obj->ob_type) {
413+
GraalPyPrivate_Log(PY_TRUFFLE_LOG_FINER,
414+
"%s: Object: 0x%zx has no type anymore, we have "
415+
"no idea what it is, we will just free it",
416+
__func__, obj);
417+
free(obj);
418+
} else if (obj->ob_refcnt == _Py_IMMORTAL_REFCNT) {
419+
GraalPyPrivate_Log(PY_TRUFFLE_LOG_FINER,
420+
"%s: Not deallocating immortal object: 0x%zx (%s%s)",
421+
__func__, obj,
422+
PyType_Check(obj) ? "class " : "a ",
423+
PyType_Check(obj) ? ((PyTypeObject*)obj)->tp_name : Py_TYPE(obj)->tp_name);
424+
} else {
425+
GraalPyPrivate_Log(PY_TRUFFLE_LOG_FINER,
426+
"%s: _Py_Dealloc(0x%zx)",
427+
__func__, obj);
428+
_Py_Dealloc(obj);
429+
}
415430
}
416431
return 0;
417432
}
@@ -621,10 +636,6 @@ PyAPI_FUNC(void*) GraalPyPrivate_VaArgPointer(va_list* va) {
621636
return va_arg(*va, void*);
622637
}
623638

624-
PyAPI_FUNC(void*) GraalPyPrivate_ConvertPointer(Py_ssize_t value) {
625-
return (void*) value;
626-
}
627-
628639
PyAPI_FUNC(int) GraalPyPrivate_NoOpClear(PyObject* o) {
629640
return 0;
630641
}

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/cext/capi/CApiContext.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1120,8 +1120,8 @@ public void finalizeCApi() {
11201120
* and the singleton wrappers were cleared because they might also end up in the
11211121
* lookup table and may otherwise be double-freed.
11221122
*/
1123+
CApiTransitions.freeNativeReplacementStructs(context, handleContext);
11231124
CApiTransitions.freeNativeObjectStubs(handleContext);
1124-
CApiTransitions.freeNativeReplacementStructs(handleContext);
11251125
CApiTransitions.freeNativeStorages(handleContext);
11261126
}
11271127
if (pyDateTimeCAPICapsule != null) {

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/cext/capi/CExtNodes.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -328,7 +328,7 @@ static PDict allocateNativePart(Object cls, PDict managedSide,
328328
managedSide.setNativeWrapper(nativeWrapper);
329329
long nativeObject = (long) call.call(NativeCAPISymbol.FUN_PY_TYPE_GENERIC_NEW_RAW, toNative.execute(cls), 0L, 0L);
330330
CApiTransitions.writeNativeRefCount(nativeObject, MANAGED_REFCNT);
331-
CApiTransitions.createReference(nativeWrapper, nativeObject, true);
331+
CApiTransitions.createReference(nativeWrapper, nativeObject, false);
332332
return managedSide;
333333
}
334334
}

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/cext/capi/NativeCAPISymbol.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,6 @@
6262
public enum NativeCAPISymbol implements NativeCExtSymbol {
6363

6464
FUN_VA_ARG_POINTER("GraalPyPrivate_VaArgPointer", Pointer, Pointer),
65-
FUN_CONVERT_POINTER("GraalPyPrivate_ConvertPointer", Pointer, Py_ssize_t),
6665
FUN_NO_OP_CLEAR("GraalPyPrivate_NoOpClear", Int, PyObject),
6766
FUN_NO_OP_TRAVERSE("GraalPyPrivate_NoOpTraverse", Int, PyObject, Pointer, Pointer),
6867

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/cext/capi/PThreadState.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,10 @@ public final class PThreadState extends PythonStructNativeWrapper {
7676
@TruffleBoundary
7777
private PThreadState(PythonThreadState threadState) {
7878
super(threadState, true);
79-
// 'registerReplacement' will set the native pointer if not running LLVM managed mode.
79+
// TODO: this passes false for allocatedFromJava although we actually do allocate it from
80+
// Java, but we currently free it on context exit using a call to
81+
// FUN_PY_GC_COLLECT_NO_FAIL, which can bite us when we change allocators for the Java
82+
// side...
8083
replacement = registerReplacement(allocateCLayout(threadState), false, InteropLibrary.getUncached());
8184
}
8285

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/cext/capi/PyMemoryViewWrapper.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,9 @@ private static Object allocate(PMemoryView object) {
151151
public Object getReplacement(InteropLibrary lib) {
152152
if (replacement == null) {
153153
Object pointerObject = allocate((PMemoryView) getDelegate());
154+
// TODO: this passes "false" for allocatedFromJava, although it actually is. The
155+
// problem, however, is that this struct contains nested allocations from Java. This
156+
// needs to be cleaned up...
154157
replacement = registerReplacement(pointerObject, false, lib);
155158
}
156159
return replacement;

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/cext/capi/PythonNativeWrapper.java‎

Lines changed: 9 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -40,10 +40,10 @@
4040
*/
4141
package com.oracle.graal.python.builtins.objects.cext.capi;
4242

43-
import com.oracle.graal.python.builtins.objects.cext.capi.CExtNodes.PCallCapiFunction;
4443
import com.oracle.graal.python.builtins.objects.cext.capi.transitions.CApiTransitions;
4544
import com.oracle.graal.python.builtins.objects.cext.capi.transitions.CApiTransitions.HandlePointerConverter;
4645
import com.oracle.graal.python.builtins.objects.cext.capi.transitions.CApiTransitions.PythonObjectReference;
46+
import com.oracle.graal.python.builtins.objects.cext.common.NativePointer;
4747
import com.oracle.graal.python.util.PythonUtils;
4848
import com.oracle.truffle.api.CompilerDirectives;
4949
import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
@@ -136,30 +136,19 @@ public final void setReplacement(Object replacement) {
136136
}
137137

138138
@TruffleBoundary
139-
public final Object registerReplacement(Object pointer, boolean freeAtCollection, InteropLibrary lib) {
139+
public final Object registerReplacement(Object pointer, boolean wasAllocatedFromJava, InteropLibrary lib) {
140140
LOGGER.finest(() -> PythonUtils.formatJString("assigning %s with %s", getDelegate(), pointer));
141141
Object result;
142142
if (pointer instanceof Long lptr) {
143-
// need to convert to actual pointer
144-
result = PCallCapiFunction.callUncached(NativeCAPISymbol.FUN_CONVERT_POINTER, lptr);
145-
CApiTransitions.createReference(this, lptr, freeAtCollection);
143+
// need to convert to interop pointer for NFI
144+
result = new NativePointer(lptr);
145+
CApiTransitions.createReference(this, lptr, wasAllocatedFromJava);
146146
} else {
147147
result = pointer;
148-
if (lib.isPointer(pointer)) {
149-
assert CApiContext.isPointerObject(pointer);
150-
try {
151-
CApiTransitions.createReference(this, lib.asPointer(pointer), freeAtCollection);
152-
} catch (UnsupportedMessageException e) {
153-
throw CompilerDirectives.shouldNotReachHere(e);
154-
}
155-
} else {
156-
/*
157-
* This branch is required for LLVM managed mode where we will never have a native
158-
* pointer (i.e. an address pointing into off-heap memory) but a managed pointer to
159-
* some object emulating the native memory.
160-
*/
161-
assert pointer.getClass().getSimpleName().contains("LLVMPointer");
162-
CApiTransitions.createManagedReference(getDelegate(), pointer);
148+
try {
149+
CApiTransitions.createReference(this, lib.asPointer(pointer), wasAllocatedFromJava);
150+
} catch (UnsupportedMessageException e) {
151+
throw CompilerDirectives.shouldNotReachHere(e);
163152
}
164153
}
165154
return result;

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/cext/capi/transitions/CApiTransitions.java‎

Lines changed: 36 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@
7070
import com.oracle.graal.python.builtins.objects.cext.capi.CExtNodes.PCallCapiFunction;
7171
import com.oracle.graal.python.builtins.objects.cext.capi.NativeCAPISymbol;
7272
import com.oracle.graal.python.builtins.objects.cext.capi.PrimitiveNativeWrapper;
73+
import com.oracle.graal.python.builtins.objects.cext.capi.PythonClassNativeWrapper;
7374
import com.oracle.graal.python.builtins.objects.cext.capi.PythonNativeWrapper;
7475
import com.oracle.graal.python.builtins.objects.cext.capi.PythonNativeWrapper.PythonAbstractObjectNativeWrapper;
7576
import com.oracle.graal.python.builtins.objects.cext.capi.TruffleObjectNativeWrapper;
@@ -233,23 +234,32 @@ public static final class PythonObjectReference extends IdReference<PythonNative
233234
private final int handleTableIndex;
234235

235236
/**
236-
* Indicates if the native memory {@link #pointer} should be freed (using {@link FreeNode})
237-
* if this reference was enqueued because the referent was collected. For example, this will
238-
* be {@code true} if the referent is
239-
* {@link com.oracle.graal.python.builtins.objects.cext.capi.PythonClassNativeWrapper} and
240-
* the class native replacement was allocated on the heap (usually a heap type). It will be
241-
* {@code false} if the class wraps a static type.
237+
* Indicates if the native memory {@link #pointer} was allocated from Java using
238+
* {@link AllocateNode} and thus must be freed using {@link FreeNode}. For any
239+
* {@link PythonObjectReference} the last collection is always on the Java side, since the
240+
* native side can only drop the ob_refcnt to
241+
* {@link PythonAbstractObjectNativeWrapper#MANAGED_REFCNT} at which point the
242+
* {@link #strongReference} is removed and the Java GC will eventually get to collect the
243+
* object and enqueue this reference. At that point, we need to make a decision how to free
244+
* the native memory. For example, for managed classes we allocated a complete type struct
245+
* from Java ({@link PythonClassNativeWrapper#getReplacement}), or for any managed object
246+
* that is represented with a stub we also allocate that stub from Java
247+
* ({@link AllocateNativeObjectStubNode}). These subsequently must be freed from Java as
248+
* well. If this field is false, however, it means the memory was allocated through C in in
249+
* some way (presumably {@code PyObject_Malloc}, via the type's {@code tp_alloc}). So when
250+
* the reference is collected we dealloc using {@code GraalPyPrivate_BulkDealloc} which
251+
* calls the right API.
242252
*/
243-
private final boolean freeAtCollection;
253+
private final boolean allocatedFromJava;
244254
private final boolean gc;
245255

246-
private PythonObjectReference(HandleContext handleContext, PythonNativeWrapper referent, boolean strong, long pointer, int handleTableIndex, boolean freeAtCollection, boolean gc) {
256+
private PythonObjectReference(HandleContext handleContext, PythonNativeWrapper referent, boolean strong, long pointer, int handleTableIndex, boolean allocatedFromJava, boolean gc) {
247257
super(handleContext, referent);
248258
this.pointer = pointer;
249259
this.strongReference = strong ? referent : null;
250260
referent.ref = this;
251261
this.handleTableIndex = handleTableIndex;
252-
this.freeAtCollection = freeAtCollection;
262+
this.allocatedFromJava = allocatedFromJava;
253263
this.gc = gc;
254264
if (LOGGER.isLoggable(Level.FINE)) {
255265
LOGGER.fine(PythonUtils.formatJString("new %s", toString()));
@@ -261,8 +271,8 @@ static PythonObjectReference create(HandleContext handleContext, PythonAbstractO
261271
return new PythonObjectReference(handleContext, referent, strong, pointer, idx, true, gc);
262272
}
263273

264-
static PythonObjectReference create(HandleContext handleContext, PythonNativeWrapper referent, long pointer, boolean freeAtCollection) {
265-
return new PythonObjectReference(handleContext, referent, true, pointer, -1, freeAtCollection, false);
274+
static PythonObjectReference create(HandleContext handleContext, PythonNativeWrapper referent, long pointer, boolean allocatedFromJava) {
275+
return new PythonObjectReference(handleContext, referent, true, pointer, -1, allocatedFromJava, false);
266276
}
267277

268278
public boolean isStrongReference() {
@@ -277,8 +287,8 @@ public int getHandleTableIndex() {
277287
return handleTableIndex;
278288
}
279289

280-
public boolean isFreeAtCollection() {
281-
return freeAtCollection;
290+
public boolean isAllocatedFromJava() {
291+
return allocatedFromJava;
282292
}
283293

284294
@Override
@@ -482,9 +492,11 @@ public static int pollReferenceQueue() {
482492
assert nativeLookupGet(handleContext, reference.pointer) != null : Long.toHexString(reference.pointer);
483493
LOGGER.finer(() -> PythonUtils.formatJString("releasing native stub lookup for managed object with replacement %x => %s", reference.pointer, reference));
484494
nativeLookupRemove(handleContext, reference.pointer);
485-
if (reference.isFreeAtCollection()) {
495+
if (reference.isAllocatedFromJava()) {
486496
LOGGER.finer(() -> PythonUtils.formatJString("freeing managed object %s replacement", reference));
487497
freeNativeStruct(reference);
498+
} else {
499+
referencesToBeFreed.add(reference.pointer);
488500
}
489501
}
490502
} else if (entry instanceof NativeObjectReference reference) {
@@ -626,8 +638,9 @@ private static boolean tableEntryRemoved(HandleContext context, PythonNativeWrap
626638
return true;
627639
}
628640

629-
public static void freeNativeReplacementStructs(HandleContext handleContext) {
630-
assert PythonContext.get(null).ownsGil();
641+
public static void freeNativeReplacementStructs(PythonContext context, HandleContext handleContext) {
642+
assert context.ownsGil();
643+
ArrayList<Long> referencesToBeFreed = new ArrayList<>();
631644
handleContext.nativeLookup.forEach((l, ref) -> {
632645
if (ref instanceof PythonObjectReference reference) {
633646
// We don't expect references to wrappers that would have a native object stub.
@@ -637,11 +650,14 @@ public static void freeNativeReplacementStructs(HandleContext handleContext) {
637650
* native memory and some of them were allocated in heap, and (b) struct wrappers,
638651
* which may be freed manually in a separate step.
639652
*/
640-
if (reference.isFreeAtCollection()) {
653+
if (reference.isAllocatedFromJava()) {
641654
freeNativeStruct(reference);
655+
} else {
656+
referencesToBeFreed.add(reference.pointer);
642657
}
643658
}
644659
});
660+
releaseNativeObjects(context, referencesToBeFreed);
645661
handleContext.nativeLookup.clear();
646662
}
647663

@@ -672,7 +688,7 @@ private static void freeNativeStub(PythonObjectReference ref) {
672688

673689
private static void freeNativeStruct(PythonObjectReference ref) {
674690
assert ref.handleTableIndex == -1;
675-
assert ref.isFreeAtCollection();
691+
assert ref.isAllocatedFromJava();
676692
assert !ref.gc;
677693
LOGGER.fine(() -> PythonUtils.formatJString("releasing %s", ref.toString()));
678694
FreeNode.executeUncached(ref.pointer);
@@ -1161,7 +1177,7 @@ static long doGeneric(Node inliningTarget, PythonAbstractObjectNativeWrapper wra
11611177
*/
11621178
@TruffleBoundary
11631179
@SuppressWarnings("try")
1164-
public static void createReference(PythonNativeWrapper obj, long ptr, boolean freeAtCollection) {
1180+
public static void createReference(PythonNativeWrapper obj, long ptr, boolean allocatedFromJava) {
11651181
try (GilNode.UncachedAcquire ignored = GilNode.uncachedAcquire()) {
11661182
/*
11671183
* The first test if '!obj.isNative()' in the caller is done on a fast-path but not
@@ -1172,7 +1188,7 @@ public static void createReference(PythonNativeWrapper obj, long ptr, boolean fr
11721188
obj.setNativePointer(ptr);
11731189
pollReferenceQueue();
11741190
HandleContext context = getContext();
1175-
nativeLookupPut(context, ptr, PythonObjectReference.create(context, obj, ptr, freeAtCollection));
1191+
nativeLookupPut(context, ptr, PythonObjectReference.create(context, obj, ptr, allocatedFromJava));
11761192
}
11771193
}
11781194
}

0 commit comments

Comments
 (0)