Skip to content

Commit 9bb6796

Browse files
zslajchrtzslajchrt
committed
[GR-16001] Regression in shootout fixed.
PullRequest: fastr/2044
2 parents 611585f + f622114 commit 9bb6796

File tree

10 files changed

+91
-27
lines changed

10 files changed

+91
-27
lines changed

‎com.oracle.truffle.r.nodes/src/com/oracle/truffle/r/nodes/access/WriteVariableSyntaxNode.java‎

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2013, 2018, Oracle and/or its affiliates. All rights reserved.
2+
* Copyright (c) 2013, 2019, Oracle and/or its affiliates. All rights reserved.
33
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
44
*
55
* This code is free software; you can redistribute it and/or modify it
@@ -31,14 +31,33 @@
3131
import com.oracle.truffle.api.source.SourceSection;
3232
import com.oracle.truffle.r.nodes.access.WriteVariableNode.Mode;
3333
import com.oracle.truffle.r.nodes.control.OperatorNode;
34+
import com.oracle.truffle.r.nodes.function.ArgumentMatcher;
3435
import com.oracle.truffle.r.nodes.function.visibility.SetVisibilityNode;
3536
import com.oracle.truffle.r.runtime.ArgumentsSignature;
37+
import com.oracle.truffle.r.runtime.RArguments;
3638
import com.oracle.truffle.r.runtime.nodes.RNode;
3739
import com.oracle.truffle.r.runtime.nodes.RSyntaxElement;
3840
import com.oracle.truffle.r.runtime.nodes.RSyntaxLookup;
3941

