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.

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

Enable splitting for EqualsSimpleNode #9268

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

Akirathan
Copy link
Member

@Akirathan Akirathan commented Mar 4, 2024

Fixes #9166

Pull Request Description

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
For some reason, EqualsSimpleNode is POLYMORPHIC. That seems to be the most visible performance problem.

First, I tried to introduce ConditionProfile with:

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.

Checklist

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

  • Run benchmarks and ensure that the regression is resolved, and no other introduced.

@Akirathan Akirathan added CI: No changelog needed Do not require a changelog entry for this PR. CI: Clean build required CI runners will be cleaned before and after this PR is built. labels Mar 4, 2024
@Akirathan Akirathan self-assigned this Mar 4, 2024
@Akirathan
Copy link
Member Author

Akirathan commented Mar 4, 2024

Stdlib benchmarks scheduled in https://github.com/enso-org/enso/actions/runs/8144579573

GitHub
Hybrid visual and textual functional programming. Contribute to enso-org/enso development by creating an account on GitHub.

@Akirathan
Copy link
Member Author

Collection.list_meta_fold is back to its original performance:
image

A similar case can be seen for many other benchmarks with "equality".

There are no regressions. Let's merge this PR.

@Akirathan Akirathan added the CI: Ready to merge This PR is eligible for automatic merge label Mar 5, 2024
Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

Great to hear the benchmarks are back to original speed. I still need to read about ReportPolymorphism.

@mergify mergify bot merged commit f02213a into develop Mar 5, 2024
29 of 32 checks passed
@mergify mergify bot deleted the wip/akirathan/9166-list-meta-regression branch March 5, 2024 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10x slowdown in Collections_list_meta_fold
2 participants