From b973926352ba91b6bbb23aad5a7d1ad281088ca8 Mon Sep 17 00:00:00 2001 From: Zbynek Slajchrt Date: Wed, 10 Oct 2018 22:39:56 +0200 Subject: [PATCH] Fixed: Objects returned from upcalls are gc'd indeterministically --- .../r/ffi/impl/upcalls/DLLUpCallsRFFI.java | 3 +- .../r/ffi/impl/upcalls/IDEUpCallsRFFI.java | 3 +- .../r/ffi/impl/upcalls/StdUpCallsRFFI.java | 31 ++++++----- .../truffle/r/ffi/processor/FFIProcessor.java | 52 +++++++++++++++---- .../r/ffi/processor/RFFIResultOwner.java | 41 +++++++++++++++ .../r/runtime/data/NativeDataAccess.java | 23 +++++--- .../truffle/r/runtime/ffi/RFFIContext.java | 27 ++++++++++ 7 files changed, 149 insertions(+), 31 deletions(-) create mode 100644 com.oracle.truffle.r.ffi.processor/src/com/oracle/truffle/r/ffi/processor/RFFIResultOwner.java diff --git a/com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/upcalls/DLLUpCallsRFFI.java b/com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/upcalls/DLLUpCallsRFFI.java index 0741898b74..e1134d74a0 100644 --- a/com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/upcalls/DLLUpCallsRFFI.java +++ b/com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/upcalls/DLLUpCallsRFFI.java @@ -25,6 +25,7 @@ import com.oracle.truffle.api.interop.TruffleObject; import com.oracle.truffle.r.ffi.processor.RFFICpointer; import com.oracle.truffle.r.ffi.processor.RFFICstring; +import com.oracle.truffle.r.ffi.processor.RFFIResultOwner; public interface DLLUpCallsRFFI { @@ -48,7 +49,7 @@ public interface DLLUpCallsRFFI { * @param fun a representation of the the C address of the function (in the table) * @param numArgs the number of arguments the function takes. */ - Object setDotSymbolValues(Object dllInfo, @RFFICstring String name, @RFFICpointer Object fun, int numArgs); + Object setDotSymbolValues(Object dllInfo, @RFFICstring String name, @RFFIResultOwner @RFFICpointer Object fun, int numArgs); /** * Directly implements {@code R_useDynamicSymbols}. diff --git a/com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/upcalls/IDEUpCallsRFFI.java b/com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/upcalls/IDEUpCallsRFFI.java index 927b38a16a..9eb85a8afe 100644 --- a/com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/upcalls/IDEUpCallsRFFI.java +++ b/com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/upcalls/IDEUpCallsRFFI.java @@ -23,6 +23,7 @@ package com.oracle.truffle.r.ffi.impl.upcalls; import com.oracle.truffle.api.frame.Frame; +import com.oracle.truffle.r.ffi.processor.RFFIResultOwner; /** * Additional upcalls created for supporting FastR in RStudio. These mainly relate to the GNU R @@ -42,7 +43,7 @@ public interface IDEUpCallsRFFI { Object R_getContextCall(Object c); - Object R_getContextSrcRef(Object c); + Object R_getContextSrcRef(@RFFIResultOwner Object c); int R_insideBrowser(); diff --git a/com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/upcalls/StdUpCallsRFFI.java b/com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/upcalls/StdUpCallsRFFI.java index ad868f627b..c2b655ad46 100644 --- a/com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/upcalls/StdUpCallsRFFI.java +++ b/com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/upcalls/StdUpCallsRFFI.java @@ -73,6 +73,8 @@ import com.oracle.truffle.r.ffi.impl.nodes.TryRfEvalNode; import com.oracle.truffle.r.ffi.processor.RFFICpointer; import com.oracle.truffle.r.ffi.processor.RFFICstring; +import com.oracle.truffle.r.ffi.processor.RFFIResultOwner; +import com.oracle.truffle.r.ffi.processor.RFFIRunGC; import com.oracle.truffle.r.ffi.processor.RFFIUpCallNode; import com.oracle.truffle.r.runtime.nmath.distr.Cauchy; import com.oracle.truffle.r.runtime.nmath.distr.Chisq; @@ -189,7 +191,7 @@ public interface StdUpCallsRFFI { void Rf_defineVar(Object symbolArg, Object value, Object envArg); @RFFIUpCallNode(GetClassDefNode.class) - Object R_getClassDef(@RFFICstring(convert = false) Object clazz); + Object R_getClassDef(@RFFIResultOwner @RFFICstring(convert = false) Object clazz); @RFFIUpCallNode(DoMakeClassNode.class) Object R_do_MAKE_CLASS(@RFFICstring(convert = false) Object clazz); @@ -202,15 +204,15 @@ public interface StdUpCallsRFFI { */ Object Rf_findVar(Object symbolArg, Object envArg); - Object Rf_findVarInFrame(Object envArg, Object symbolArg); + Object Rf_findVarInFrame(@RFFIResultOwner Object envArg, Object symbolArg); - Object Rf_findVarInFrame3(Object envArg, Object symbolArg, int doGet); + Object Rf_findVarInFrame3(@RFFIResultOwner Object envArg, Object symbolArg, int doGet); @RFFIUpCallNode(ATTRIB.class) - Object ATTRIB(Object obj); + Object ATTRIB(@RFFIResultOwner Object obj); @RFFIUpCallNode(GetAttrib.class) - Object Rf_getAttrib(Object obj, Object name); + Object Rf_getAttrib(@RFFIResultOwner Object obj, Object name); void Rf_setAttrib(Object obj, Object name, Object val); @@ -236,14 +238,19 @@ public interface StdUpCallsRFFI { void Rf_errorcall(Object call, @RFFICstring String msg); + @RFFIRunGC Object Rf_allocVector(int mode, long n); + @RFFIRunGC Object Rf_allocArray(int mode, Object dimsObj); + @RFFIRunGC Object Rf_allocMatrix(int mode, int nrow, int ncol); + @RFFIRunGC Object Rf_allocList(int length); + @RFFIRunGC Object Rf_allocSExp(int type); int Rf_nrows(Object x); @@ -286,9 +293,9 @@ public interface StdUpCallsRFFI { @RFFICpointer Object COMPLEX(Object x); - Object STRING_ELT(Object x, long i); + Object STRING_ELT(@RFFIResultOwner Object x, long i); - Object VECTOR_ELT(Object x, long i); + Object VECTOR_ELT(@RFFIResultOwner Object x, long i); int NAMED(Object x); @@ -355,10 +362,10 @@ public interface StdUpCallsRFFI { Object SETCDR(Object x, Object y); @RFFIUpCallNode(MiscNodes.GetFunctionFormals.class) - Object FORMALS(Object x); + Object FORMALS(@RFFIResultOwner Object x); @RFFIUpCallNode(MiscNodes.GetFunctionBody.class) - Object BODY(Object x); + Object BODY(@RFFIResultOwner Object x); @RFFIUpCallNode(MiscNodes.GetFunctionEnvironment.class) Object CLOENV(Object x); @@ -477,11 +484,11 @@ public interface StdUpCallsRFFI { int PRSEEN(Object x); - Object PRENV(Object x); + Object PRENV(@RFFIResultOwner Object x); - Object R_PromiseExpr(Object x); + Object R_PromiseExpr(@RFFIResultOwner Object x); - Object PRCODE(Object x); + Object PRCODE(@RFFIResultOwner Object x); @RFFICpointer Object R_CHAR(Object x); diff --git a/com.oracle.truffle.r.ffi.processor/src/com/oracle/truffle/r/ffi/processor/FFIProcessor.java b/com.oracle.truffle.r.ffi.processor/src/com/oracle/truffle/r/ffi/processor/FFIProcessor.java index e946432cd5..f4bee01287 100644 --- a/com.oracle.truffle.r.ffi.processor/src/com/oracle/truffle/r/ffi/processor/FFIProcessor.java +++ b/com.oracle.truffle.r.ffi.processor/src/com/oracle/truffle/r/ffi/processor/FFIProcessor.java @@ -170,6 +170,8 @@ private void generateCallClass(ExecutableElement m) throws IOException { List params = m.getParameters(); StringBuilder arguments = new StringBuilder(); StringBuilder unwrapNodes = new StringBuilder(); + StringBuilder unwrappedArgs = new StringBuilder(); + CharSequence resultOwnerRHS = null; boolean needsUnwrapImport = false; for (int i = 0; i < params.size(); i++) { if (i != 0) { @@ -179,6 +181,7 @@ private void generateCallClass(ExecutableElement m) throws IOException { RFFICpointer[] pointerAnnotations = params.get(i).getAnnotationsByType(RFFICpointer.class); RFFICstring[] stringAnnotations = params.get(i).getAnnotationsByType(RFFICstring.class); + RFFIResultOwner[] resultOwnerAnnotations = params.get(i).getAnnotationsByType(RFFIResultOwner.class); String paramName = params.get(i).getSimpleName().toString(); String paramTypeName = getTypeName(paramType); @@ -191,13 +194,20 @@ private void generateCallClass(ExecutableElement m) throws IOException { arguments.append('(').append(paramTypeName).append(") "); } needsUnwrapImport |= needsUnwrap; + unwrappedArgs.append(" final Object ").append(paramName).append("Unwrapped = "); if (needsUnwrap) { - arguments.append(paramName).append("Unwrap").append(".execute("); unwrapNodes.append(" @Child private FFIUnwrapNode ").append(paramName).append("Unwrap").append(" = FFIUnwrapNode.create();\n"); + unwrappedArgs.append(paramName).append("Unwrap").append(".execute("); } - arguments.append("arguments.get(").append(i).append(")"); + unwrappedArgs.append("arguments.get(").append(i).append(")"); + arguments.append(paramName).append("Unwrapped"); if (needsUnwrap) { - arguments.append(')'); + unwrappedArgs.append(')'); + } + unwrappedArgs.append(";\n"); + + if (resultOwnerAnnotations.length > 0) { + resultOwnerRHS = new StringBuilder(paramName).append("Unwrapped"); } } @@ -319,17 +329,20 @@ private void generateCallClass(ExecutableElement m) throws IOException { w.append(" }\n"); w.append(" RFFIContext ctx = RContext.getInstance().getStateRFFI();\n"); if (returnKind != TypeKind.VOID) { + w.append(" Object resultRObj0;\n"); w.append(" Object resultRObj;\n"); } w.append(" ctx.beforeUpcall(" + canRunGc + ");\n"); + w.append(unwrappedArgs); + if (resultOwnerRHS != null) { + StringBuilder resultOwner = new StringBuilder(" Object resultOwner = ").append(resultOwnerRHS).append(";\n"); + w.append(resultOwner); + } w.append(" try {\n"); if (returnKind == TypeKind.VOID) { w.append(" "); } else { - w.append(" resultRObj = "); - } - if (needsReturnWrap) { - w.append("returnWrap.execute("); + w.append(" resultRObj0 = "); } if (nodeClass != null) { w.append("node.executeObject"); @@ -340,11 +353,28 @@ private void generateCallClass(ExecutableElement m) throws IOException { if (useFrame) { w.append("frame, "); } - w.append(arguments).append(")"); - if (needsReturnWrap) { - w.append(");\n"); + w.append(arguments).append(");\n"); + + if (returnKind != TypeKind.VOID) { + w.append(" resultRObj = "); + if (needsReturnWrap) { + w.append("returnWrap.execute("); + } + w.append("resultRObj0"); + if (needsReturnWrap) { + w.append(");\n"); + } else { + w.append(";\n"); + } + } + if (resultOwnerRHS != null) { + w.append(" ctx.protectChild(resultOwner, resultRObj);\n"); } else { - w.append(";\n"); + if (returnKind != TypeKind.VOID) { + w.append(" if (resultRObj0 != resultRObj) {\n"); + w.append(" ctx.protectChild(resultRObj0, resultRObj);\n"); + w.append(" }\n"); + } } w.append(" } catch (Throwable ex) {\n"); w.append(" CompilerDirectives.transferToInterpreter();\n"); diff --git a/com.oracle.truffle.r.ffi.processor/src/com/oracle/truffle/r/ffi/processor/RFFIResultOwner.java b/com.oracle.truffle.r.ffi.processor/src/com/oracle/truffle/r/ffi/processor/RFFIResultOwner.java new file mode 100644 index 0000000000..515ec76af2 --- /dev/null +++ b/com.oracle.truffle.r.ffi.processor/src/com/oracle/truffle/r/ffi/processor/RFFIResultOwner.java @@ -0,0 +1,41 @@ +/* + * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 3 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 3 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 3 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ +package com.oracle.truffle.r.ffi.processor; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Target; + +/** + * Tags an upcall argument as the owner of the upcall result. Tagging is necessary always when the + * result logically belongs to the owning object (i.e. the lifetime of the result is the same as the + * lifetime of the owning object), but the owner does not refer to the result object (perhaps + * because the result is a secondary value created from another value retrieved from the owning + * object in the upcall). Not tagging the owner argument can lead to an accidental garbage + * collecting of the result even when its owner is still alive. + * + * An example of such a situation is a retrieval of a vector attribute, which is wrapped or cast + * upon the return from the upcall. + */ +@Target(ElementType.PARAMETER) +public @interface RFFIResultOwner { +} diff --git a/com.oracle.truffle.r.runtime/src/com/oracle/truffle/r/runtime/data/NativeDataAccess.java b/com.oracle.truffle.r.runtime/src/com/oracle/truffle/r/runtime/data/NativeDataAccess.java index 7816bc484c..e4ce6d5e64 100644 --- a/com.oracle.truffle.r.runtime/src/com/oracle/truffle/r/runtime/data/NativeDataAccess.java +++ b/com.oracle.truffle.r.runtime/src/com/oracle/truffle/r/runtime/data/NativeDataAccess.java @@ -104,15 +104,26 @@ public interface Releasable { private static final boolean TRACE_MIRROR_ALLOCATION_SITES = false; - private static final long EMPTY_DATA_ADDRESS; + private static final AtomicLong emptyDataAddress; static { - EMPTY_DATA_ADDRESS = allocateNativeMemory(8); + emptyDataAddress = new AtomicLong(0); } private static final ReferenceQueue nativeRefQueue = new ReferenceQueue<>(); private static final AtomicReference nativeRefQueueThread = new AtomicReference<>(null); + private static long getEmptyDataAddress() { + long addr = emptyDataAddress.get(); + if (addr == 0L) { + addr = allocateNativeMemory(8); + if (!emptyDataAddress.compareAndSet(0L, addr)) { + freeNativeMemory(addr); + } + } + return emptyDataAddress.get(); + } + private static void initNativeRefQueueThread() { Thread thread = nativeRefQueueThread.get(); if (thread == null) { @@ -211,12 +222,12 @@ void allocateNative(Object source, int len, int trueLen, int elementBase, int el dataAddress = allocateNativeMemory(trueLen * elementSize); UnsafeAdapter.UNSAFE.copyMemory(source, elementBase, null, dataAddress, trueLen * elementSize); } else { - dataAddress = EMPTY_DATA_ADDRESS; + dataAddress = getEmptyDataAddress(); } this.length = len; // ensure that marker address is not used - assert this.length == 0 || dataAddress != EMPTY_DATA_ADDRESS; + assert this.length == 0 || dataAddress != getEmptyDataAddress(); } @TruffleBoundary @@ -229,7 +240,7 @@ void allocateNativeString(byte[] bytes) { this.length = bytes.length + 1; // ensure that marker address is not used - assert this.length == 0 || dataAddress != EMPTY_DATA_ADDRESS; + assert this.length == 0 || dataAddress != getEmptyDataAddress(); } @TruffleBoundary @@ -258,7 +269,7 @@ public void release() { nativeMirrors.remove(id, this); } // System.out.println(String.format("gc'ing %16x", id)); - if (dataAddress == EMPTY_DATA_ADDRESS) { + if (dataAddress == getEmptyDataAddress()) { assert (dataAddress = 0xbadbad) != 0; } else if (dataAddress != 0) { // System.out.println(String.format("freeing data at %16x", dataAddress)); diff --git a/com.oracle.truffle.r.runtime/src/com/oracle/truffle/r/runtime/ffi/RFFIContext.java b/com.oracle.truffle.r.runtime/src/com/oracle/truffle/r/runtime/ffi/RFFIContext.java index 8fbc98391e..26e6f765b1 100644 --- a/com.oracle.truffle.r.runtime/src/com/oracle/truffle/r/runtime/ffi/RFFIContext.java +++ b/com.oracle.truffle.r.runtime/src/com/oracle/truffle/r/runtime/ffi/RFFIContext.java @@ -23,7 +23,10 @@ package com.oracle.truffle.r.runtime.ffi; import java.util.ArrayList; +import java.util.HashSet; import java.util.IdentityHashMap; +import java.util.Set; +import java.util.WeakHashMap; import java.util.concurrent.atomic.AtomicInteger; import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary; @@ -80,6 +83,8 @@ public final void registerReferenceUsedInNative(Object obj) { */ public final IdentityHashMap preserveList = new IdentityHashMap<>(); + private final WeakHashMap> protectedChildren = new WeakHashMap<>(); + public abstract TruffleObject lookupNativeFunction(NativeFunction function); /** @@ -144,6 +149,28 @@ private void cooperativeGc() { protectedNativeReferences.clear(); } + /** + * Establish a weak relationship between an object and its owner to prevent a premature garbage + * collecting of the object. See com.oracle.truffle.r.ffi.processor.RFFIResultOwner + * for more commentary. + * + * Note: It is meant to be applied only on certain return values from upcalls. + * + * @param parent + * @param child + * @return the child + * + */ + public final Object protectChild(Object parent, Object child) { + Set children = protectedChildren.get(parent); + if (children == null) { + children = new HashSet<>(); + protectedChildren.put(parent, children); + } + children.add(child); + return child; + } + private RFFI instance; public RFFI getRFFI() {