From b72553ec08f5d101dae290fe551306f346d0e4e0 Mon Sep 17 00:00:00 2001
From: Tomas Stupka <tomas.stupka@oracle.com>
Date: Tue, 29 Aug 2017 17:02:43 +0200
Subject: [PATCH] removed not necessary unboxing and isNull checks when
 converting foreign values

---
 .../truffle/r/nodes/builtin/base/Names.java   | 18 ++++++------------
 .../access/vector/ExtractVectorNode.java      | 19 +------------------
 .../truffle/r/nodes/function/RCallNode.java   | 10 ----------
 .../r/test/library/fastr/TestJavaInterop.java | 16 +++++++++++-----
 4 files changed, 18 insertions(+), 45 deletions(-)

diff --git a/com.oracle.truffle.r.nodes.builtin/src/com/oracle/truffle/r/nodes/builtin/base/Names.java b/com.oracle.truffle.r.nodes.builtin/src/com/oracle/truffle/r/nodes/builtin/base/Names.java
index cc09614f38..6db16cea3c 100644
--- a/com.oracle.truffle.r.nodes.builtin/src/com/oracle/truffle/r/nodes/builtin/base/Names.java
+++ b/com.oracle.truffle.r.nodes.builtin/src/com/oracle/truffle/r/nodes/builtin/base/Names.java
@@ -81,14 +81,12 @@ public abstract class Names extends RBuiltinNode.Arg1 {
     protected Object getNames(TruffleObject obj,
                     @Cached("GET_SIZE.createNode()") Node getSizeNode,
                     @Cached("KEYS.createNode()") Node keysNode,
-                    @Cached("READ.createNode()") Node readNode,
-                    @Cached("IS_BOXED.createNode()") Node isBoxedNode,
-                    @Cached("UNBOX.createNode()") Node unboxNode) {
+                    @Cached("READ.createNode()") Node readNode) {
 
         try {
             String[] names;
             try {
-                names = readKeys(keysNode, obj, getSizeNode, readNode, isBoxedNode, unboxNode);
+                names = readKeys(keysNode, obj, getSizeNode, readNode);
             } catch (UnsupportedMessageException e) {
                 // because it is a java function, java.util.Map (has special handling too) ... ?
                 return RNull.instance;
@@ -96,7 +94,7 @@ public abstract class Names extends RBuiltinNode.Arg1 {
             String[] staticNames = new String[0];
             try {
                 if (JavaInterop.isJavaObject(Object.class, obj)) {
-                    staticNames = readKeys(keysNode, toJavaClass(obj), getSizeNode, readNode, isBoxedNode, unboxNode);
+                    staticNames = readKeys(keysNode, toJavaClass(obj), getSizeNode, readNode);
                 }
             } catch (UnknownIdentifierException | NoSuchFieldError | UnsupportedMessageException e) {
                 // because it is a class ... ?
@@ -118,18 +116,14 @@ public abstract class Names extends RBuiltinNode.Arg1 {
         return JavaInterop.toJavaClass(obj);
     }
 
-    private static String[] readKeys(Node keysNode, TruffleObject obj, Node getSizeNode, Node readNode, Node isBoxedNode, Node unboxNode)
+    private static String[] readKeys(Node keysNode, TruffleObject obj, Node getSizeNode, Node readNode)
                     throws UnknownIdentifierException, InteropException, UnsupportedMessageException {
-        TruffleObject keys = (TruffleObject) ForeignAccess.send(keysNode, obj);
+        TruffleObject keys = ForeignAccess.sendKeys(keysNode, obj);
         if (keys != null) {
             int size = (Integer) ForeignAccess.sendGetSize(getSizeNode, keys);
             String[] names = new String[size];
             for (int i = 0; i < size; i++) {
-                Object value;
-                value = ForeignAccess.sendRead(readNode, keys, i);
-                if (value instanceof TruffleObject && ForeignAccess.sendIsBoxed(isBoxedNode, (TruffleObject) value)) {
-                    value = ForeignAccess.sendUnbox(unboxNode, (TruffleObject) value);
-                }
+                Object value = ForeignAccess.sendRead(readNode, keys, i);
                 names[i] = (String) value;
             }
             return names;
diff --git a/com.oracle.truffle.r.nodes/src/com/oracle/truffle/r/nodes/access/vector/ExtractVectorNode.java b/com.oracle.truffle.r.nodes/src/com/oracle/truffle/r/nodes/access/vector/ExtractVectorNode.java
index a493d13ce2..93bad4256d 100644
--- a/com.oracle.truffle.r.nodes/src/com/oracle/truffle/r/nodes/access/vector/ExtractVectorNode.java
+++ b/com.oracle.truffle.r.nodes/src/com/oracle/truffle/r/nodes/access/vector/ExtractVectorNode.java
@@ -31,7 +31,6 @@ import com.oracle.truffle.api.interop.ForeignAccess;
 import com.oracle.truffle.api.interop.InteropException;
 import com.oracle.truffle.api.interop.KeyInfo;
 import com.oracle.truffle.api.interop.TruffleObject;
-import com.oracle.truffle.api.interop.UnsupportedMessageException;
 import com.oracle.truffle.api.interop.java.JavaInterop;
 import com.oracle.truffle.api.nodes.Node;
 import com.oracle.truffle.api.profiles.ValueProfile;
@@ -220,16 +219,12 @@ public abstract class ExtractVectorNode extends RBaseNode {
                     @Cached("create()") CastStringNode castNode,
                     @Cached("createFirstString()") FirstStringNode firstString,
                     @Cached("createClassProfile()") ValueProfile positionProfile,
-                    @Cached("IS_NULL.createNode()") Node isNullNode,
-                    @Cached("IS_BOXED.createNode()") Node isBoxedNode,
-                    @Cached("UNBOX.createNode()") Node unboxNode,
                     @Cached("createForeign2RNode()") Foreign2R foreign2RNode) {
         Object[] pos = positionProfile.profile(positions);
         if (pos.length == 0) {
             throw error(RError.Message.GENERIC, "No positions for foreign access.");
         }
         try {
-            // TODO implicite unboxing ok? method calls seem to behave this way
             Object result = object;
             for (int i = 0; i < pos.length; i++) {
                 result = read(this, pos[i], foreignRead, keyInfoNode, hasSizeNode, (TruffleObject) result, firstString, castNode);
@@ -237,24 +232,12 @@ public abstract class ExtractVectorNode extends RBaseNode {
                     assert result instanceof TruffleObject;
                 }
             }
-            return unbox(result, isNullNode, isBoxedNode, unboxNode, foreign2RNode);
+            return foreign2RNode.execute(result);
         } catch (InteropException | NoSuchFieldError e) {
             throw RError.interopError(RError.findParentRBase(this), e, object);
         }
     }
 
-    private static Object unbox(Object obj, Node isNullNode, Node isBoxedNode, Node unboxNode, Foreign2R foreign2RNode) throws UnsupportedMessageException {
-        if (RRuntime.isForeignObject(obj)) {
-            if (ForeignAccess.sendIsNull(isNullNode, (TruffleObject) obj)) {
-                return RNull.instance;
-            }
-            if (ForeignAccess.sendIsBoxed(isBoxedNode, (TruffleObject) obj)) {
-                return foreign2RNode.execute(ForeignAccess.sendUnbox(unboxNode, (TruffleObject) obj));
-            }
-        }
-        return foreign2RNode.execute(obj);
-    }
-
     public static Object read(RBaseNode caller, Object positions, Node foreignRead, Node keyInfoNode, Node hasSizeNode, TruffleObject object, FirstStringNode firstString, CastStringNode castNode)
                     throws RError, InteropException {
         Object pos = positions;
diff --git a/com.oracle.truffle.r.nodes/src/com/oracle/truffle/r/nodes/function/RCallNode.java b/com.oracle.truffle.r.nodes/src/com/oracle/truffle/r/nodes/function/RCallNode.java
index b1732b1e1c..58a993f871 100644
--- a/com.oracle.truffle.r.nodes/src/com/oracle/truffle/r/nodes/function/RCallNode.java
+++ b/com.oracle.truffle.r.nodes/src/com/oracle/truffle/r/nodes/function/RCallNode.java
@@ -539,7 +539,6 @@ public abstract class RCallNode extends RCallBaseNode implements RSyntaxNode, RS
 
         @Child private CallArgumentsNode arguments;
         @Child private Node foreignCall;
-        @Child private Node isNullCall;
         @CompilationFinal private int foreignCallArgCount;
 
         public ForeignCall(CallArgumentsNode arguments) {
@@ -558,15 +557,6 @@ public abstract class RCallNode extends RCallBaseNode implements RSyntaxNode, RS
                     argumentsArray[i] = r2Foreign.execute(argumentsArray[i]);
                 }
                 Object result = ForeignAccess.sendExecute(foreignCall, function, argumentsArray);
-                if (RRuntime.isForeignObject(result)) {
-                    if (isNullCall == null) {
-                        CompilerDirectives.transferToInterpreterAndInvalidate();
-                        isNullCall = insert(Message.IS_NULL.createNode());
-                    }
-                    if (ForeignAccess.sendIsNull(isNullCall, (TruffleObject) result)) {
-                        return RNull.instance;
-                    }
-                }
                 return foreign2R.execute(result);
             } catch (ArityException | UnsupportedMessageException | UnsupportedTypeException e) {
                 CompilerDirectives.transferToInterpreter();
diff --git a/com.oracle.truffle.r.test/src/com/oracle/truffle/r/test/library/fastr/TestJavaInterop.java b/com.oracle.truffle.r.test/src/com/oracle/truffle/r/test/library/fastr/TestJavaInterop.java
index 6c62351cee..e28f6b0ebb 100644
--- a/com.oracle.truffle.r.test/src/com/oracle/truffle/r/test/library/fastr/TestJavaInterop.java
+++ b/com.oracle.truffle.r.test/src/com/oracle/truffle/r/test/library/fastr/TestJavaInterop.java
@@ -390,6 +390,8 @@ public class TestJavaInterop extends TestBase {
                 testForValue(name, f.get(t));
             }
         }
+        assertEvalFastR(CREATE_TRUFFLE_OBJECT + " to$mixedTypesArray",
+                        "cat('[external object]\n[[1]]\n[1] 1\n\n[[2]]\n[1] 2.1\n\n[[3]]\n[1] \"a\"\n\n[[4]]\n[1] TRUE\n\n[[5]]\nNULL\n\n', sep='')");
     }
 
     @Test
@@ -1417,12 +1419,16 @@ public class TestJavaInterop extends TestBase {
         return sb.toString();
     }
 
-    private static String getBooleanPrefix(Object value, int i) {
-        if (value.getClass().getComponentType() == Boolean.TYPE && (boolean) Array.get(value, i)) {
+    private static String getBooleanPrefix(Object array, int i) {
+        Object element = Array.get(array, i);
+        if (element == null) {
+            return "";
+        }
+        if (array.getClass().getComponentType() == Boolean.TYPE && (boolean) element) {
             return " ";
         }
-        if (i > 0 && value.getClass().getComponentType() == String.class &&
-                        (Array.get(value, i).equals("T") || Array.get(value, i).equals("F") || Array.get(value, i).equals("TRUE") || Array.get(value, i).equals("FALSE"))) {
+        if (i > 0 && array.getClass().getComponentType() == String.class &&
+                        (element.equals("T") || element.equals("F") || element.equals("TRUE") || element.equals("FALSE"))) {
             return " ";
         }
         return "";
@@ -1830,7 +1836,7 @@ public class TestJavaInterop extends TestBase {
             objectArray = new Object[]{new Object(), new Object(), new Object()};
             objectIntArray = new Object[]{1, 2, 3};
             objectDoubleArray = new Object[]{1.1, 2.1, 3.1};
-            mixedTypesArray = new Object[]{1, 2.1, 'a'};
+            mixedTypesArray = new Object[]{1, 2.1, 'a', true, null};
             hasNullIntArray = new Integer[]{1, null, 3};
 
             map = new HashMap<>();
-- 
GitLab