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

WarningBenchmarks 5000% regressions on 2025-02-11 #12258

Closed
Akirathan opened this issue Feb 11, 2025 · 5 comments · Fixed by #12388
Closed

WarningBenchmarks 5000% regressions on 2025-02-11 #12258

Akirathan opened this issue Feb 11, 2025 · 5 comments · Fixed by #12388
Assignees
Labels
--low-performance --regression Important: regression -compiler p-high Should be completed in the next sprint

Comments

@Akirathan
Copy link
Member

Akirathan commented Feb 11, 2025

+5000% regressions in WarningBenchmarks on 2025-02-11 caused by warnings.hasWarnings call:

Image

Image

To run single benchmark:

sbt:runtime-benchmarks> benchOnly org.enso.interpreter.bench.benchmarks.semantic.WarningBenchmarks.sameWarningVecSum
@Akirathan Akirathan added --low-performance --regression Important: regression -compiler p-high Should be completed in the next sprint labels Feb 11, 2025
@JaroslavTulach JaroslavTulach changed the title Engine benchmark regressions on 2025-02-11 WarningBenchmarks 5000% regressions on 2025-02-11 Feb 11, 2025
@JaroslavTulach
Copy link
Member

JaroslavTulach commented Feb 11, 2025

Running

enso/tools/performance/engine-benchmarks$ ./bench_download.py -s engine -b develop wip/jtulach/BuiltinArgNode11827

shows that benchmarks were fine and even faster, but regressed with one of the last commits in the PR:

WarningBenchmarks_sameWarningVecSum

The culprit must be between

enso$ git log f211c19963357211c65597da92c8f910f940371f...e6af654e514dda263fa1dd4c2c7c015874b1b67b

There is a bunch of commits and a merge in the range, so bisecting will be needed.

@JaroslavTulach
Copy link
Member

JaroslavTulach commented Feb 12, 2025

After bisecting:

e6af654e514dda263fa1dd4c2c7c015874b1b67b is the first bad commit
commit e6af654e514dda263fa1dd4c2c7c015874b1b67b
Author: Jaroslav Tulach <[email protected]>
Date:   Fri Feb 7 09:38:41 2025 +0100

    Let SliceArrayVectorNode handle warnings on its own

Let's see what e6af654 has done: It's warnings.hasWarnings.

@JaroslavTulach
Copy link
Member

An example of slowdown: check for hasWarnings in LengthVectorMethodGen node:

VectorBenchmarks_averageOverSliceWrapped10
VectorBenchmarks_averageOverSliceWrapped100

obrazek

@JaroslavTulach
Copy link
Member

JaroslavTulach commented Feb 27, 2025

Good benchmarks at a829bdb run at this speed

[info] WarningBenchmarks.sameWarningVecSum                avgt    3   1,107 ±  0,021  ms/op

I can get to same or even better number:

[info] WarningBenchmarks.sameWarningVecSum  avgt    3  0,840 ± 0,008  ms/op

with the following patch:

diff --git a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/BuiltinRootNode.java b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/BuiltinRootNode.java
index 86eaa3997e..d873fdc6ad 100644
--- a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/BuiltinRootNode.java
+++ b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/BuiltinRootNode.java
@@ -22,6 +22,7 @@ import org.enso.interpreter.runtime.error.PanicException;
 import org.enso.interpreter.runtime.error.PanicSentinel;
 import org.enso.interpreter.runtime.warning.AppendWarningNode;
 import org.enso.interpreter.runtime.warning.WarningsLibrary;
+import org.enso.interpreter.runtime.warning.WithWarnings;
 import org.enso.pkg.QualifiedName;
 
 /** Root node for use by all the builtin functions. */
