Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Equality with conversions #9070

Merged
merged 29 commits into from
Feb 19, 2024
Merged

Equality with conversions #9070

merged 29 commits into from
Feb 19, 2024

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Feb 15, 2024 •
edited
Loading

Pull Request Description

Fixes #8855 by providing support for == between different types with conversions.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

@JaroslavTulach JaroslavTulach marked this pull request as draft February 15, 2024 04:53
@JaroslavTulach JaroslavTulach changed the title Wip/jtulach/equality with conversion 8855 Equality with conversions Feb 15, 2024
@JaroslavTulach JaroslavTulach self-assigned this Feb 15, 2024
@JaroslavTulach JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label Feb 15, 2024
@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Feb 15, 2024 •
edited
Loading

There are currently failures in EnsoMultiValue conversions. Looks like the tests need to be updated to the new behavior: bbbbf73

@JaroslavTulach
Copy link
Member Author

First benchmark results are here and at least the equalsPrimitives benchmark seems negatively affected:

equalsPrimitives

@JaroslavTulach JaroslavTulach marked this pull request as ready for review February 15, 2024 14:00
@enso-bot enso-bot bot mentioned this pull request Feb 16, 2024
Closed
@JaroslavTulach JaroslavTulach force-pushed the wip/jtulach/EqualityWithConversion_8855 branch from f33faad to 69510cd Compare February 16, 2024 05:32
@JaroslavTulach
Copy link
Member Author

Second run of benchmarks seem to indicate equalsPrimitives are now fine:

equalsPrimitives

Akirathan
Akirathan approved these changes Feb 16, 2024
View reviewed changes
Copy link
Member

@Akirathan Akirathan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Please, don't forget to provide some docs somewhere how the equality works now. It's starting to get really complex.

engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/EqualsNode.java Outdated Show resolved Hide resolved
engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/EqualsNode.java
} catch (ArityException ex) {
var assertsOn = false;
assert assertsOn = true;
if (assertsOn) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just panic here? ArityException is unexpected here, no? So it should result in some IllegalStateException

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The answer is probably the same as in the previous discussion about the topic.

...untime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/EqualsSimpleNode.java Show resolved Hide resolved
...ntime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/EqualsComplexNode.java Show resolved Hide resolved
@Akirathan Akirathan added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Feb 16, 2024
@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Feb 19, 2024 •
edited
Loading

Running benchmarks again for engine and stdlib. The results aren't bad, so let's integrate this.

@JaroslavTulach JaroslavTulach merged commit a664dd9 into develop Feb 19, 2024
25 of 26 checks passed
@JaroslavTulach JaroslavTulach deleted the wip/jtulach/EqualityWithConversion_8855 branch February 19, 2024 16:18
@Akirathan Akirathan mentioned this pull request Mar 4, 2024
Merged
1 task
mergify bot pushed a commit that referenced this pull request Mar 5, 2024
@Akirathan
Enable splitting for EqualsSimpleNode (#9268)
f02213a 
Fixes the regression introduced by #9070 in `org.enso.benchmarks.generated.Collections.list_meta_fold` benchmark.

# Important Notes
As can be seen on the graph in IGV:
![image](https://github.com/enso-org/enso/assets/14013887/31b6ceca-4909-4a8f-987f-b456b3fb0a1b)
For some reason, `EqualsSimpleNode` is POLYMORPHIC. That seems to be the most visible performance problem.

First, I tried to introduce `ConditionProfile` with:
```diff
diff --git a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/EqualsNode.java b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/EqualsNode.java
index b368fb7fe..57274b37e 100644
--- a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/EqualsNode.java
+++ b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/EqualsNode.java
@@ -9,6 +9,7 @@ import com.oracle.truffle.api.dsl.Specialization;
import com.oracle.truffle.api.frame.VirtualFrame;
import com.oracle.truffle.api.interop.ArityException;
import com.oracle.truffle.api.nodes.Node;
+import com.oracle.truffle.api.profiles.ConditionProfile;
import org.enso.interpreter.dsl.AcceptsError;
import org.enso.interpreter.dsl.BuiltinMethod;
import org.enso.interpreter.node.EnsoRootNode;
@@ -46,6 +47,7 @@ public final class EqualsNode extends Node {
@child private EqualsSimpleNode node;
@child private TypeOfNode types;
@child private WithConversionNode convert;
+  private final ConditionProfile equalsProfile = ConditionProfile.create();

private static final EqualsNode UNCACHED =
new EqualsNode(EqualsSimpleNodeGen.getUncached(), TypeOfNode.getUncached(), true);
@@ -85,7 +87,7 @@ public final class EqualsNode extends Node {
public boolean execute(
VirtualFrame frame, @AcceptsError Object self, @AcceptsError Object other) {
var areEqual = node.execute(frame, self, other);
-    if (!areEqual) {
+    if (!equalsProfile.profile(areEqual)) {
var selfType = types.execute(self);
var otherType = types.execute(other);
if (selfType != otherType) {

```
But that did not resolve the issue.

My second attempt was to enable splitting for `EqualsSimpleNode` with `@com.oracle.truffle.api.dsl.ReportPolymorphism` annotation, which seems to resolve the issue. The benchmark is back to its original score, and `EqualsSimpleNode` is no longer POLYMORPHIC.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@4e6 4e6 4e6 approved these changes

@Akirathan Akirathan Akirathan approved these changes

@GregoryTravis GregoryTravis GregoryTravis approved these changes

@jdunkerley jdunkerley Awaiting requested review from jdunkerley jdunkerley is a code owner

@radeusgd radeusgd Awaiting requested review from radeusgd radeusgd is a code owner

@AdRiley AdRiley Awaiting requested review from AdRiley AdRiley is a code owner

@hubertp hubertp Awaiting requested review from hubertp hubertp is a code owner

Assignees

@JaroslavTulach JaroslavTulach

Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: No changelog needed Do not require a changelog entry for this PR.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

(1.0 . to BigDecimal) != 1.0 - e.g. equality isn't working correctly
4 participants
@JaroslavTulach @4e6 @Akirathan @GregoryTravis