Short-circuiting || and && #3492

hubertp · 2022-05-27T21:36:05Z

Pull Request Description

Short-circuiting || and && is typically taken for granted
by users of other PLs. This change makes it happen for Enso.

Related to https://www.pivotaltracker.com/story/show/182261401

Checklist

Please include the following checklist in your PR:

The documentation has been updated if necessary.
All code conforms to the
Scala,
Java,
and
Rust
style guides.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed: Enso GUI was tested when built using BOTH
  ./run.sh ide dist and ./run.sh ide watch.

JaroslavTulach

I have to admit I cannot imagine how these changes reflect in the shape of our IGV graphs. It is certainly desirable to verify things like instanceof DataflowError or makeTypeError don't appear in graphs that are trained just with Boolean.

test/Tests/src/Data/Bool_Spec.enso

engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/bool/OrNode.java

engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/bool/AndNode.java

Lazy evaluation of RHS for || and && is typically taken for granted by users of other PLs. This change makes it happen for Enso.

Pushed lhs guard check inside the specialization so as to prevent potentially continuous flipping between optimized and deoptimized version, depending on the LHS value. Simple benchmarks showed 10x improvement compared to the previous solution.

Rather than using multiple specializations in AndNode/OrNode and various instance checks, it seems Truffle is able to better optimize potential DataflowError/Panic by delegating to a separate Node. Benchmarks also appear to be happier that way.

kustosz · 2022-06-02T08:20:30Z

engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/bool/AndNode.java

+      @Suspend Object that,
+      @Cached("build()") ThunkExecutorNode rhsThunkExecutorNode,
+      @Cached("build()") ToBoolNode ensureBoolNode) {
+    if (!_this) {


To make sure the compiler really understands this method, use a profile for this condition.

kustosz · 2022-06-02T08:23:39Z

engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/bool/AndNode.java

+      return new Stateful(state, false);
+    }
+    Stateful result =
+        rhsThunkExecutorNode.executeThunk(that, state, BaseNode.TailStatus.TAIL_DIRECT);


This is not a TAIL_DIRECT, it's a NON_TAIL. TAIL_DIRECT is not guaranteed to return in this method (may unwind)

OTOH I wonder if we really want that type check? Given we don't actually depend on the expression evaluating to bool? Why not just return the result of executeThunk?

kustosz · 2022-06-02T08:26:41Z

engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/bool/ToBoolNode.java

+    return ToBoolNodeGen.create();
+  }
+
+  abstract Stateful execute(@MonadicState Object state, Object value);


This should not return a Stateful, it's not the node's responsibility – the point of this node is "make sure my arg is a bool", so just return a boolean.

kustosz · 2022-06-02T08:27:04Z

engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/bool/ToBoolNode.java

+import org.enso.interpreter.runtime.state.Stateful;
+
+@NodeInfo(description = "Helper node to handle Bool values that can also return DataflowErrors")
+public abstract class ToBoolNode extends Node {


A convention we use is to call such throwing conversions ExpectBoolNode. ToBoolNode suggests you can convert anything.

kustosz

Was to quick to submit the previous review – so here comes the prose. The more I think about the result casting, the less I like it. I'd just allow any return type and not verify it. It's like if you implemented these in Enso:

Boolean.|| ~that = if this then this else that

You wouldn't check the result type in this impl, and I think we should follow this thinking here.

hubertp · 2022-06-02T09:10:02Z

Was to quick to submit the previous review – so here comes the prose. The more I think about the result casting, the less I like it. I'd just allow any return type and not verify it. It's like if you implemented these in Enso:
Boolean.|| ~that = if this then this else that
You wouldn't check the result type in this impl, and I think we should follow this thinking here.

In that case the type of || should be changed from Boolean -> Boolean to Any -> Any. And that sounds a bit counter-intuitive given the actual method description.
e.g., False || 42 would return 42 rather than a runtime exception and that would be completely fine.

kustosz · 2022-06-02T10:08:50Z

No, we should leave the type as is – it is normal for a type signature to be more restrictive than the implementation :)

hubertp · 2022-06-02T10:18:01Z

@kustosz as discussed

kustosz

There's more simplification to do :)

engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/bool/AndNode.java

engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/bool/OrNode.java

hubertp requested review from 4e6, jdunkerley, radeusgd and JaroslavTulach as code owners May 27, 2022 21:36

