Convert Array_Like_Helpers.map to a builtin to reduce stack size
#11329
Assignees
Labels
Comments
Experiment to collect the biggest (Truffle) stack during execution of Base_TestsApply the following patch: diff --git a/engine/runtime/src/main/java/org/enso/interpreter/node/ClosureRootNode.java b/engine/runtime/src/main/java/org/enso/interpreter/node/ClosureRootNode.java
index f2ac6c965..bcac8eeab 100644
--- a/engine/runtime/src/main/java/org/enso/interpreter/node/ClosureRootNode.java
+++ b/engine/runtime/src/main/java/org/enso/interpreter/node/ClosureRootNode.java
@@ -1,10 +1,13 @@
package org.enso.interpreter.node;
import com.oracle.truffle.api.CompilerDirectives;
+import com.oracle.truffle.api.Truffle;
import com.oracle.truffle.api.dsl.ReportPolymorphism;
import com.oracle.truffle.api.frame.VirtualFrame;
import com.oracle.truffle.api.nodes.NodeInfo;
import com.oracle.truffle.api.source.SourceSection;
+import java.util.ArrayList;
+import java.util.List;
import org.enso.compiler.context.LocalScope;
import org.enso.interpreter.EnsoLanguage;
import org.enso.interpreter.runtime.scope.ModuleScope;
@@ -71,6 +74,10 @@ public class ClosureRootNode extends EnsoRootNode {
usedInBinding);
}
+ private static List<String> biggestTruffleStackTrace = List.of();
+ private static int jStackSize;
+ private static boolean shutdownHookRegistered;
+
/**
* Executes the node.
*
@@ -82,9 +89,51 @@ public class ClosureRootNode extends EnsoRootNode {
if (CompilerDirectives.inCompilationRoot() || CompilerDirectives.inInterpreter()) {
com.oracle.truffle.api.TruffleSafepoint.poll(this);
}
+ if (!shutdownHookRegistered) {
+ Runtime.getRuntime().addShutdownHook(new Thread(() -> {
+ System.out.printf("[mylog] biggestStackTrace [java stack size = %d][truffle stack size = %d]:%n",
+ jStackSize,
+ biggestTruffleStackTrace.size());
+ biggestTruffleStackTrace.forEach(stackFrame -> System.out.println(" " + stackFrame));
+ }));
+ shutdownHookRegistered = true;
+ }
+
+ var truffleStack = collectTruffleStackTrace();
+ if (truffleStack.size() > biggestTruffleStackTrace.size()) {
+ biggestTruffleStackTrace = truffleStack;
+ jStackSize = Thread.currentThread().getStackTrace().length;
+ }
return body.executeGeneric(frame);
}
+ private List<String> collectTruffleStackTrace() {
+ List<String> stackTrace = new ArrayList<>();
+ Truffle.getRuntime().iterateFrames((frameInstance) -> {
+ var callNode = frameInstance.getCallNode();
+ if (callNode != null) {
+ var rootNode = callNode.getRootNode();
+ if (rootNode != null) {
+ var nodeName = rootNode.getName();
+ if (rootNode.getSourceSection() != null) {
+ nodeName += " (" +
+ rootNode.getSourceSection().getSource().getName() +
+ "[" +
+ rootNode.getSourceSection().getStartLine() +
+ ":" +
+ rootNode.getSourceSection().getStartColumn() +
+ "])";
+ }
+ stackTrace.add(nodeName);
+ return null;
+ }
+ }
+ stackTrace.add("<unknown>");
+ return null;
+ });
+ return stackTrace;
+ }
+
final ExpressionNode getBody() {
return body;
}And run Output: We can see that there are a lot of frames related to |
|
I was just profiling one of the "sieve benchmarks" (as introduced by #11351): and this is the beginning of the compilation:
|
4 tasks
|
Pavel Marek reports a new STANDUP for today (2024-10-18): Progress: - Experimenting with a regression test for stack trace sizes - #11363 (comment)
|
|
There is an option to
use overloaded methods: public int minus(int a, int b) {
return a-b;
}
public int minus(int a) {
return minus(a, 5);
}when compiled with minus(b=7, a=3)
minus(a=7)that provides (limited) support for default arguments. Things will get trickier with the increased number of arguments, but when one doesn't need all combination, but only few - as is the case with PS: You are allowed to modify Scala source to keep the number of needed overloads up to three. If the Scala code continues to compile against Scala PPS: Probably better to explicitly spell the default values and work with overloads than generate Scala code... |
4 tasks
|
Pavel Marek reports a new STANDUP for today (2024-10-21): Progress: - Started converting
|
|
Pavel Marek reports a new STANDUP for today (2024-10-22): Progress: - Managed to reduce the stack size for every
|
|
Please more |
|
Pavel Marek reports a new STANDUP for yesterday (2024-10-23): Progress: - Reduced Java stack frames for each
|
|
Pavel Marek reports a new STANDUP for today (2024-10-24): Progress: - Fixing tests
|
|
Pavel Marek reports a new STANDUP for today (2024-10-25): Progress: - Still fixing JVM, stdlib and native image tests. It should be finished by 2024-10-29. |
|
Pavel Marek reports a new STANDUP for today (2024-10-28): Progress: - Tests fixed, benchmarks scheduled #11363 (comment).
|
|
Pavel Marek reports a new STANDUP for today (2024-10-29): Progress: - When checking benchmark regressions locally, bumped into #11437
|
|
Pavel Marek reports a new STANDUP for today (2024-10-30): Progress: - Created some issues for unstable or failing benchmarks.
|
|
Pavel Marek reports a new STANDUP for today (2024-11-04): Progress: - Review #11399
|
|
Pavel Marek reports a new STANDUP for today (2024-11-05): Progress: - Ensure that there is at most one
|
github-project-automation
bot
moved this from 👁️ Code review
to 🟢 Accepted
in Issues Board
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
@jdunkerley complained about strange stacktraces when invoking
.map:The stacktraces were much simpler prior to https://github.com/enso-org/enso/pull/8307/files#diff-c5e24473d47930ce7b6fd3f7f643967fcb54fcb01df1e6803438203749988f70R39 when all the logic was just a simple builtin. Now, when there is bunch of wrapper functions in Enso, we see them in the stack. The following program yields an exception:
and the stacktrace is quite deep:
Goal: rewrite the logic into a builtin and make the call appear as a single element on the stack - e.g. the above stack should compose of only three lines:
Btw. such a stack compression should also help with @AdRiley problem of too deep stacktraces as one
.mapinvocation is more than hundred Java stack frames in the interpreter right now:By having
.mapas a builtin we lower the stack requirements a lot given how frequently we are using.map& co.The text was updated successfully, but these errors were encountered: