Removing State from method signatures #12233

JaroslavTulach · 2025-02-05T08:06:52Z

Pull Request Description

Fixes #7117 by removing State from method signatures and moving it into Truffle thread context local variable:

there is a massive refactoring of (mostly) builtins signatures - just ignore it
there is a Enso test demonstrating State and Contexts are not preserved through the Polyglot boundary #7117 is fixed
there is JUnit test demonstrating that two threads have an independent State
then there is the implementation
- EnsoContext & EnsoLanguage hold the state
- Function.ArgumentsHelper no longer passes/uses State

Everything compiles. Tests pass after fixing one of them. Benchmarks shall be OK soon.

Checklist

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

All code follows the
Scala,
Java,
Unit tests continue to pass
Benchmarks are fine
- 1st (slow) run, 2nd run, 3rd run. New round of runs after 42e2337 starts.
- stdlibs run

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

JaroslavTulach · 2025-02-06T04:32:08Z

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

@@ -74,7 +75,7 @@ public static DataflowError withDefaultTrace(

  /** Slow version of {@link #withDefaultTrace(State, Object, Node, HasContextEnabledNode)}. */
  public static DataflowError withDefaultTrace(Object payload, Node location) {
-    return withDefaultTrace(null, payload, location, HasContextEnabledNode.getUncached());
+    return withDefaultTrace(payload, location, HasContextEnabledNode.getUncached());


The removal of this null is causing the change in test behavior:

DataflowError.withDefaultTrace() (DataflowError.java:65) DataflowError.withDefaultTrace() (DataflowError.java:78) DivNode.doLong() (DivNode.java:29)

Previously the withDefaultTrace(Object payload, Node location) was actually always attaching the full stack trace. That wasn't correct and we need to update the test: 952125c

…nContext7115

JaroslavTulach · 2025-02-06T06:52:45Z

1st run of engine benchmarks show some regressions:

Possibly in caseBench6 benchmark:

Multi value benchmark:

Also in RecursionBenchmarks:

VectorBenchmarks:

Some of them are addressed by 4ef8f4d, but many remain regressed.

test/Base_Tests/src/Runtime/State_Spec.enso

...e/runtime-instrument-common/src/main/java/org/enso/interpreter/service/ExecutionService.java

...gger/src/main/java/org/enso/interpreter/instrument/repl/debugger/ReplDebuggerInstrument.java

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

...gger/src/main/java/org/enso/interpreter/instrument/repl/debugger/ReplDebuggerInstrument.java

JaroslavTulach · 2025-02-12T09:59:42Z

Some benchmarks still show 2-3x regression:

AtomBenchmarks_benchMapReverseListCurry
IfVsCaseBenchmarks_caseBench6
MultiValueBenchmarks_sumOverComplexAndFloat5
MultiValueBenchmarks_sumOverFloatAndComplex6
MultiValueBenchmarks_sumOverFloatComplexRecastedToFloat4
RecursionBenchmarks_benchNestedThunkSum
RecursionBenchmarks_benchSumStateTCO
WarningBenchmarks_noWarningsVecSum
WarningBenchmarks_randomElementsVecSum

VectorBenchmarks_averageOverArray
VectorBenchmarks_averageOverArrayProxy
VectorBenchmarks_averageOverArrayProxyNew
VectorBenchmarks_averageOverPolyglotVector
VectorBenchmarks_averageOverSliceWrapped10
VectorBenchmarks_averageOverSliceWrapped100

something has to be done to speed them up.

JaroslavTulach · 2025-02-12T17:15:53Z

Running engine bechmarks again

JaroslavTulach · 2025-02-13T05:02:32Z

Here is some difference in boxing:

The previous analysis is more related to hasWarnings problem: #12258 (comment)

JaroslavTulach · 2025-02-13T15:23:03Z

There are now AllocatedObject nodes in the graph. They aren't there on the develop branch.

Obviously allocating an object is costly. I am not yet sure what is causing such allocation.

Update: with 42e2337 the VectorBenchmarks.averageOverSliceWrapped100 benchmark seems to be back at full speed. New engine benchmarks run scheduled.

JaroslavTulach · 2025-02-14T04:12:08Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/callable/function/Function.java

-     *     ArgumentsHelper#buildArguments(Function,CallerInfo, Object, Object[])}
-     * @return the state for the function
-     */
-    public static State getState(Object[] arguments) {


State is no longer passed as a VirtualFrame.getArguments() when a Function is invoked.

JaroslavTulach · 2025-02-14T04:26:56Z

VectorBenchmarks_averageOverArray
VectorBenchmarks_averageOverArrayProxy
VectorBenchmarks_averageOverArrayProxyNew
VectorBenchmarks_averageOverPolyglotVector
VectorBenchmarks_averageOverSliceWrapped10
VectorBenchmarks_averageOverSliceWrapped100

something has to be done to speed them up.

With 42e2337 these benchmarks are on par with develop:

[info] Benchmark                                    Mode  Cnt  Score    Error  Units
[info] VectorBenchmarks.averageAbstractList         avgt    5  0.054 ±  0.001  ms/op
[info] VectorBenchmarks.averageOverArray            avgt    5  0.001 ±  0.001  ms/op
[info] VectorBenchmarks.averageOverArrayProxy       avgt    5  0.001 ±  0.001  ms/op
[info] VectorBenchmarks.averageOverArrayProxyNew    avgt    5  0.001 ±  0.001  ms/op
[info] VectorBenchmarks.averageOverPolyglotArray    avgt    5  0.001 ±  0.001  ms/op
[info] VectorBenchmarks.averageOverPolyglotVector   avgt    5  0.001 ±  0.001  ms/op
[info] VectorBenchmarks.averageOverSlice            avgt    5  0.001 ±  0.001  ms/op
[info] VectorBenchmarks.averageOverSliceWrapped10   avgt    5  0.005 ±  0.001  ms/op
[info] VectorBenchmarks.averageOverSliceWrapped100  avgt    5  0.005 ±  0.001  ms/op
[info] VectorBenchmarks.averageOverVector           avgt    5  0.001 ±  0.001  ms/op

Remaining Regessions

StringBenchmarks_lengthOfStrings
RecursionBenchmarks_benchSumStateTCO
RecursionBenchmarks_benchNestedThunkSum

radeusgd · 2025-02-14T09:45:06Z

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

@@ -60,7 +60,8 @@ protected Object callInlineable(
      Object[] arguments,
      @Cached("function.getCallTarget()") RootCallTarget cachedTarget,
      @Cached("createInlineableNode(cachedTarget)") InlineableNode callNode) {
-    var args = Function.ArgumentsHelper.buildArguments(function, callerInfo, state, arguments);
+    var args =
+        Function.ArgumentsHelper.buildArguments(function, callerInfo, arguments); // XXX no state


What does this comment mean?

Removing State from method signatures

512b5a9

JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label Feb 5, 2025

JaroslavTulach self-assigned this Feb 5, 2025

JaroslavTulach requested review from 4e6, hubertp and Akirathan as code owners February 5, 2025 08:06

JaroslavTulach marked this pull request as draft February 5, 2025 08:07

JaroslavTulach added 3 commits February 5, 2025 13:04

Provide some non-null value of State

ef1fd5e

Test verifying state propagates thru Java

84eb0bb

Use the State from EnsoContext

22afe54

JaroslavTulach added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Feb 5, 2025

Make sure currentState() is local to the executing thread

86520e3

JaroslavTulach force-pushed the wip/jtulach/StateInContext7117 branch from 1231c92 to 86520e3 Compare February 5, 2025 15:45

JaroslavTulach marked this pull request as ready for review February 5, 2025 16:00

JaroslavTulach requested review from jdunkerley, radeusgd, GregoryTravis, AdRiley and marthasharkey as code owners February 5, 2025 16:00

JaroslavTulach commented Feb 5, 2025

View reviewed changes

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

enso-bot bot mentioned this pull request Feb 6, 2025

Dispatch of to_text, is_a & co. on intersection types #12192

Merged

2 tasks

JaroslavTulach commented Feb 6, 2025

View reviewed changes

JaroslavTulach added 2 commits February 6, 2025 05:40

DataflowError shouldn't provide stack by default

952125c

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

84da6c6

…nContext7115

Speculate on single State being used per EnsoContext

4ef8f4d

radeusgd reviewed Feb 6, 2025

View reviewed changes

test/Base_Tests/src/Runtime/State_Spec.enso Show resolved Hide resolved

hubertp approved these changes Feb 6, 2025

View reviewed changes

JaroslavTulach added 2 commits February 6, 2025 13:23

State has been removed from the arguments

c8852c6

FunctionCall.state was removed

f4a4fec

Removing annotation processor support for State argument

078efef

Akirathan approved these changes Feb 6, 2025

View reviewed changes

...gger/src/main/java/org/enso/interpreter/instrument/repl/debugger/ReplDebuggerInstrument.java Outdated Show resolved Hide resolved

JaroslavTulach added 3 commits February 12, 2025 04:52

Merging with most recent develop

4902f6e

REPL debugger naturally inherits the state from EnsoContext

a62c3c0

Check propagation of environment via Java interop

800633c

Merging with latest develop

edf21ec

enso-bot bot mentioned this pull request Feb 13, 2025

Let EnsoMultiValue.to_text delegate to first type to_text #11827

Closed

Internal nodes keep passing State as a parameter

42e2337

JaroslavTulach requested a review from radeusgd February 14, 2025 04:05

JaroslavTulach commented Feb 14, 2025

View reviewed changes

radeusgd reviewed Feb 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removing State from method signatures #12233

Removing State from method signatures #12233

JaroslavTulach commented Feb 5, 2025 •

edited

Loading

JaroslavTulach Feb 6, 2025 •

edited

Loading

JaroslavTulach commented Feb 6, 2025 •

edited

Loading

JaroslavTulach commented Feb 12, 2025

JaroslavTulach commented Feb 12, 2025

JaroslavTulach commented Feb 13, 2025 •

edited

Loading

JaroslavTulach commented Feb 13, 2025 •

edited

Loading

JaroslavTulach Feb 14, 2025

JaroslavTulach commented Feb 14, 2025 •

edited

Loading

radeusgd Feb 14, 2025

Removing State from method signatures #12233

Are you sure you want to change the base?

Removing State from method signatures #12233

Conversation

JaroslavTulach commented Feb 5, 2025 • edited Loading

Pull Request Description

Checklist

JaroslavTulach Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

JaroslavTulach commented Feb 6, 2025 • edited Loading

JaroslavTulach commented Feb 12, 2025

JaroslavTulach commented Feb 12, 2025

JaroslavTulach commented Feb 13, 2025 • edited Loading

JaroslavTulach commented Feb 13, 2025 • edited Loading

JaroslavTulach Feb 14, 2025

Choose a reason for hiding this comment

JaroslavTulach commented Feb 14, 2025 • edited Loading

Remaining Regessions

radeusgd Feb 14, 2025

Choose a reason for hiding this comment

JaroslavTulach commented Feb 5, 2025 •

edited

Loading

JaroslavTulach Feb 6, 2025 •

edited

Loading

JaroslavTulach commented Feb 6, 2025 •

edited

Loading

JaroslavTulach commented Feb 13, 2025 •

edited

Loading

JaroslavTulach commented Feb 13, 2025 •

edited

Loading

JaroslavTulach commented Feb 14, 2025 •

edited

Loading