hubertp changed the title ~~Don't evaluate RHS of || and && eagerly~~ Lazy evaluation of RHS argument for || and && May 27, 2022

JaroslavTulach reviewed May 28, 2022

View reviewed changes

hubertp requested a review from JaroslavTulach May 28, 2022 21:49

hubertp marked this pull request as draft May 28, 2022 22:40

hubertp force-pushed the wip/hubert/short-circuit-bool-ops-182261401 branch 2 times, most recently from 7ef43d7 to 4fb0842 Compare May 29, 2022 10:05

JaroslavTulach reviewed May 30, 2022

View reviewed changes

engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/bool/AndNode.java Outdated Show resolved Hide resolved

hubertp marked this pull request as ready for review May 30, 2022 15:18

hubertp force-pushed the wip/hubert/short-circuit-bool-ops-182261401 branch from fdb2c0e to caaa5bf Compare May 31, 2022 16:35

hubertp added 7 commits June 2, 2022 10:13

Don't evaluate RHS of || and && eagerly

13ee6d7

Lazy evaluation of RHS for || and && is typically taken for granted by users of other PLs. This change makes it happen for Enso.

format

b0fb2da

Changelog

d411831

Rewrite with specializations

261263e

format

572cd85

hubertp force-pushed the wip/hubert/short-circuit-bool-ops-182261401 branch from 4cf6665 to 73716eb Compare June 2, 2022 08:15

hubertp requested review from JaroslavTulach and kustosz June 2, 2022 08:15

Formatting

d1d3016

kustosz suggested changes Jun 2, 2022

View reviewed changes

kustosz reviewed Jun 2, 2022

View reviewed changes

hubertp changed the title ~~Lazy evaluation of RHS argument for || and &&~~ Short circuiting || and && Jun 2, 2022

hubertp changed the title ~~Short circuiting || and &&~~ Short-circuiting || and && Jun 2, 2022

simplify

2c23da9

hubertp requested a review from kustosz June 2, 2022 10:17

hubertp added 2 commits June 2, 2022 12:20

Update documentation

b5de163

Adapt test case to changes

651ea32

hubertp force-pushed the wip/hubert/short-circuit-bool-ops-182261401 branch from 02f707e to 651ea32 Compare June 2, 2022 10:31

kustosz suggested changes Jun 2, 2022

View reviewed changes

PR review

bfa1ea2

hubertp requested a review from kustosz June 2, 2022 12:08

minor: drop unnecessary annotations

292df14

kustosz approved these changes Jun 2, 2022

View reviewed changes

4e6 approved these changes Jun 2, 2022

View reviewed changes

hubertp added the CI: Ready to merge This PR is eligible for automatic merge label Jun 2, 2022

Merge branch 'develop' into wip/hubert/short-circuit-bool-ops-182261401

6d862f3

mergify bot merged commit e43325b into develop Jun 2, 2022

mergify bot deleted the wip/hubert/short-circuit-bool-ops-182261401 branch June 2, 2022 16:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Short-circuiting || and && #3492

Short-circuiting || and && #3492

hubertp commented May 27, 2022 •

edited

Loading

JaroslavTulach left a comment

kustosz Jun 2, 2022

kustosz Jun 2, 2022

kustosz Jun 2, 2022

kustosz Jun 2, 2022

kustosz Jun 2, 2022

kustosz left a comment

hubertp commented Jun 2, 2022 •

edited

Loading

kustosz commented Jun 2, 2022

hubertp commented Jun 2, 2022

kustosz left a comment

Short-circuiting || and && #3492

Short-circuiting || and && #3492

Conversation

hubertp commented May 27, 2022 • edited Loading

Pull Request Description

Checklist

JaroslavTulach left a comment

Choose a reason for hiding this comment

kustosz Jun 2, 2022

Choose a reason for hiding this comment

kustosz Jun 2, 2022

Choose a reason for hiding this comment

kustosz Jun 2, 2022

Choose a reason for hiding this comment

kustosz Jun 2, 2022

Choose a reason for hiding this comment

kustosz Jun 2, 2022

Choose a reason for hiding this comment

kustosz left a comment

Choose a reason for hiding this comment

hubertp commented Jun 2, 2022 • edited Loading

kustosz commented Jun 2, 2022

hubertp commented Jun 2, 2022

kustosz left a comment

Choose a reason for hiding this comment

hubertp commented May 27, 2022 •

edited

Loading

hubertp commented Jun 2, 2022 •

edited

Loading