Replace IOContexts with execution env and contexts #6171

hubertp · 2023-04-02T19:33:48Z

Pull Request Description

As per design, IOContexts controlled via type signatures are going away. They are replaced by explicit Context.if_enabled runtime checks that will be added to particular method implementations.

production/development IOPermissions are replaced with live and design execution enviornment. Currently, the live env has a hardcoded list of allowed contexts i.e. Input and Output.

Important Notes

As per design PR-55. Closes #6129. Closes #6131.

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.
[ x All code follows the
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
All code has been tested:
- Unit tests have been written where possible.
- ~~If GUI codebase was changed, the GUI was tested when built using ./run ide build.~~

distribution/lib/Standard/Base/0.0.0-dev/src/Errors/Common.enso

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

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

engine/runtime/src/main/java/org/enso/interpreter/runtime/builtin/Error.java

JaroslavTulach · 2023-04-03T08:51:20Z

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

+  }
+
+  @CompilerDirectives.TruffleBoundary
+  public ExecutionEnvironment withContextEnabled(Atom context) {


Shouldn't we strive for making this function working on fast path?

I would have to refactor pretty much any call to map i.e. almost every line in that method, to make native image build happy. I would say that kind of defeats the purpose?

What's your purpose? Is the purpose to run Enso fast? Well, in such case that's not happening as soon as somebody uses withContextEnabled - that's always going to be slow.

refactor pretty much any call to map

Possibly using HashMap for storage of two values (Input, Output) wasn't good idea to begin with?

Fine, replaced by two arrays that emulate the map. The reason why I don't hardcode those in fields is that I'm trying to be generic enough in preparation for potential future Context values.

It is also a big overkill given that currently we only have Input and Output. So if you feel strongly about it, I could just go back to storing those in fields.

I am assuming that withContextEnabled/withContextDisabled operations are going to be spread all around the codebase possibly included inside of .map lambdas processing large data structures and we want to make them fast in order to avoid slowing down the rest of the interpreter. Is such assumption a false expectation? As I am seeing a disagreement and I don't understand what are its roots...

replaced by two arrays that emulate the map

The code is now less readable, but at least it doesn't need to be behind @TruffleBoundary. However I cannot judge how well it partially evaluates without looking at IGV graphs. Btw. what do we want to see in IGV when we look at withContextEnabled?

when already enabled - a single field/bit check to verify the context is already enabled

when disabled - more work - field write and switching the State to a new configuration (possibly also a costly allocation of the new configuration)

trying to be generic enough in preparation for potential future Context values.

Generic code can't be as fast as code that handles/speculates on fixed set of values.

... if you feel strongly ...

Maybe we could change one of our benchmarks to use withContextEnabled few million of times and compare it to a benchmark without context manipulation? Then we all would have something measurable to know how to feel about this matter.

I will file a separate ticket to investigate if this becomes a perf issue.

engine/runtime/src/test/java/org/enso/interpreter/test/ValuesGenerator.java

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

hubertp · 2023-04-03T09:59:51Z

@jdunkerley added a CLI option --execution-environment, as requested. Part of #6131.

engine/runner/src/main/scala/org/enso/runner/Main.scala

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

JaroslavTulach · 2023-04-04T07:44:42Z

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

+              .getBuiltins()
+              .error()
+              .makeUnimplemnted("execution environment mismatch");
+      throw new PanicException(error, this);


I really doubt a Panic is what we want! I don't even understand what's the purpose of specifying environment name in the is_enabled check. Environments are supposed to be a set of contexts. All the user wants is to take a context and check if it is currently enabled. E.g. Context.Input.is_enabled.

This isn't really specified well in the design doc. Nor is the usage of the environment name so if you feel strongly about it, probably worth bringing up with that doc's authors.

At least for the moment - there is no intention to specify anything other than the current context when calling this function.

If we find a need to use this we can revisit.

At least for the moment - there is no intention to specify anything other than the current context when calling this function.

Obviously! Specifying anything else makes no sense. At least I cannot envision the use-case.

I'd say: One doesn't care about the environment. One only wants to find out if a Context is_enabled "right now". Nothing else makes sense.

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

JaroslavTulach · 2023-04-04T07:46:57Z

...enso/interpreter/node/expression/builtin/runtime/RuntimeCurrentExecutionEnvironmentNode.java

+
+@BuiltinMethod(
+    type = "Runtime",
+    name = "current_execution_environment",


This method shall not exists from my API designer perspective as it bypasses the Context. I assume @jdunkerley wants the method to do production/non-production property configuration, but we should offer other means for that - not this method.

We agreed that Execution_Environment is really an alias for Text and I believe I'm following the design doc so I don't really understand the comment.

I won't be using this for configuration - from the libs perspective I don't expect this to be used.

There is a story about magical strings. I have a feeling the returned textual value has the potential to provide "magical" insights into something that should rather be hidden.

...I don't expect this to be used.

Personally I'd rather not expose such API, especially if there is no use-case for it.

engine/runtime/src/main/java/org/enso/interpreter/runtime/builtin/Error.java

JaroslavTulach · 2023-04-04T07:51:28Z

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

+  }
+
+  @CompilerDirectives.TruffleBoundary
+  public ExecutionEnvironment withContextEnabled(Atom context) {


What's your purpose? Is the purpose to run Enso fast? Well, in such case that's not happening as soon as somebody uses withContextEnabled - that's always going to be slow.

refactor pretty much any call to map

Possibly using HashMap for storage of two values (Input, Output) wasn't good idea to begin with?

jdunkerley

Couple of nit pics

engine/runner/src/main/scala/org/enso/runner/Main.scala

jdunkerley · 2023-04-04T12:04:43Z

...enso/interpreter/node/expression/builtin/runtime/RuntimeCurrentExecutionEnvironmentNode.java

+
+@BuiltinMethod(
+    type = "Runtime",
+    name = "current_execution_environment",


I won't be using this for configuration - from the libs perspective I don't expect this to be used.

jdunkerley · 2023-04-04T12:06:10Z

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

+              .getBuiltins()
+              .error()
+              .makeUnimplemnted("execution environment mismatch");
+      throw new PanicException(error, this);


At least for the moment - there is no intention to specify anything other than the current context when calling this function.

If we find a need to use this we can revisit.

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

engine/runner/src/main/scala/org/enso/runner/ContextFactory.scala

engine/runner/src/main/scala/org/enso/runner/Main.scala

As per design, IOContexts controlled via type signatures are going away. They are replaced by explicit `Context.if_enabled` runtime checks that will be added to particular method implementations. `production`/`development` `IOPermissions` are replaced with `live` and `design` execution enviornment. Currently, the `live` env has a hardcoded list of allowed contexts i.e. `Input` and `Output`.

Added a fix for #6131 to allow for some testing.

Changed the default execution env to `live` since that is probably what we want for CLI anyway. Got rid of `TruffleBoundary` annotations by using straight arrays that emulate map.

As per design, IOContexts controlled via type signatures are going away. They are replaced by explicit `Context.if_enabled` runtime checks that will be added to particular method implementations. `production`/`development` `IOPermissions` are replaced with `live` and `design` execution enviornment. Currently, the `live` env has a hardcoded list of allowed contexts i.e. `Input` and `Output`. # Important Notes As per design PR-55. Closes #6129. Closes #6131.

hubertp requested review from jdunkerley, radeusgd, 4e6 and JaroslavTulach as code owners April 2, 2023 19:33

hubertp force-pushed the wip/hubert/6129-runtime-context branch from 7bc7cce to 009c3f2 Compare April 2, 2023 21:25

radeusgd reviewed Apr 3, 2023

View reviewed changes

distribution/lib/Standard/Base/0.0.0-dev/src/Errors/Common.enso Outdated Show resolved Hide resolved

radeusgd reviewed Apr 3, 2023

View reviewed changes

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

radeusgd reviewed Apr 3, 2023

View reviewed changes

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

JaroslavTulach approved these changes Apr 3, 2023

View reviewed changes

hubertp force-pushed the wip/hubert/6129-runtime-context branch from e550579 to ad3b1bb Compare April 3, 2023 13:28

hubertp added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Apr 3, 2023

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

Delay creation of project root EnsoFile until it is really needed #6149

Merged

2 tasks

JaroslavTulach approved these changes Apr 4, 2023

View reviewed changes

hubertp force-pushed the wip/hubert/6129-runtime-context branch from a667c50 to 065462d Compare April 4, 2023 10:31

hubertp requested a review from radeusgd April 4, 2023 10:39

jdunkerley approved these changes Apr 4, 2023

View reviewed changes

4e6 reviewed Apr 4, 2023

View reviewed changes

engine/runner/src/main/scala/org/enso/runner/ContextFactory.scala Show resolved Hide resolved

engine/runner/src/main/scala/org/enso/runner/Main.scala Show resolved Hide resolved

hubertp force-pushed the wip/hubert/6129-runtime-context branch from 065462d to a4b267b Compare April 5, 2023 09:22

hubertp added the CI: Ready to merge This PR is eligible for automatic merge label Apr 5, 2023

unfurl-links bot mentioned this pull request Apr 6, 2023

Easy way to delay & cache computed atom fields #6134

Closed

4 tasks

hubertp added 9 commits April 6, 2023 12:29

changelog

a479184

make native image build happy

80a7018

Add cli option to control environment

31730d2

Added a fix for #6131 to allow for some testing.

PR review

c3ccb90

PR review

ef71994

revert to original truffleboundary

ff51b06

Address more PR comments

1809928

Changed the default execution env to `live` since that is probably what we want for CLI anyway. Got rid of `TruffleBoundary` annotations by using straight arrays that emulate map.

Address PR comments

9b5c12b

hubertp force-pushed the wip/hubert/6129-runtime-context branch from 7fb8813 to 9b5c12b Compare April 6, 2023 10:29

mergify bot added 2 commits April 6, 2023 11:00

Merge branch 'develop' into wip/hubert/6129-runtime-context

8fb0d73

Merge branch 'develop' into wip/hubert/6129-runtime-context

e02365a

mergify bot merged commit 3cce3b3 into develop Apr 6, 2023

mergify bot deleted the wip/hubert/6129-runtime-context branch April 6, 2023 15:47

JaroslavTulach mentioned this pull request May 2, 2023

Make execution mode live default for CLI #6496

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace IOContexts with execution env and contexts #6171

Replace IOContexts with execution env and contexts #6171

hubertp commented Apr 2, 2023 •

edited

Loading

JaroslavTulach Apr 3, 2023

hubertp Apr 3, 2023

JaroslavTulach Apr 4, 2023 •

edited

Loading

hubertp Apr 4, 2023

hubertp Apr 4, 2023 •

edited

Loading

JaroslavTulach Apr 5, 2023

hubertp Apr 5, 2023

hubertp commented Apr 3, 2023 •

edited

Loading

JaroslavTulach Apr 4, 2023

hubertp Apr 4, 2023 •

edited

Loading

jdunkerley Apr 4, 2023

JaroslavTulach Apr 4, 2023

JaroslavTulach Apr 4, 2023

hubertp Apr 4, 2023 •

edited

Loading

jdunkerley Apr 4, 2023

JaroslavTulach Apr 5, 2023

JaroslavTulach Apr 4, 2023 •

edited

Loading

jdunkerley left a comment

jdunkerley Apr 4, 2023

jdunkerley Apr 4, 2023

Replace IOContexts with execution env and contexts #6171

Replace IOContexts with execution env and contexts #6171

Conversation

hubertp commented Apr 2, 2023 • edited Loading

Pull Request Description

Important Notes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaroslavTulach Apr 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hubertp Apr 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hubertp commented Apr 3, 2023 • edited Loading

Choose a reason for hiding this comment

hubertp Apr 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hubertp Apr 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaroslavTulach Apr 4, 2023 • edited Loading

Choose a reason for hiding this comment

jdunkerley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hubertp commented Apr 2, 2023 •

edited

Loading

JaroslavTulach Apr 4, 2023 •

edited

Loading

hubertp Apr 4, 2023 •

edited

Loading

hubertp commented Apr 3, 2023 •

edited

Loading

hubertp Apr 4, 2023 •

edited

Loading

hubertp Apr 4, 2023 •

edited

Loading

JaroslavTulach Apr 4, 2023 •

edited

Loading