@@ -137,7 +138,7 @@ public abstract class BuiltinRootNode extends RootNode {
         throw sentinel;
       }
       if (warnings != null) {
-        if (warnings.hasWarnings(value)) {
+        if (value instanceof WithWarnings) {
           if (mapInsertAllNode == null) {
             CompilerDirectives.transferToInterpreterAndInvalidate();
             this.mapInsertAllNode = insert(HashMapInsertAllNode.build());
@@ -148,6 +149,13 @@ public abstract class BuiltinRootNode extends RootNode {
           } catch (UnsupportedMessageException ex) {
             throw raise(RuntimeException.class, ex);
           }
+        } else {
+          /*
+          if (warnings.hasWarnings(value)) {
+            CompilerDirectives.transferToInterpreter();
+            new Exception("Special warnings for value: " + value).printStackTrace();
+          }
+          */
         }
       }
       if (is(REQUIRES_CAST) && type != Object.class) {
diff --git a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/immutable/AtVectorNode.java b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/immutable/AtVectorNode.java
index 73519af7a1..6c58c2b733 100644
--- a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/immutable/AtVectorNode.java
+++ b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/immutable/AtVectorNode.java
@@ -3,6 +3,7 @@ package org.enso.interpreter.node.expression.builtin.immutable;
 import com.oracle.truffle.api.CompilerDirectives;
 import com.oracle.truffle.api.interop.InvalidArrayIndexException;
 import com.oracle.truffle.api.nodes.Node;
+import org.enso.interpreter.dsl.AcceptsWarning;
 import org.enso.interpreter.dsl.BuiltinMethod;
 import org.enso.interpreter.runtime.EnsoContext;
 import org.enso.interpreter.runtime.data.vector.ArrayLikeAtNode;
@@ -18,7 +19,7 @@ public class AtVectorNode extends Node {
   private @Child ArrayLikeAtNode at = ArrayLikeAtNode.create();
   private @Child ArrayLikeLengthNode length;
 
-  Object execute(Object arrayLike, long index) {
+  Object execute(@AcceptsWarning Object arrayLike, @AcceptsWarning long index) {
     try {
       long actualIndex = index < 0 ? index + len(arrayLike) : index;
       return at.executeAt(arrayLike, actualIndex);
diff --git a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/immutable/LengthVectorNode.java b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/immutable/LengthVectorNode.java
index 29c65fea60..839dbf277f 100644
--- a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/immutable/LengthVectorNode.java
+++ b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/immutable/LengthVectorNode.java
@@ -1,6 +1,7 @@
 package org.enso.interpreter.node.expression.builtin.immutable;
 
 import com.oracle.truffle.api.nodes.Node;
+import org.enso.interpreter.dsl.AcceptsWarning;
 import org.enso.interpreter.dsl.BuiltinMethod;
 import org.enso.interpreter.runtime.data.vector.ArrayLikeLengthNode;
 
@@ -11,7 +12,7 @@ import org.enso.interpreter.runtime.data.vector.ArrayLikeLengthNode;
 public class LengthVectorNode extends Node {
   @Child ArrayLikeLengthNode length = ArrayLikeLengthNode.create();
 
-  long execute(Object arrayLike) {
+  long execute(@AcceptsWarning Object arrayLike) {
     return length.executeLength(arrayLike);
   }
 }
diff --git a/engine/runtime/src/main/java/org/enso/interpreter/runtime/data/vector/ArrayLikeAtNode.java b/engine/runtime/src/main/java/org/enso/interpreter/runtime/data/vector/ArrayLikeAtNode.java
index 37ed33437d..7b36d923a6 100644
--- a/engine/runtime/src/main/java/org/enso/interpreter/runtime/data/vector/ArrayLikeAtNode.java
+++ b/engine/runtime/src/main/java/org/enso/interpreter/runtime/data/vector/ArrayLikeAtNode.java
@@ -10,13 +10,15 @@ import com.oracle.truffle.api.interop.InvalidArrayIndexException;
 import com.oracle.truffle.api.interop.UnsupportedMessageException;
 import com.oracle.truffle.api.library.CachedLibrary;
 import com.oracle.truffle.api.nodes.Node;
+import org.enso.interpreter.dsl.AcceptsWarning;
 import org.enso.interpreter.node.expression.foreign.HostValueToEnsoNode;
 import org.enso.interpreter.runtime.warning.AppendWarningNode;
 import org.enso.interpreter.runtime.warning.WarningsLibrary;
 
 @GenerateUncached
 public abstract class ArrayLikeAtNode extends Node {
-  public abstract Object executeAt(Object arrayLike, long index) throws InvalidArrayIndexException;
+  public abstract Object executeAt(@AcceptsWarning Object arrayLike, @AcceptsWarning long index)
+      throws InvalidArrayIndexException;
 
   @NeverDefault
   public static ArrayLikeAtNode create() {

@enso-bot
Copy link

enso-bot bot commented Mar 3, 2025

Jaroslav Tulach reports a new STANDUP for yesterday (2025-03-02):

Progress: .

@github-project-automation github-project-automation bot moved this from ⚙️ Design to 🟢 Accepted in Issues Board Mar 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
--low-performance --regression Important: regression -compiler p-high Should be completed in the next sprint
Projects
Status: 🟢 Accepted
Development

Successfully merging a pull request may close this issue.

2 participants