From 039ecbe3771aa43f0f372bfb8845f22fc8f77ca2 Mon Sep 17 00:00:00 2001
From: Lukas Stadler <lukas.stadler@oracle.com>
Date: Thu, 2 Jun 2016 17:34:05 +0200
Subject: [PATCH] more careful comparisons on boxing objects
---
.../r/nodes/access/BaseWriteVariableNode.java | 15 ++--
.../access/variables/ReadVariableNode.java | 3 +-
.../env/frame/FrameSlotChangeMonitor.java | 78 ++++++++++++++-----
3 files changed, 70 insertions(+), 26 deletions(-)
diff --git a/com.oracle.truffle.r.nodes/src/com/oracle/truffle/r/nodes/access/BaseWriteVariableNode.java b/com.oracle.truffle.r.nodes/src/com/oracle/truffle/r/nodes/access/BaseWriteVariableNode.java
index 573f5e5c89..b880d98d86 100644
--- a/com.oracle.truffle.r.nodes/src/com/oracle/truffle/r/nodes/access/BaseWriteVariableNode.java
+++ b/com.oracle.truffle.r.nodes/src/com/oracle/truffle/r/nodes/access/BaseWriteVariableNode.java
@@ -69,7 +69,7 @@ abstract class BaseWriteVariableNode extends WriteVariableNode {
* replacement forms of vector updates where a vector is assigned to a temporary (visible)
* variable and then, again, to the original variable (which would cause the vector to be copied
* each time); (non-Javadoc)
- *
+ *
* @see com.oracle.truffle.r.nodes.access.AbstractWriteVariableNode#shareObjectValue(com.oracle.
* truffle .api.frame.Frame, com.oracle.truffle.api.frame.FrameSlot, java.lang.Object,
* com.oracle.truffle.r.nodes.access.AbstractWriteVariableNode.Mode, boolean)
@@ -77,7 +77,6 @@ abstract class BaseWriteVariableNode extends WriteVariableNode {
protected final Object shareObjectValue(Frame frame, FrameSlot frameSlot, Object value, Mode mode, boolean isSuper) {
CompilerAsserts.compilationConstant(mode);
CompilerAsserts.compilationConstant(isSuper);
- Object newValue = value;
// for the meaning of INVISIBLE mode see the comment preceding the current method;
// also change state when assigning to the enclosing frame as there must
// be a distinction between variables with the same name defined in
@@ -85,11 +84,17 @@ abstract class BaseWriteVariableNode extends WriteVariableNode {
// x<-1:3; f<-function() { x[2]<-10; x[2]<<-100; x[2]<-1000 }; f()
// or
// x<-c(1); f<-function() { x[[1]]<<-x[[1]] + 1; x }; a<-f(); b<-f(); c(a,b)
- if ((mode != Mode.INVISIBLE || isSuper) && !isCurrentProfile.profile(isCurrentValue(frame, frameSlot, value))) {
+ if ((mode != Mode.INVISIBLE || isSuper)) {
if (isShareableProfile.profile(value instanceof RShareable)) {
+
+ // this comparison does not work consistently for boxing objects, so it's important
+ // to do the RShareable check first.
+ if (isCurrentProfile.profile(isCurrentValue(frame, frameSlot, value))) {
+ return value;
+ }
RShareable rShareable = (RShareable) shareableProfile.profile(value);
if (mode == Mode.COPY) {
- newValue = rShareable.copy();
+ return rShareable.copy();
} else {
if (isRefCountUpdateable.profile(!rShareable.isSharedPermanent())) {
if (isSuper) {
@@ -104,7 +109,7 @@ abstract class BaseWriteVariableNode extends WriteVariableNode {
}
}
}
- return newValue;
+ return value;
}
private static boolean isCurrentValue(Frame frame, FrameSlot frameSlot, Object value) {
diff --git a/com.oracle.truffle.r.nodes/src/com/oracle/truffle/r/nodes/access/variables/ReadVariableNode.java b/com.oracle.truffle.r.nodes/src/com/oracle/truffle/r/nodes/access/variables/ReadVariableNode.java
index 1e3daa3e19..831e6a87db 100644
--- a/com.oracle.truffle.r.nodes/src/com/oracle/truffle/r/nodes/access/variables/ReadVariableNode.java
+++ b/com.oracle.truffle.r.nodes/src/com/oracle/truffle/r/nodes/access/variables/ReadVariableNode.java
@@ -495,6 +495,7 @@ public final class ReadVariableNode extends RSourceSectionNode implements RSynta
private final LookupResult lookup;
private final ValueProfile frameProfile = ValueProfile.createClassProfile();
+ private final ConditionProfile nullValueProfile = kind == ReadKind.Silent ? null : ConditionProfile.createBinaryProfile();
private LookupLevel(LookupResult lookup) {
this.lookup = lookup;
@@ -509,7 +510,7 @@ public final class ReadVariableNode extends RSourceSectionNode implements RSynta
} else {
value = lookup.getValue();
}
- if (kind != ReadKind.Silent && value == null) {
+ if (kind != ReadKind.Silent && nullValueProfile.profile(value == null)) {
throw RError.error(RError.SHOW_CALLER, mode == RType.Function ? RError.Message.UNKNOWN_FUNCTION : RError.Message.UNKNOWN_OBJECT, identifier);
}
return value;
diff --git a/com.oracle.truffle.r.runtime/src/com/oracle/truffle/r/runtime/env/frame/FrameSlotChangeMonitor.java b/com.oracle.truffle.r.runtime/src/com/oracle/truffle/r/runtime/env/frame/FrameSlotChangeMonitor.java
index f5f15fd1f3..7d19a9c479 100644
--- a/com.oracle.truffle.r.runtime/src/com/oracle/truffle/r/runtime/env/frame/FrameSlotChangeMonitor.java
+++ b/com.oracle.truffle.r.runtime/src/com/oracle/truffle/r/runtime/env/frame/FrameSlotChangeMonitor.java
@@ -59,7 +59,7 @@ public final class FrameSlotChangeMonitor {
* result based on the system's knowledge about the hierarchy of environments and the stable
* values of certain bindings. Most function lookups can be answered based only on this
* information.
- *
+ *
* These lookups are stored for caching and invalidation, i.e., to save on repeated lookups and
* to invalidate lookups in case the environment hierarchy changes.
*/
@@ -458,18 +458,44 @@ public final class FrameSlotChangeMonitor {
return stableValue != null;
}
+ /*
+ * Special cases for primitive types to force value (instead of identity) comparison.
+ */
+
+ public void setValue(byte value, FrameSlot slot) {
+ if (stableValue != null && (!(stableValue.getValue() instanceof Byte) || ((byte) stableValue.getValue()) != value)) {
+ invalidateStableValue(value, slot);
+ }
+ }
+
+ public void setValue(int value, FrameSlot slot) {
+ if (stableValue != null && (!(stableValue.getValue() instanceof Integer) || ((int) stableValue.getValue()) != value)) {
+ invalidateStableValue(value, slot);
+ }
+ }
+
+ public void setValue(double value, FrameSlot slot) {
+ if (stableValue != null && (!(stableValue.getValue() instanceof Double) || ((double) stableValue.getValue()) != value)) {
+ invalidateStableValue(value, slot);
+ }
+ }
+
public void setValue(Object value, FrameSlot slot) {
if (stableValue != null && stableValue.getValue() != value) {
- CompilerDirectives.transferToInterpreterAndInvalidate();
- stableValue.getAssumption().invalidate();
- if (invalidationCount > 0) {
- invalidationCount--;
- out("setting singleton value %s = %s", slot.getIdentifier(), value == null ? "null" : value.getClass());
- stableValue = new StableValue<>(value, String.valueOf(slot.getIdentifier()));
- } else {
- out("setting non-singleton value %s", slot.getIdentifier());
- stableValue = null;
- }
+ invalidateStableValue(value, slot);
+ }
+ }
+
+ private void invalidateStableValue(Object value, FrameSlot slot) {
+ CompilerDirectives.transferToInterpreterAndInvalidate();
+ stableValue.getAssumption().invalidate();
+ if (invalidationCount > 0) {
+ invalidationCount--;
+ out("setting singleton value %s = %s", slot.getIdentifier(), value == null ? "null" : value.getClass());
+ stableValue = new StableValue<>(value, String.valueOf(slot.getIdentifier()));
+ } else {
+ out("setting non-singleton value %s", slot.getIdentifier());
+ stableValue = null;
}
}
@@ -520,12 +546,8 @@ public final class FrameSlotChangeMonitor {
* {@link RInternalError} otherwise
* @param invalidateProfile Used to guard the invalidation code.
*/
- private static void checkAndInvalidate(Frame curFrame, FrameSlot slot, boolean isNonLocal, Object newValue, BranchProfile invalidateProfile) {
+ private static void checkAndInvalidate(Frame curFrame, FrameSlot slot, boolean isNonLocal, BranchProfile invalidateProfile) {
assert curFrame.getFrameDescriptor() == slot.getFrameDescriptor();
- FrameSlotInfoImpl info = getFrameSlotInfo(slot);
- if (info.needsInvalidation()) {
- info.setValue(newValue, slot);
- }
if (getNotChangedNonLocallyAssumption(slot).isValid()) {
// Check whether current frame is used outside a regular stack
@@ -542,22 +564,38 @@ public final class FrameSlotChangeMonitor {
public static void setByteAndInvalidate(Frame frame, FrameSlot frameSlot, byte newValue, boolean isNonLocal, BranchProfile invalidateProfile) {
frame.setByte(frameSlot, newValue);
- checkAndInvalidate(frame, frameSlot, isNonLocal, newValue, invalidateProfile);
+ FrameSlotInfoImpl info = getFrameSlotInfo(frameSlot);
+ if (info.needsInvalidation()) {
+ info.setValue(newValue, frameSlot);
+ }
+ checkAndInvalidate(frame, frameSlot, isNonLocal, invalidateProfile);
}
public static void setIntAndInvalidate(Frame frame, FrameSlot frameSlot, int newValue, boolean isNonLocal, BranchProfile invalidateProfile) {
frame.setInt(frameSlot, newValue);
- checkAndInvalidate(frame, frameSlot, isNonLocal, newValue, invalidateProfile);
+ FrameSlotInfoImpl info = getFrameSlotInfo(frameSlot);
+ if (info.needsInvalidation()) {
+ info.setValue(newValue, frameSlot);
+ }
+ checkAndInvalidate(frame, frameSlot, isNonLocal, invalidateProfile);
}
public static void setDoubleAndInvalidate(Frame frame, FrameSlot frameSlot, double newValue, boolean isNonLocal, BranchProfile invalidateProfile) {
frame.setDouble(frameSlot, newValue);
- checkAndInvalidate(frame, frameSlot, isNonLocal, newValue, invalidateProfile);
+ FrameSlotInfoImpl info = getFrameSlotInfo(frameSlot);
+ if (info.needsInvalidation()) {
+ info.setValue(newValue, frameSlot);
+ }
+ checkAndInvalidate(frame, frameSlot, isNonLocal, invalidateProfile);
}
public static void setObjectAndInvalidate(Frame frame, FrameSlot frameSlot, Object newValue, boolean isNonLocal, BranchProfile invalidateProfile) {
frame.setObject(frameSlot, newValue);
- checkAndInvalidate(frame, frameSlot, isNonLocal, newValue, invalidateProfile);
+ FrameSlotInfoImpl info = getFrameSlotInfo(frameSlot);
+ if (info.needsInvalidation()) {
+ info.setValue(newValue, frameSlot);
+ }
+ checkAndInvalidate(frame, frameSlot, isNonLocal, invalidateProfile);
}
/**
--
GitLab