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

Improve inlining of <| on GraalVM CE #6416

Closed
JaroslavTulach opened this issue Apr 25, 2023 · 4 comments · Fixed by #6442
Closed

Improve inlining of <| on GraalVM CE #6416

JaroslavTulach opened this issue Apr 25, 2023 · 4 comments · Fixed by #6442

Comments

@JaroslavTulach
Copy link
Member

#6384 improved the speed of cascade of <| functions on GraalVM EE only. However we'd like to improve it for CE as well. Let's address that as a new issue.

The goal is to run:

sbt> runtime/benchOnly ifBench6

on GraalVM CE and make sure ifBench6 and ifBench6In are similarly fast.

@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Apr 25, 2023

--- a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/function/ApplicationOperator.java
+++ b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/function/ApplicationOperator.java
@@ -25,7 +25,7 @@
     invokeCallableNode.setTailStatus(BaseNode.TailStatus.TAIL_DIRECT);
   }
 
-  Object execute(VirtualFrame frame, State state, Object self, @Suspend Object argument) {
-    return invokeCallableNode.execute(self, frame, state, new Object[] {argument});
+  Object execute(State state, Object self, @Suspend Object argument) {
+    return invokeCallableNode.execute(self, null, state, new Object[] {argument});
   }
 }

this change is known to speed the benchmark up. I am just not sure whether it is correct from functional perspective. But sbt runtime/test and test/Tests tests run without any problems. CCing @kustosz

@enso-bot
Copy link

enso-bot bot commented Apr 27, 2023

Jaroslav Tulach reports a new STANDUP for yesterday (2023-04-26):

Progress: - @BuiltinMethod.needsFrame: #6442

Next Day: Polish needsFrame, process backlog

@JaroslavTulach JaroslavTulach moved this from ❓New to 👁️ Code review in Issues Board Apr 27, 2023
@enso-bot
Copy link

enso-bot bot commented Apr 28, 2023

Jaroslav Tulach reports a new STANDUP for yesterday (2023-04-27):

Progress: - Cleaning PR-6442 up: Rewriting to not subclass the DirectCallNode

Next Day: Investigate 160% slowdown in one of the benchmarks

GitHub
Pull Request Description Fixes #6416 by introducing InlineableNode. It runs fast even on GraalVM CE, fixes (forever broken) Debug.eval with <| and removes discouraged subclassing of DirectCallNo...

@mergify mergify bot closed this as completed in #6442 Apr 28, 2023
mergify bot pushed a commit that referenced this issue Apr 28, 2023
Fixes #6416 by introducing `InlineableNode`. It runs fast even on GraalVM CE, fixes ([forever broken](#6442 (comment))) `Debug.eval` with `<|` and [removes discouraged subclassing](#6442 (comment)) of `DirectCallNode`. Introduces `@BuiltinMethod.needsFrame` - something that was requested by #6293. Just in this PR the attribute is optional - its implicit value continues to be derived from `VirtualFrame` presence/absence in the builtin method argument list. A lot of methods had to be modified to pass the `VirtualFrame` parameter along to propagate it where needed.
@github-project-automation github-project-automation bot moved this from 👁️ Code review to 🟢 Accepted in Issues Board Apr 28, 2023
@enso-bot
Copy link

enso-bot bot commented Apr 28, 2023

Jaroslav Tulach reports a new STANDUP for today (2023-04-28):

Progress: - Fix for #6416 has been integrated #6442

Next Day: Bugfixing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

1 participant