Introducing @BuiltinMethod.inlineable and InlineableNode #6442

JaroslavTulach · 2023-04-26T16:48:08Z

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 DirectCallNode. Introduces @BuiltinMethod.inlineable - 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.

Checklist

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

All code follows the
Java,
All code has been tested:
- Unit tests have been written where possible.
- Benchmarks are good on GraalVM CE

...e/interpreter-dsl-test/src/test/java/org/enso/interpreter/dsl/test/InliningBuiltinsTest.java

JaroslavTulach · 2023-04-27T08:09:18Z

engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/BuiltinRootNode.java

-   * @param <E> extra data to keep in the node
-   * @param <N> node to delegate to from {@link #call(java.lang.Object...)} method
-   */
-  protected abstract static class InlinedCallNode<E, N extends Node> extends DirectCallNode {


We've been warned (in addition to the javadoc warning), that subclassing DirectCallNode creates unnecessary polymorphism when calling DirectCallNode.call. Such a polymorphism isn't a problem for "PE mode" (the Node being invoke is always a compilation constant, so it is clear what call is going to be invoked), but before the JVM gets there, it slows things down (Node isn't compilation constant for the regular JVM).

Replacing with a parallel InlineableNode concept.

JaroslavTulach · 2023-04-27T08:12:31Z

engine/runtime/src/test/scala/org/enso/interpreter/test/semantic/EvalTest.scala

+           |
+           |main =
+           |    x = "Hello World!"
+           |    Debug.eval <| $rawTQ


The combination of Debug.eval and <| was always broken. The <| builtin was providing its own VirtualFrame to the Debug code. Now the <| builtin specifies needsFrame = false and gets the caller's frame instance. As such it provides the right frame to the Debug subsystem.

engine/runtime/src/main/java/org/enso/interpreter/node/callable/ExecuteCallNode.java

JaroslavTulach · 2023-04-27T08:22:33Z

Running benchmarks to check the effect of the changes. Good, the ifBench6In is now sped up even on GraalVM CE:

IfVsCaseBenchmarks.ifBench6In   21.32783      1.29492    -93.92852%

Let's perform one more benchmark run with the most recent state to verify the results. Alas, there seems to be a slowdown in

ArrayProxyBenchmarks.sumOverVectorBackedByDelegatingProxy    0.36719      0.97045         +164.28833%

I am going to ignore this 164% decrease result. I am running benchmarks since the morning, both on develop as well as on my branch and the results oscillate from 220ms/op to 400ms/op on both branches. Just right now I got:

Result "org.enso.interpreter.bench.benchmarks.semantic.ArrayProxyBenchmarks.sumOverVectorBackedByDelegatingProxy":
  0.224 ±(99.9%) 0.014 ms/op [Average]
  (min, avg, max) = (0.218, 0.224, 0.228), stdev = 0.004
  CI (99.9%): [0.210, 0.238] (assumes normal distribution)

which is close to the best result I've ever seen. This result was achieved on 162048a - that's a signal this branch is doing fine, I think.

Akirathan · 2023-04-27T09:53:28Z

build.sbt

When running sbt:interpreter-dsl-test/test on a clean build, I get multiple warnings of type:

[warn] Could not determine source for class org.enso.interpreter.dsl.test.InliningBuiltinsInMethodGen

Moreover, In IntelliJ, I seem to be unable to locate generated sources, e.g., InliningBuiltinsInMethodGen, which results in poor inspection of InliningBuiltinsTest.java. @hubertp shouldn't there be an explicit setting for sourceManaged directory for interpreter-dsl-test project? I guess we should get rid of all the warnings even in tests.

Akirathan

Some minor nitpicks. I would really like to see the needsFrame renamed to forceInlining - see my comments in the code. I find the needsFrame name perplexing. Apart from that seems very impressive, I can't wait to see the benchmark results.

engine/runtime/src/main/java/org/enso/interpreter/node/InlineableNode.java

engine/runtime/src/main/java/org/enso/interpreter/node/callable/ExecuteCallNode.java

lib/scala/interpreter-dsl/src/main/java/org/enso/interpreter/dsl/BuiltinMethod.java

...ava/org/enso/interpreter/node/expression/builtin/runtime/RuntimeWithDisabledContextNode.java

...src/main/java/org/enso/interpreter/node/expression/builtin/function/ApplicationOperator.java

…GraalVMCE_6416

hubertp

minor nits

...time/src/bench/java/org/enso/interpreter/bench/benchmarks/semantic/ArrayProxyBenchmarks.java

engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/bool/AndNode.java

lib/scala/interpreter-dsl/src/main/java/org/enso/interpreter/dsl/Builtin.java

…GraalVMCE_6416

* develop: (34 commits) Continued Execution Context work and some little fixes (#6506) IDE's logging to a file (#6478) Fix application config (#6513) Cloud/desktop mode switcher (#6448) Fix doubled named arguments bug (#6422) Reimplement `enso_project` as a proper builtin (#6352) Fix layer ordering between dropdown and breadcrumbs backgrounds. (#6483) Multiflavor layers (#6477) DataflowAnalysis preserves dependencies order (#6493) Implement `create_database_table` for Database Table (#6467) Limit Dead Letter logging (#6482) More reliable shutdown of the EnsoContext to save resources (#6468) Make execution mode `live` default for CLI (#6496) Finishing Vector Editor (#6470) Proper handling of multiple list views. (#6461) Fix disappearing cached shapes (#6458) Add Execution Context control to Text.write (#6459) Change defaults for `Connection.tables` and ensure that `Connection.query` recognizes all available tables (#6443) Introducing @BuiltinMethod.needsFrame and InlineableNode (#6442) Hide profile button behind a feature flag (#6430) ...

Introducing @BuildinMethod.needsFrame that can overide the default

b169e49

JaroslavTulach added CI: No changelog needed Do not require a changelog entry for this PR. --low-performance labels Apr 26, 2023

JaroslavTulach added this to the Design Partners milestone Apr 26, 2023

JaroslavTulach requested review from kustosz, radeusgd and Akirathan April 26, 2023 16:48

JaroslavTulach requested a review from 4e6 as a code owner April 26, 2023 16:48

JaroslavTulach self-assigned this Apr 26, 2023

JaroslavTulach requested a review from hubertp as a code owner April 26, 2023 16:48

enso-bot bot mentioned this pull request Apr 27, 2023

Improve inlining of <| on GraalVM CE #6416

Closed

JaroslavTulach added 2 commits April 27, 2023 06:49

Propagating frame via more nodes

00e70fe

InlineableNode no longer extends DirectCallNode

9b4a466

JaroslavTulach commented Apr 27, 2023

View reviewed changes

...e/interpreter-dsl-test/src/test/java/org/enso/interpreter/dsl/test/InliningBuiltinsTest.java Show resolved Hide resolved

JaroslavTulach commented Apr 27, 2023

View reviewed changes

engine/runtime/src/main/java/org/enso/interpreter/node/callable/ExecuteCallNode.java Show resolved Hide resolved

JaroslavTulach changed the title ~~Introducing @BuiltinMethod.needsFrame that can overide the default~~ Introducing @BuiltinMethod.needsFrame and InlineableNode Apr 27, 2023

Necessary changes in annotation processor

ea96ab1

JaroslavTulach added the -compiler label Apr 27, 2023

JaroslavTulach added 2 commits April 27, 2023 11:15

Include interpreter-dsl-test project in the aggregates

59e2eba

javafmtAll on the new interpreter-dsl-test files

0ee6900

Akirathan reviewed Apr 27, 2023

View reviewed changes

Akirathan approved these changes Apr 27, 2023

View reviewed changes

JaroslavTulach added 5 commits April 27, 2023 14:56

inlineable attribute

c1c81a4

Better documentation of callInlineable specialization

81102bd

Cannot compile without allowUncached = true

737a7d2

Use SrcUtil.source consistently

e1d1429

Making up-to-date with develop branch

162048a

Merge remote-tracking branch 'origin/develop' into wip/jtulach/FastOn…

26db84d

…GraalVMCE_6416

hubertp approved these changes Apr 28, 2023

View reviewed changes

...time/src/bench/java/org/enso/interpreter/bench/benchmarks/semantic/ArrayProxyBenchmarks.java Outdated Show resolved Hide resolved

engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/bool/AndNode.java Show resolved Hide resolved

JaroslavTulach added 2 commits April 28, 2023 12:28

Give the node class a name

28e7fb4

callIndirect replaces callDirect as well as callInlineable

5297ffb

JaroslavTulach commented Apr 28, 2023

View reviewed changes

lib/scala/interpreter-dsl/src/main/java/org/enso/interpreter/dsl/Builtin.java Show resolved Hide resolved

Unifying into SrcUtil.findName

61af2a5

JaroslavTulach added CI: Ready to merge This PR is eligible for automatic merge CI: Clean build required CI runners will be cleaned before and after this PR is built. labels Apr 28, 2023

Merge remote-tracking branch 'origin/develop' into wip/jtulach/FastOn…

f18bbf0

…GraalVMCE_6416

JaroslavTulach removed the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Apr 28, 2023

mergify bot merged commit efe904c into develop Apr 28, 2023

mergify bot deleted the wip/jtulach/FastOnGraalVMCE_6416 branch April 28, 2023 15:32

JaroslavTulach changed the title ~~Introducing @BuiltinMethod.needsFrame and InlineableNode~~ Introducing @BuiltinMethod.inlineable and InlineableNode Apr 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introducing @BuiltinMethod.inlineable and InlineableNode #6442

Introducing @BuiltinMethod.inlineable and InlineableNode #6442

JaroslavTulach commented Apr 26, 2023 •

edited

Loading

JaroslavTulach Apr 27, 2023 •

edited

Loading

JaroslavTulach Apr 27, 2023

JaroslavTulach commented Apr 27, 2023 •

edited

Loading

Akirathan Apr 27, 2023

Akirathan left a comment

hubertp left a comment

Introducing @BuiltinMethod.inlineable and InlineableNode #6442

Introducing @BuiltinMethod.inlineable and InlineableNode #6442

Conversation

JaroslavTulach commented Apr 26, 2023 • edited Loading

Pull Request Description

Checklist

JaroslavTulach Apr 27, 2023 • edited Loading

Choose a reason for hiding this comment

JaroslavTulach Apr 27, 2023

Choose a reason for hiding this comment

JaroslavTulach commented Apr 27, 2023 • edited Loading

Akirathan Apr 27, 2023

Choose a reason for hiding this comment

Akirathan left a comment

Choose a reason for hiding this comment

hubertp left a comment

Choose a reason for hiding this comment

JaroslavTulach commented Apr 26, 2023 •

edited

Loading

JaroslavTulach Apr 27, 2023 •

edited

Loading

JaroslavTulach commented Apr 27, 2023 •

edited

Loading