4042
/**
4143
* This node represents a write to a variable on the syntax level, i.e., in the R source code.
44+
* <p>
45+
* When executed in the context of an eager promise evaluation, it must be checked whether there are
46+
* no side effects, which is the important assumption for the eager promise evaluation. Unless the
47+
* write node writes outside the current frame (i.e. if {@code isSuper} is false), it is assumed
48+
* that the operation has no side effect and thus safe w.r.t. the eager promise evaluation. Any
49+
* changes made by this node in the current frame will be destroyed in the case that any subsequent
50+
* operation will cancel the ongoing eager promise evaluation by throwing
51+
* {@code CannotOptimizePromise}.
52+
* <p>
53+
* NB: When an assignment is present in an argument expression, the argument is evicted from the
54+
* eager evaluation (see {@code matchNodes} in {@link ArgumentMatcher}). Therefore, in the following
55+
* example, the argument of {@code foo} will not be evaluated eagerly:
56+
*
57+
* <pre>
58+
* foo <- function(a) a
59+
* bar <- function() { x <- 3; foo(x <- 4); print(x); }
60+
* </pre>
4261
*/
4362
@NodeInfo(cost = NONE)
4463
public final class WriteVariableSyntaxNode extends OperatorNode {
@@ -47,41 +66,61 @@ public final class WriteVariableSyntaxNode extends OperatorNode {
4766
@Child private SetVisibilityNode visibility;
4867

4968
private final RSyntaxElement lhs;
69+
private final boolean isSuper;
5070

5171
public WriteVariableSyntaxNode(SourceSection source, RSyntaxLookup operator, RSyntaxElement lhs, String name, RNode rhs, boolean isSuper) {
5272
super(source, operator);
5373
this.lhs = lhs;
5474
this.write = WriteVariableNode.createAnonymous(name, Mode.REGULAR, rhs, isSuper);
75+
this.isSuper = isSuper;
5576
assert write != null;
5677
}
5778

5879
@Override
5980
public void voidExecute(VirtualFrame frame) {
81+
if (isSuper) {
82+
RArguments.getCall(frame).checkEagerPromiseOnly();
83+
}
6084
write.execute(frame);
6185
}
6286

6387
@Override
6488
public Object execute(VirtualFrame frame) {
89+
if (isSuper) {
90+
RArguments.getCall(frame).checkEagerPromiseOnly();
91+
}
6592
return write.execute(frame);
6693
}
6794

6895
@Override
6996
public int executeInteger(VirtualFrame frame) throws UnexpectedResultException {
97+
if (isSuper) {
98+
RArguments.getCall(frame).checkEagerPromiseOnly();
99+
}
70100
return write.executeInteger(frame);
71101
}
72102

73103
@Override
74104
public double executeDouble(VirtualFrame frame) throws UnexpectedResultException {
105+
if (isSuper) {
106+
RArguments.getCall(frame).checkEagerPromiseOnly();
107+
}
75108
return write.executeDouble(frame);
76109
}
77110

78111
@Override
79112
public byte executeByte(VirtualFrame frame) throws UnexpectedResultException {
113+
if (isSuper) {
114+
RArguments.getCall(frame).checkEagerPromiseOnly();
115+
}
80116
return write.executeByte(frame);
81117
}
82118

83119
@Override
84120
public Object visibleExecute(VirtualFrame frame) {
121+
if (isSuper) {
122+
RArguments.getCall(frame).checkEagerPromiseOnly();
123+
}
85124
Object result = write.execute(frame);
86125
if (visibility == null) {
87126
CompilerDirectives.transferToInterpreterAndInvalidate();

‎com.oracle.truffle.r.nodes/src/com/oracle/truffle/r/nodes/builtin/InternalNode.java‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,11 +41,13 @@
4141
import com.oracle.truffle.r.nodes.function.S3FunctionLookupNode.Result;
4242
import com.oracle.truffle.r.nodes.function.visibility.SetVisibilityNode;
4343
import com.oracle.truffle.r.runtime.ArgumentsSignature;
44+
import com.oracle.truffle.r.runtime.RArguments;
4445
import com.oracle.truffle.r.runtime.RDispatch;
4546
import com.oracle.truffle.r.runtime.RError;
4647
import com.oracle.truffle.r.runtime.RError.Message;
4748
import com.oracle.truffle.r.runtime.RInternalError;
4849
import com.oracle.truffle.r.runtime.RRuntime;
50+
import com.oracle.truffle.r.runtime.builtins.RBehavior;
4951
import com.oracle.truffle.r.runtime.builtins.RBuiltinKind;
5052
import com.oracle.truffle.r.runtime.conn.RConnection;
5153
import com.oracle.truffle.r.runtime.context.RContext;
@@ -201,6 +203,7 @@ private abstract static class InternalCallNode extends InternalNode {
201203

202204
protected final RBuiltinFactory factory;
203205
protected final int varArgIndex;
206+
private final boolean pure;
204207

205208
@Children protected final RNode[] arguments;
206209
@Child private RBuiltinNode builtin;
@@ -216,6 +219,9 @@ private abstract static class InternalCallNode extends InternalNode {
216219
arguments[i] = ((RSyntaxNode) args[i]).asRNode();
217220
}
218221
this.varArgIndex = factory.getSignature().getVarArgIndex();
222+
223+
RBehavior behavior = factory.getBehavior();
224+
pure = behavior != null ? behavior.isPure() : false;
219225
}
220226

221227
@Override
@@ -227,6 +233,8 @@ public RBaseNode getErrorContext() {
227233

228234
@Override
229235
public Object execute(VirtualFrame frame) {
236+
checkEagerPromiseOnly(frame);
237+
230238
Object[] args = prepareArgs(frame);
231239
Object result = doInternalDispatch(frame, args);
232240
if (result == null) {
@@ -238,6 +246,12 @@ public Object execute(VirtualFrame frame) {
238246
return result;
239247
}
240248

249+
private void checkEagerPromiseOnly(VirtualFrame frame) {
250+
if (!pure) {
251+
RArguments.getCall(frame).checkEagerPromiseOnly();
252+
}
253+
}
254+
241255
private Object doInternalDispatch(VirtualFrame frame, Object[] args) {
242256
if (factory.getDispatch() != RDispatch.INTERNAL_GENERIC) {
243257
return null;
@@ -251,6 +265,8 @@ private Object doInternalDispatch(VirtualFrame frame, Object[] args) {
251265

252266
@Override
253267
public void voidExecute(VirtualFrame frame) {
268+
checkEagerPromiseOnly(frame);
269+
254270
builtin.call(frame, prepareArgs(frame));
255271
}
256272
}

‎com.oracle.truffle.r.nodes/src/com/oracle/truffle/r/nodes/control/ReplacementNode.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@
4040
import com.oracle.truffle.r.nodes.function.visibility.SetVisibilityNode;
4141
import com.oracle.truffle.r.nodes.unary.GetNonSharedNode;
4242
import com.oracle.truffle.r.runtime.ArgumentsSignature;
43+
import com.oracle.truffle.r.runtime.RArguments;
4344
import com.oracle.truffle.r.runtime.builtins.RBuiltinDescriptor;
4445
import com.oracle.truffle.r.runtime.builtins.RSpecialFactory.FullCallNeededException;
4546
import com.oracle.truffle.r.runtime.context.RContext;
@@ -212,6 +213,7 @@ private abstract static class ReplacementWithRhsNode extends ReplacementNode {
212213

213214
@Override
214215
public final Object execute(VirtualFrame frame) {
216+
RArguments.getCall(frame).checkEagerPromiseOnly();
215217
storeRhs.execute(frame);
216218
executeReplacement(frame);
217219
try {
@@ -227,6 +229,7 @@ public final Object execute(VirtualFrame frame) {
227229

228230
@Override
229231
public final void voidExecute(VirtualFrame frame) {
232+
RArguments.getCall(frame).checkEagerPromiseOnly();
230233
storeRhs.execute(frame);
231234
executeReplacement(frame);
232235
removeRhs.execute(frame);

‎com.oracle.truffle.r.nodes/src/com/oracle/truffle/r/nodes/function/FunctionDefinitionNode.java‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@
7272
import com.oracle.truffle.r.runtime.data.ClosureCache.RNodeClosureCache;
7373
import com.oracle.truffle.r.runtime.data.RNull;
7474
import com.oracle.truffle.r.runtime.data.RPairList;
75+
import com.oracle.truffle.r.runtime.env.frame.CannotOptimizePromise;
7576
import com.oracle.truffle.r.runtime.env.frame.FrameSlotChangeMonitor;
7677
import com.oracle.truffle.r.runtime.env.frame.RFrameSlot;
7778
import com.oracle.truffle.r.runtime.interop.FastRInteropTryException;
@@ -318,7 +319,12 @@ public Object execute(VirtualFrame frame) {
318319
*/
319320
runOnExitHandlers = false;
320321
throw e;
322+
} else if (e instanceof CannotOptimizePromise) {
323+
assert RArguments.getCall(frame).evaluateOnlyEagerPromises();
324+
runOnExitHandlers = false;
325+
throw e;
321326
} else if (e instanceof FastRInteropTryException) {
327+
assert !RArguments.getCall(frame).evaluateOnlyEagerPromises();
322328
throw e;
323329
} else if (e instanceof TruffleException && !((TruffleException) e).isInternalError()) {
324330
throw e;

‎com.oracle.truffle.r.nodes/src/com/oracle/truffle/r/nodes/function/PromiseHelperNode.java‎

Lines changed: 1 addition & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
import com.oracle.truffle.api.profiles.PrimitiveValueProfile;
4343
import com.oracle.truffle.api.profiles.ValueProfile;
4444
import com.oracle.truffle.r.nodes.InlineCacheNode;
45+
import com.oracle.truffle.r.nodes.function.PromiseHelperNodeFactory.GenerateValueForEagerPromiseNodeGen;
4546
import com.oracle.truffle.r.nodes.function.call.CallerFrameClosureProvider;
4647
import com.oracle.truffle.r.nodes.function.opt.ShareObjectNode;
4748
import com.oracle.truffle.r.nodes.function.visibility.GetVisibilityNode;
@@ -57,9 +58,7 @@
5758
import com.oracle.truffle.r.runtime.data.RPromise.EagerPromise;
5859
import com.oracle.truffle.r.runtime.data.RPromise.PromiseState;
5960
import com.oracle.truffle.r.runtime.data.RShareable;
60-
import com.oracle.truffle.r.runtime.env.frame.CannotOptimizePromise;
6161
import com.oracle.truffle.r.runtime.nodes.RBaseNode;
62-
import com.oracle.truffle.r.nodes.function.PromiseHelperNodeFactory.GenerateValueForEagerPromiseNodeGen;
6362

6463
/**
6564
* Holds {@link RPromise}-related functionality that cannot be implemented in
@@ -161,14 +160,6 @@ public PromiseHelperNode(byte recursiveCounter) {
161160
@Child private GenerateValueForEagerPromiseNode generateValueNonDefaultOptimizedNode;
162161
private final ValueProfile promiseFrameProfile = ValueProfile.createClassProfile();
163162

164-
private static void checkFullPromise(Frame frame) {
165-
if (frame != null) {
166-
if (RArguments.getCall(frame).evaluateOnlyEagerPromises()) {
167-
throw new CannotOptimizePromise();
168-
}
169-
}
170-
}
171-
172163
/**
173164
* Main entry point for proper evaluation of the given Promise when visibility updating is not
174165
* required; including {@link RPromise#isEvaluated()}, dependency cycles. Guarded by
@@ -224,8 +215,6 @@ private Object evaluateImpl(VirtualFrame frame, RPromise promise, boolean visibl
224215
}
225216

226217
private Object generateValueFromFullPromise(VirtualFrame frame, RPromise promise, boolean visibleExec) {
227-
checkFullPromise(frame);
228-
229218
// Check for dependency cycle
230219
if (isUnderEvaluation(promise)) {
231220
throw RError.error(RError.SHOW_CALLER, RError.Message.PROMISE_CYCLE);
@@ -297,14 +286,11 @@ private Object generateValueFromEagerPromise(VirtualFrame frame, int state, Eage
297286
if (result != null) {
298287
return result;
299288
} else {
300-
checkFullPromise(frame);
301289
// Fallback: eager evaluation failed, now take the slow path
302290
CompilerDirectives.transferToInterpreter();
303291
promise.notifyFailure();
304292
promise.materialize();
305293
}
306-
} else {
307-
checkFullPromise(frame);
308294
}
309295

310296
// Call
@@ -359,8 +345,6 @@ public static Object evaluateSlowPath(VirtualFrame frame, RPromise promise, bool
359345
}
360346

361347
private static Object generateValueFromFullPromiseSlowPath(VirtualFrame frame, RPromise promise, boolean visibleExec) {
362-
checkFullPromise(frame);
363-
364348
// Check for dependency cycle
365349
if (promise.isUnderEvaluation()) {
366350
throw RError.error(RError.SHOW_CALLER, RError.Message.PROMISE_CYCLE);
@@ -407,14 +391,11 @@ private static Object generateValueFromEagerPromiseSlowPath(VirtualFrame frame,
407391
return o;
408392
}
409393
} else {
410-
checkFullPromise(frame);
411394
promise.notifyFailure();
412395

413396
// Fallback: eager evaluation failed, now take the slow path
414397
promise.materialize();
415398
}
416-
} else {
417-
checkFullPromise(frame);
418399
}
419400
// Call
420401
return generateValueFromFullPromiseSlowPath(frame, promise, visibleExec);

‎com.oracle.truffle.r.nodes/src/com/oracle/truffle/r/nodes/function/PromiseNode.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,7 @@ static RNode create(RPromiseFactory factory, boolean noOpt, boolean forcedEager,
116116
if (arg instanceof WrapArgumentNode) {
117117
wrapIndex = ((WrapArgumentNode) arg).getIndex();
118118
}
119-
if (forcedEager && EvaluatedArgumentsVisitor.isSimpleArgument(expr.asRSyntaxNode())) {
119+
if (forcedEager) {
120120
return new OptForcedEagerPromiseNode(factory, wrapIndex, allArgPromisesCanOptimize);
121121
} else {
122122
Object optimizableConstant = getOptimizableConstant(expr);

‎com.oracle.truffle.r.nodes/src/com/oracle/truffle/r/nodes/function/RCallNode.java‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,7 @@
9292
import com.oracle.truffle.r.runtime.RVisibility;
9393
import com.oracle.truffle.r.runtime.Utils;
9494
import com.oracle.truffle.r.runtime.builtins.FastPathFactory;
95+
import com.oracle.truffle.r.runtime.builtins.RBehavior;
9596
import com.oracle.truffle.r.runtime.builtins.RBuiltinDescriptor;
9697
import com.oracle.truffle.r.runtime.conn.RConnection;
9798
import com.oracle.truffle.r.runtime.context.RContext;
@@ -1008,6 +1009,7 @@ public static final class BuiltinCallNode extends LeafCallFunctionNode {
10081009
private final FormalArguments formals;
10091010
private final RBuiltinDescriptor builtinDescriptor;
10101011
private final boolean explicitArgs;
1012+
private final boolean pure;
10111013

10121014
public BuiltinCallNode(RBuiltinNode builtin, RBuiltinDescriptor builtinDescriptor, FormalArguments formalArguments, RCallNode originalCall, boolean explicitArgs) {
10131015
super(originalCall);
@@ -1020,6 +1022,9 @@ public BuiltinCallNode(RBuiltinNode builtin, RBuiltinDescriptor builtinDescripto
10201022
varArgSeen = new boolean[formals.getLength()];
10211023
nonWrapSeen = new boolean[formals.getLength()];
10221024
wrapSeen = new boolean[formals.getLength()];
1025+
1026+
RBehavior behavior = builtinDescriptor.getBehavior();
1027+
pure = behavior != null ? behavior.isPure() : false;
10231028
}
10241029

10251030
@Override
@@ -1137,6 +1142,10 @@ private static Object createPromise(Object arg) {
11371142

11381143
@Override
11391144
public Object execute(VirtualFrame frame, RFunction currentFunction, RArgsValuesAndNames orderedArguments, S3Args s3Args) {
1145+
if (!pure) {
1146+
RArguments.getCall(frame).checkEagerPromiseOnly();
1147+
}
1148+
11401149
Object result = builtin.call(frame, forceArgPromises(frame, orderedArguments.getArguments()));
11411150
assert result != null : "builtins cannot return 'null': " + builtinDescriptor.getName();
11421151
assert !(result instanceof RConnection) : "builtins cannot return connection': " + builtinDescriptor.getName();

‎com.oracle.truffle.r.runtime/src/com/oracle/truffle/r/runtime/RCaller.java‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,10 +25,12 @@
2525
import java.util.function.Consumer;
2626
import java.util.function.Supplier;
2727

28+
import com.oracle.truffle.api.CompilerDirectives;
2829
import com.oracle.truffle.api.frame.Frame;
2930
import com.oracle.truffle.api.profiles.BranchProfile;
3031
import com.oracle.truffle.api.profiles.ConditionProfile;
3132
import com.oracle.truffle.r.runtime.env.REnvironment;
33+
import com.oracle.truffle.r.runtime.env.frame.CannotOptimizePromise;
3234
import com.oracle.truffle.r.runtime.env.frame.RFrameSlot;
3335
import com.oracle.truffle.r.runtime.nodes.RSyntaxElement;
3436
import com.oracle.truffle.r.runtime.nodes.RSyntaxNode;
@@ -165,6 +167,7 @@ private RCaller(int depth, RCaller previous, Object payload) {
165167
this.depth = depth;
166168
this.previous = previous;
167169
this.payload = payload;
170+
this.evaluateOnlyEagerPromises = previous != null ? previous.evaluateOnlyEagerPromises : false;
168171
}
169172

170173
private static int depthFromFrame(Frame callingFrame) {
@@ -471,6 +474,13 @@ public void setEvaluateOnlyEagerPromises(boolean evaluateOnlyEagerPromises) {
471474
this.evaluateOnlyEagerPromises = evaluateOnlyEagerPromises;
472475
}
473476

477+
public void checkEagerPromiseOnly() {
478+
if (evaluateOnlyEagerPromises()) {
479+
CompilerDirectives.transferToInterpreterAndInvalidate();
480+
throw new CannotOptimizePromise();
481+
}
482+
}
483+
474484
/**
475485
* An instance of one of two subclasses of this class is held in the {@link RCaller#payload}
476486
* field. Let's call such an {@link RCaller} instance the owner of this instance. The

‎com.oracle.truffle.r.runtime/src/com/oracle/truffle/r/runtime/context/FastROptions.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,7 @@ public class FastROptions {
102102
@Option(category = OptionCategory.INTERNAL, help = "Factor by which are multiplied all DSL 'limit' values where applicable.") //
103103
public static final OptionKey<Double> DSLCacheSizeFactor = new OptionKey<>(1.0);
104104
@Option(category = OptionCategory.EXPERT, help = "Aproximate block size limit given in AST nodes. Bigger blocks will be split into smaller units.") //
105-
public static final OptionKey<Integer> BlockSizeLimit = new OptionKey<>(50);
105+
public static final OptionKey<Integer> BlockSizeLimit = new OptionKey<>(400);
106106
@Option(category = OptionCategory.EXPERT, help = "Skip block size evaluation if amount of direct children nodes is <= than the given value.") //
107107
public static final OptionKey<Integer> BlockSequenceSizeLimit = new OptionKey<>(5);
108108

@@ -116,8 +116,8 @@ public class FastROptions {
116116
@Option(category = OptionCategory.EXPERT, help = "Restrict force splitting of call targets") //
117117
public static final OptionKey<Boolean> RestrictForceSplitting = new OptionKey<>(true);
118118

119-
// Dicontinued since rc12
120-
// only a warning is printed to use the default logger mechaninsm
119+
// Discontinued since rc12
120+
// only a warning is printed to use the default logger mechanism
121121
// TODO remove at some later point
122122
@Option(category = OptionCategory.EXPERT, help = "Print FastR performance warning") //
123123
public static final OptionKey<Boolean> PerformanceWarnings = new OptionKey<>(false);

0 commit comments

Comments
 (0)