Skip to content

Commit

Permalink
Fixed: Objects returned from upcalls are gc'd indeterministically
Browse files Browse the repository at this point in the history
  • Loading branch information
@zslajchrt
zslajchrt committed Oct 11, 2018
1 parent 343fefa commit b973926
Show file tree
Hide file tree
Showing 7 changed files with 149 additions and 31 deletions.
3 changes: 2 additions & 1 deletion com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/upcalls/DLLUpCallsRFFI.java
View file
Expand Up @@ -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 {

Expand All @@ -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}.
Expand Down
3 changes: 2 additions & 1 deletion com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/upcalls/IDEUpCallsRFFI.java
View file
Expand Up @@ -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
Expand All @@ -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();

Expand Down
31 changes: 19 additions & 12 deletions com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/upcalls/StdUpCallsRFFI.java
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -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);

Expand All @@ -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);
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
52 changes: 41 additions & 11 deletions com.oracle.truffle.r.ffi.processor/src/com/oracle/truffle/r/ffi/processor/FFIProcessor.java
View file
Expand Up @@ -170,6 +170,8 @@ private void generateCallClass(ExecutableElement m) throws IOException {
List<? extends VariableElement> 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) {
Expand All @@ -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);
Expand All @@ -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");
}
}

Expand Down Expand Up @@ -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");
Expand All @@ -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");
Expand Down
41 changes: 41 additions & 0 deletions ...racle.truffle.r.ffi.processor/src/com/oracle/truffle/r/ffi/processor/RFFIResultOwner.java
View file
@@ -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 {
}
23 changes: 17 additions & 6 deletions com.oracle.truffle.r.runtime/src/com/oracle/truffle/r/runtime/data/NativeDataAccess.java
View file
Expand Up @@ -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<Object> nativeRefQueue = new ReferenceQueue<>();

private static final AtomicReference<Thread> 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) {
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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));
Expand Down
27 changes: 27 additions & 0 deletions com.oracle.truffle.r.runtime/src/com/oracle/truffle/r/runtime/ffi/RFFIContext.java
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -80,6 +83,8 @@ public final void registerReferenceUsedInNative(Object obj) {
*/
public final IdentityHashMap<RObject, AtomicInteger> preserveList = new IdentityHashMap<>();

private final WeakHashMap<Object, Set<Object>> protectedChildren = new WeakHashMap<>();

public abstract TruffleObject lookupNativeFunction(NativeFunction function);

/**
Expand Down Expand Up @@ -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 <code>com.oracle.truffle.r.ffi.processor.RFFIResultOwner</code>
* 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<Object> 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() {
Expand Down

0 comments on commit b973926

Please sign in to comment.