Speedup DataflowError.withDefaultTrace #11153

Akirathan · 2024-09-23T11:54:51Z

Fixes #10702

Pull Request Description

Improves the speed of ExecutionEnvironment.hasContextEnabled.

Important Notes

Local speedup of Map_Error_Benchmark_Vector_ignore.Map_Id_All_Errors benchmark is roughly ???.

Checklist

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

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
Unit tests have been written where possible.

JaroslavTulach

Consider @TruffleBoundary. If you want to keep BranchProfile approach, then it might be better to use the nodes approach - e.g. replace the withDefaultTrace method by class DataflowError.WithDefaultTraceNode and keep the profile hidden in it.

Anyway you are building on top of

Move execution environment from State to EnsoContext #11075

and that code is clearly wrong. I like the speed up, but maybe we should fix the broken code first and only then measure the speed.

engine/runtime/src/main/java/org/enso/interpreter/runtime/error/DataflowError.java

Akirathan · 2024-09-25T09:25:05Z

Waiting for @4e6.

JaroslavTulach

I am afraid right now this PR isn't ready to be merged. Moreover it needs to be adjusted to changes by Dmitry.

Akirathan · 2024-10-14T13:57:02Z

#11153 (comment) : This line fixes the problem? That is surprising

You are right. My initial commit is clearly wrong. I will revert it, rebase on latest develop, and start over.

mergify · 2024-10-14T15:38:54Z

⚠️ The sha of the head commit of this PR conflicts with #11321. Mergify cannot evaluate rules on this PR. ⚠️

mergify · 2024-10-14T16:03:22Z

⚠️ The sha of the head commit of this PR conflicts with #11321. Mergify cannot evaluate rules on this PR. ⚠️

mergify · 2024-10-14T16:05:09Z

⚠️ The sha of the head commit of this PR conflicts with #11321. Mergify cannot evaluate rules on this PR. ⚠️

Akirathan · 2024-10-16T11:29:38Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/error/DataflowError.java

+    var dataflowStacktraceCtx = ensoCtx.getBuiltins().context().getDataflowStackTrace();
+    // dataflowStacktraceCtx is a compiler constant, because it is a constructor of a builtin
+    // type.
+    CompilerAsserts.partialEvaluationConstant(dataflowStacktraceCtx);


This helps with the performance. The compiler is most probably not able to determine that dataflowStacktraceCtx is actually a constant (reference to a constructor of a builtin type).

This helps with the performance.

That would be surprising. It is a CompilerAsserts - e.g. not a directive, but only an assert that something is true. E.g. either dataflowStacktraceCtx is constant or the compilation is going to fail. The fact whether it is constant or not isn't changed by the assert. The fact whether compilation goes or not is influenced by the assert.

Akirathan · 2024-10-16T11:32:31Z

With the changes in 243847c I managed to get the performance of Map_Error_Benchmark_Vector_ignore.Map_Id_All_Errors from 1.179 ± 0.648 to 0.981 ± 0.073 (note that the standard deviation is also much lower, which is a good thing as it suggests the score is more reliable). Since the execution environment itself cannot be treated as a constant, I'd say this is a reasonable gain for such a small modification.

This reverts commit baeddd0.

JaroslavTulach

The use of Permissions seems to go the right way.

engine/runtime/src/main/java/org/enso/interpreter/runtime/state/ExecutionEnvironment.java

JaroslavTulach · 2024-10-16T12:32:45Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/error/DataflowError.java

+    var dataflowStacktraceCtx = ensoCtx.getBuiltins().context().getDataflowStackTrace();
+    // dataflowStacktraceCtx is a compiler constant, because it is a constructor of a builtin
+    // type.
+    CompilerAsserts.partialEvaluationConstant(dataflowStacktraceCtx);


This helps with the performance.

That would be surprising. It is a CompilerAsserts - e.g. not a directive, but only an assert that something is true. E.g. either dataflowStacktraceCtx is constant or the compilation is going to fail. The fact whether it is constant or not isn't changed by the assert. The fact whether compilation goes or not is influenced by the assert.

engine/runtime/src/main/java/org/enso/interpreter/runtime/state/ExecutionEnvironment.java

The use of Permissions seems to go the right way!

Akirathan · 2024-10-17T14:36:29Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/state/HasContextEnabledNode.java

