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.

Sign up for GitHub

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

Move execution environment from State to EnsoContext #11075

Merged
merged 3 commits into from
Sep 17, 2024
Merged

Move execution environment from State to EnsoContext #11075

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8ae12b0
refactor: move execution environment from State to EnsoContext
4e6 Sep 13, 2024
4dfcdfb
fix: dataflow error
4e6 Sep 15, 2024
22b7451
fix: with context
4e6 Sep 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions .../main/java/org/enso/interpreter/node/expression/builtin/runtime/ContextIsEnabledNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
import org.enso.interpreter.runtime.data.atom.Atom;
import org.enso.interpreter.runtime.error.PanicException;
import org.enso.interpreter.runtime.state.ExecutionEnvironment;
import org.enso.interpreter.runtime.state.State;

@BuiltinMethod(
type = "Context",
Expand All @@ -16,9 +15,9 @@
public class ContextIsEnabledNode extends Node {
private @Child ExpectStringNode expectStringNode = ExpectStringNode.build();

Object execute(State state, Atom self, Object environmentName) {
Object execute(Atom self, Object environmentName) {
String envName = expectStringNode.execute(environmentName);
ExecutionEnvironment currentEnv = state.currentEnvironment();
ExecutionEnvironment currentEnv = EnsoContext.get(this).getExecutionEnvironment();
if (!currentEnv.getName().equals(envName)) {
Atom error =
EnsoContext.get(this)
Expand Down
6 changes: 3 additions & 3 deletions ...o/interpreter/node/expression/builtin/runtime/RuntimeCurrentExecutionEnvironmentNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,16 @@

import com.oracle.truffle.api.nodes.Node;
import org.enso.interpreter.dsl.BuiltinMethod;
import org.enso.interpreter.runtime.EnsoContext;
import org.enso.interpreter.runtime.data.text.Text;
import org.enso.interpreter.runtime.state.State;

@BuiltinMethod(
type = "Runtime",
name = "current_execution_environment",
description = "Returns the name of the current execution environment.",
autoRegister = false)
public class RuntimeCurrentExecutionEnvironmentNode extends Node {
Object execute(State state) {
return Text.create(state.currentEnvironment().getName());
Object execute() {
return Text.create(EnsoContext.get(this).getExecutionEnvironment().getName());
}
}
11 changes: 9 additions & 2 deletions .../org/enso/interpreter/node/expression/builtin/runtime/RuntimeWithDisabledContextNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@
import org.enso.interpreter.node.BaseNode;
import org.enso.interpreter.node.callable.thunk.ThunkExecutorNode;
import org.enso.interpreter.node.expression.builtin.text.util.ExpectStringNode;
import org.enso.interpreter.runtime.EnsoContext;
import org.enso.interpreter.runtime.data.atom.Atom;
import org.enso.interpreter.runtime.state.ExecutionEnvironment;
import org.enso.interpreter.runtime.state.State;

@BuiltinMethod(
Expand All @@ -23,7 +25,12 @@ public class RuntimeWithDisabledContextNode extends Node {
Object execute(
VirtualFrame frame, State state, Atom context, Object env_name, @Suspend Object action) {
String envName = expectStringNode.execute(env_name);
return thunkExecutorNode.executeThunk(
frame, action, state.withContextDisabledIn(context, envName), BaseNode.TailStatus.NOT_TAIL);
ExecutionEnvironment original =
EnsoContext.get(this).disableExecutionEnvironment(context, envName);
try {
return thunkExecutorNode.executeThunk(frame, action, state, BaseNode.TailStatus.NOT_TAIL);
} finally {
EnsoContext.get(this).setExecutionEnvironment(original);
}
}
}
11 changes: 9 additions & 2 deletions ...a/org/enso/interpreter/node/expression/builtin/runtime/RuntimeWithEnabledContextNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@
import org.enso.interpreter.node.BaseNode;
import org.enso.interpreter.node.callable.thunk.ThunkExecutorNode;
import org.enso.interpreter.node.expression.builtin.text.util.ExpectStringNode;
import org.enso.interpreter.runtime.EnsoContext;
import org.enso.interpreter.runtime.data.atom.Atom;
import org.enso.interpreter.runtime.state.ExecutionEnvironment;
import org.enso.interpreter.runtime.state.State;

@BuiltinMethod(
Expand All @@ -23,7 +25,12 @@ public class RuntimeWithEnabledContextNode extends Node {
Object execute(
VirtualFrame frame, State state, Atom context, Object env_name, @Suspend Object action) {
String envName = expectStringNode.execute(env_name);
return thunkExecutorNode.executeThunk(
frame, action, state.withContextEnabledIn(context, envName), BaseNode.TailStatus.NOT_TAIL);
ExecutionEnvironment original =
EnsoContext.get(this).enableExecutionEnvironment(context, envName);
try {
return thunkExecutorNode.executeThunk(frame, action, state, BaseNode.TailStatus.NOT_TAIL);
} finally {
EnsoContext.get(this).setExecutionEnvironment(original);
}
}
}
29 changes: 29 additions & 0 deletions engine/runtime/src/main/java/org/enso/interpreter/runtime/EnsoContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
import org.enso.interpreter.OptionsHelper;
import org.enso.interpreter.runtime.builtin.Builtins;
import org.enso.interpreter.runtime.data.Type;
import org.enso.interpreter.runtime.data.atom.Atom;
import org.enso.interpreter.runtime.error.DataflowError;
import org.enso.interpreter.runtime.error.PanicException;
import org.enso.interpreter.runtime.instrument.NotificationHandler;
Expand Down Expand Up @@ -875,6 +876,34 @@ public void setExecutionEnvironment(ExecutionEnvironment executionEnvironment) {
this.executionEnvironment = executionEnvironment;
}

/**
* Enable execution context in the execution environment.
*
* @param context the execution context
* @param environmentName the execution environment name
*/
public ExecutionEnvironment enableExecutionEnvironment(Atom context, String environmentName) {
ExecutionEnvironment original = executionEnvironment;
if (executionEnvironment.getName().equals(environmentName)) {
executionEnvironment = executionEnvironment.withContextEnabled(context);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change changes the executionEnvironment for all threads, not only for the caller. That's a change in semantics.

}
return original;
}

/**
* Enable execution context in the execution environment.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description is wrong.

*
* @param context the execution context
* @param environmentName the execution environment name
*/
public ExecutionEnvironment disableExecutionEnvironment(Atom context, String environmentName) {
ExecutionEnvironment original = executionEnvironment;
if (executionEnvironment.getName().equals(environmentName)) {
executionEnvironment = executionEnvironment.withContextDisabled(context);
}
return original;
}

/** Returns a maximal number of warnings that can be attached to a value */
public int getWarningsLimit() {
return this.warningsLimit;
Expand Down
5 changes: 4 additions & 1 deletion engine/runtime/src/main/java/org/enso/interpreter/runtime/error/DataflowError.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

|| EnsoContext.get(location)
.getExecutionEnvironment()
.hasContextEnabled("Dataflow_Stack_Trace");
if (attachFullStackTrace) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a performance bug

related to code in this method. How does your change affects the benchmarks?

var result = new DataflowError(payload, UNLIMITED_STACK_TRACE, location);
TruffleStackTrace.fillIn(result);
Expand Down
28 changes: 2 additions & 26 deletions engine/runtime/src/main/java/org/enso/interpreter/runtime/state/State.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,44 +3,20 @@
import com.oracle.truffle.api.object.DynamicObject;
import com.oracle.truffle.api.object.Shape;
import org.enso.interpreter.runtime.EnsoContext;
import org.enso.interpreter.runtime.data.atom.Atom;

public class State {
private final Container container;

private final ExecutionEnvironment executionEnvironment;

public State(Container container, ExecutionEnvironment executionEnvironment) {
public State(Container container) {
this.container = container;
this.executionEnvironment = executionEnvironment;
}

public Container getContainer() {
return container;
}

public ExecutionEnvironment currentEnvironment() {
return executionEnvironment;
}

public static State create(EnsoContext context) {
return new State(Container.create(context), context.getExecutionEnvironment());
}

public State withContextEnabledIn(Atom context, String environmentName) {
if (executionEnvironment.getName().equals(environmentName)) {
return new State(container, executionEnvironment.withContextEnabled(context));
} else {
return this;
}
}

public State withContextDisabledIn(Atom context, String environmentName) {
if (executionEnvironment.getName().equals(environmentName)) {
return new State(container, executionEnvironment.withContextDisabled(context));
} else {
return this;
}
return new State(Container.create(context));
}

public static class Container extends DynamicObject {
Expand Down
Loading