Move execution environment from State to EnsoContext #11075
Conversation
4e6
added
the
CI: No changelog needed
4e6
requested review from
JaroslavTulach,
hubertp and
Akirathan
as code owners
engine/runtime/src/main/java/org/enso/interpreter/runtime/error/DataflowError.java
Outdated
Show resolved
Hide resolved
4e6
added
the
CI: Clean build required
mergify
bot
deleted the
wip/db/10719-refactor-state-execution-environment
branch
There was a problem hiding this comment.
It was originally suggested to:
Changing the state may require
e.g. to store State as "thread local variable" - but that's something we should do anyway.
I am sad that this PR only moves Environment and not the whole State.
Anyway, this PR changes the behavior. Previously the ExecutionEnvironment was associated with a thread - now each change is shared among all Enso threads.
E.g. this is change enables unexpected side-effects. That's wrong in a functional language like Enso.
| } | ||
|
|
||
| /** | ||
| * Enable execution context in the execution environment. |
There was a problem hiding this comment.
The description is wrong.
| @@ -54,7 +54,10 @@ public final class DataflowError extends AbstractTruffleException implements Ens | |||
| public static DataflowError withDefaultTrace(State state, Object payload, Node location) { | |||
| assert payload != null; | |||
| boolean attachFullStackTrace = | |||
| state == null ? true : state.currentEnvironment().hasContextEnabled("Dataflow_Stack_Trace"); | |||
| state == null | |||
There was a problem hiding this comment.
Why do we check for state? Previously we called a method on the state - e.g. the check was necessary. Now such a check is superfluous.
| public ExecutionEnvironment enableExecutionEnvironment(Atom context, String environmentName) { | ||
| ExecutionEnvironment original = executionEnvironment; | ||
| if (executionEnvironment.getName().equals(environmentName)) { | ||
| executionEnvironment = executionEnvironment.withContextEnabled(context); |
There was a problem hiding this comment.
This change changes the executionEnvironment for all threads, not only for the caller. That's a change in semantics.
| state == null | ||
| || EnsoContext.get(location) | ||
| .getExecutionEnvironment() | ||
| .hasContextEnabled("Dataflow_Stack_Trace"); | ||
| if (attachFullStackTrace) { |
There was a problem hiding this comment.
There is a performance bug
related to code in this method. How does your change affects the benchmarks?
Pull Request Description
related #10719
Changelog:
StatetoEnsoContextImportant Notes
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.