+  boolean doIt(
+      ExecutionEnvironment executionEnvironment,
+      AtomConstructor runtimeCtxCtor,
+      @Cached("executionEnvironment") ExecutionEnvironment cachedEnv) {


With cached ExecutionEnvironment, the performance improved from 1.179 ± 0.648 to 0.218 ± 0.014.

engine/runtime/src/main/java/org/enso/interpreter/runtime/state/ExecutionEnvironment.java

JaroslavTulach

The idea of replacing array with record with three fields looks fine. If there is speed up, then we can probably integrate.

engine/runtime/src/main/java/org/enso/interpreter/runtime/state/ContextPermissions.java

engine/runtime/src/main/java/org/enso/interpreter/runtime/state/HasContextEnabledNode.java

…ression

Akirathan added the CI: No changelog needed Do not require a changelog entry for this PR. label Sep 23, 2024

Akirathan self-assigned this Sep 23, 2024

Akirathan requested review from 4e6, JaroslavTulach and hubertp as code owners September 23, 2024 11:54

JaroslavTulach approved these changes Sep 23, 2024

View reviewed changes

engine/runtime/src/main/java/org/enso/interpreter/runtime/error/DataflowError.java Outdated Show resolved Hide resolved

engine/runtime/src/main/java/org/enso/interpreter/runtime/error/DataflowError.java Show resolved Hide resolved

JaroslavTulach reviewed Sep 23, 2024

View reviewed changes

engine/runtime/src/main/java/org/enso/interpreter/runtime/error/DataflowError.java Outdated Show resolved Hide resolved

JaroslavTulach self-requested a review September 23, 2024 12:24

JaroslavTulach previously requested changes Oct 9, 2024

View reviewed changes

Akirathan force-pushed the wip/akirathan/10702-dataflowerr-stack-regression branch from baeddd0 to 5c1e9d1 Compare October 14, 2024 13:56

Akirathan mentioned this pull request Oct 15, 2024

stdlib benchmarks regression: DataflowError with stack #10702

Closed

Akirathan commented Oct 16, 2024

View reviewed changes

Akirathan added 4 commits October 16, 2024 13:35

DataflowError.withDefaultTrace has BranchProfile as parameter

482d93a

Revert "DataflowError.withDefaultTrace has BranchProfile as parameter"

b63021a

This reverts commit baeddd0.

Do not use hardcoded string

1e58228

Permissions in ExecutionEnvironment is a final record

243847c

Akirathan force-pushed the wip/akirathan/10702-dataflowerr-stack-regression branch from f113ce7 to 243847c Compare October 16, 2024 11:35

JaroslavTulach reviewed Oct 16, 2024

View reviewed changes

Akirathan added 3 commits October 17, 2024 14:15

hasContextEnabled and update exec env are nodes

dd25a6f

Remove unnecessary CompilerAssert.

3020fe8

Cache executionEnvironment in the node

6ccf304

Akirathan commented Oct 17, 2024

View reviewed changes

Akirathan added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Oct 18, 2024

JaroslavTulach reviewed Oct 21, 2024

View reviewed changes

engine/runtime/src/main/java/org/enso/interpreter/runtime/state/ExecutionEnvironment.java Outdated Show resolved Hide resolved

JaroslavTulach approved these changes Oct 21, 2024

View reviewed changes

engine/runtime/src/main/java/org/enso/interpreter/runtime/state/ContextPermissions.java Show resolved Hide resolved

JaroslavTulach reviewed Oct 21, 2024

View reviewed changes

engine/runtime/src/main/java/org/enso/interpreter/runtime/state/HasContextEnabledNode.java Show resolved Hide resolved

Akirathan added 3 commits October 21, 2024 16:39

Move execution context update functionality to WithContextNode

ee54f74

transfer to interpreter before string concatenation

d3f4c8c

FIx UnsupportedSpecializationException

be57ead

Akirathan added the CI: Ready to merge This PR is eligible for automatic merge label Oct 22, 2024

Akirathan added 2 commits October 22, 2024 17:42

Merge branch 'develop' into wip/akirathan/10702-dataflowerr-stack-reg…

a59af0a

…ression

Merge branch 'develop' into wip/akirathan/10702-dataflowerr-stack-reg…

07b918e

…ression

mergify bot merged commit c839309 into develop Oct 24, 2024
40 of 41 checks passed

mergify bot deleted the wip/akirathan/10702-dataflowerr-stack-regression branch October 24, 2024 03:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Speedup DataflowError.withDefaultTrace #11153

Speedup DataflowError.withDefaultTrace #11153

Akirathan commented Sep 23, 2024 •

edited

Loading

JaroslavTulach left a comment •

edited

Loading

Akirathan commented Sep 25, 2024

JaroslavTulach left a comment

Akirathan commented Oct 14, 2024

mergify bot commented Oct 14, 2024

mergify bot commented Oct 14, 2024

mergify bot commented Oct 14, 2024

Akirathan Oct 16, 2024

JaroslavTulach Oct 16, 2024

Akirathan commented Oct 16, 2024 •

edited

Loading

JaroslavTulach left a comment

JaroslavTulach Oct 16, 2024

Akirathan Oct 17, 2024

JaroslavTulach left a comment

Speedup DataflowError.withDefaultTrace #11153

Speedup DataflowError.withDefaultTrace #11153

Conversation

Akirathan commented Sep 23, 2024 • edited Loading

Pull Request Description

Important Notes

Checklist

JaroslavTulach left a comment • edited Loading

Choose a reason for hiding this comment

Akirathan commented Sep 25, 2024

JaroslavTulach left a comment

Choose a reason for hiding this comment

Akirathan commented Oct 14, 2024

mergify bot commented Oct 14, 2024

mergify bot commented Oct 14, 2024

mergify bot commented Oct 14, 2024

Akirathan Oct 16, 2024

Choose a reason for hiding this comment

JaroslavTulach Oct 16, 2024

Choose a reason for hiding this comment

Akirathan commented Oct 16, 2024 • edited Loading

JaroslavTulach left a comment

Choose a reason for hiding this comment

JaroslavTulach Oct 16, 2024

Choose a reason for hiding this comment

Akirathan Oct 17, 2024

Choose a reason for hiding this comment

JaroslavTulach left a comment

Choose a reason for hiding this comment

Akirathan commented Sep 23, 2024 •

edited

Loading

JaroslavTulach left a comment •

edited

Loading

Akirathan commented Oct 16, 2024 •

edited

Loading