Extract mutable builder from `ModuleScope` #9914

hubertp · 2024-05-10T09:16:37Z

Pull Request Description

Refactored mutable parts of ModuleScope into builder to make it easier to reduce unnecessary locks.

Important Notes

Elements of ModuleScope (types, imports etc) are used while building of it may still be in progress. In order to make static typing happy, every ModuleScope.Builder can be exposed as (unmodifiable) ModuleScope.

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

Ideally, I'd like the Builder to be more hidden, not flying around being accessed by everybody. But even the current state that makes ModuleScope immutable is a step forward and deserves to be integrated. Thank you!

JaroslavTulach · 2024-05-13T12:03:58Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/scope/ModuleScope.java

-  private Map<Type, Map<Type, Function>> conversions;
-  private Set<ModuleScope> imports;
-  private Set<ModuleScope> exports;
+  private final Map<String, Object> polyglotSymbols;


Yup, this is great. Having all fields in ModuleScope final, is the goal.

engine/runtime/src/main/java/org/enso/interpreter/runtime/scope/ModuleScope.java

JaroslavTulach · 2024-05-13T12:11:59Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/scope/ModuleScope.java

+
+    public ModuleScope build() {
+      if (moduleScope == null) {
+        synchronized (this) {


synchronized? Only useful if we encourage the Builder to be used from multiple threads. But it shouldn't be used that way. A Builder should only be used from a single thread, am I right?

If so, we should assert Thread.currentThread() == builderCreatorThread at few places to ensure Builder is always used from a single thread.

AFAIK, ModuleScope is constructed only from IrToTruffle and that is single-threaded. Moreover, only one module is processed at single time. So I believe that usage of synchronized does not make sense as well. Moreover, I believe that it does not make sense to use ConcurrentHashMap as fields.

Note that I have removed all the ConcurrentHashMap stuff in my recently merged PR.

If we enable multi-threading for visualizations, which can as a side-effect trigger compilation, then we can have multiple threads accessing/adding to the builder. That was also the motivation for this PR, to make it more explicit.

engine/runtime/src/main/java/org/enso/interpreter/runtime/scope/ModuleScope.java

JaroslavTulach · 2024-05-13T12:14:20Z

engine/runtime/src/main/scala/org/enso/interpreter/runtime/IrToTruffle.scala

@@ -131,7 +131,7 @@ import scala.jdk.OptionConverters._
 class IrToTruffle(
  val context: EnsoContext,
  val source: Source,
-  val moduleScope: ModuleScope,
+  val moduleScope: ModuleScope.Builder,


That looks promising!

engine/runtime/src/main/java/org/enso/interpreter/node/EnsoRootNode.java

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

Akirathan

Altough draft PR, I am still submitting "Request changes" here. In the current form, I don't see a lot of improvements over the previous state. I would like to see most, if not all, the runtime nodes and classes to receive immutable ModuleScope as parameters and fields, not ModuleScope.Builder.

...on-tests/src/test/java/org/enso/interpreter/node/expression/builtin/meta/TypeOfNodeTest.java

engine/runtime/src/main/java/org/enso/interpreter/runtime/scope/ModuleScope.java

Akirathan · 2024-05-22T10:36:39Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/scope/ModuleScope.java

+
+    public ModuleScope build() {
+      if (moduleScope == null) {
+        synchronized (this) {


AFAIK, ModuleScope is constructed only from IrToTruffle and that is single-threaded. Moreover, only one module is processed at single time. So I believe that usage of synchronized does not make sense as well. Moreover, I believe that it does not make sense to use ConcurrentHashMap as fields.

Akirathan · 2024-05-22T10:40:18Z

engine/runtime/src/main/java/org/enso/interpreter/node/ClosureRootNode.java

@@ -54,7 +54,7 @@ public class ClosureRootNode extends EnsoRootNode {
  public static ClosureRootNode build(
      EnsoLanguage language,
      LocalScope localScope,
-      ModuleScope moduleScope,
+      ModuleScope.Builder moduleScope,


I believe only IrToTruffle should use ModuleScope.Builder for its purposes. All the runtime nodes should have only immutable ModuleScope. Is that possible? Having ModuleScope.Builder as fields in some truffle nodes diminishes the whole idea of this PR, since we will still be able to mutate the underlying builder at runtime, which should be restricted. No?

Akirathan · 2024-05-22T10:43:46Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/Type.java

  private final String name;
-  private @CompilerDirectives.CompilationFinal ModuleScope definitionScope;
+  private @CompilerDirectives.CompilationFinal ModuleScope.Builder definitionScope;


DOes it make sense to declare fields of data as @CompilationFinal?

It might make sense. Instances of Type are directly referenced from various Enso Node - e.g. they are part of the AST. Remember we have the EXCLUSIVE sharing policy - there is no sharing of ASTs.

However, please don't reference builder, reference Module.

hubertp · 2024-05-24T10:49:18Z

@Akirathan @JaroslavTulach
To cut the long story short, your requirement of having only ModuleScope is satisfied in runtime because you won't be able to mutate builder once something calls build(). Statically, yes, you still see ModuleScope.Builder in various places.

I understand the sentiment that builder shouldn't be visible outside of IrToTruffle but take into account that since Truffle nodes are built during IrToTruffle the only thing that I can provide reference to is ModuleScope.Builder not ModuleScope.
In fact that is pretty much the only reason why you still see ModuleScope.Builder in various nodes.
I wanted to have only ModuleScope in Truffle nodes but I have yet to figure out, if possible at all, how to make typechecker happy with that requirement.

engine/runtime/src/main/scala/org/enso/interpreter/runtime/IrToTruffle.scala

Added `built` method, apart from the existing `build` method, to return ModuleScope associated with the builder. This way it becomes clear when builder transforms into `ModuleScope`.

JaroslavTulach · 2024-05-27T07:52:29Z

builder shouldn't be visible outside of IrToTruffle but take into account that since Truffle nodes are built during IrToTruffle the only thing that I can provide reference to is ModuleScope.Builder not ModuleScope.

Don't we have to support some form of mutability to allow recompilation of Module? Where such mutability happens? A Module may get assigned different ModuleScope? If so...

I can provide reference to is ModuleScope.Builder not ModuleScope.

... provide reference to uninitialized Module and fill it in later when .build() is called.

engine/runtime/src/main/java/org/enso/interpreter/runtime/Module.java

engine/runtime/src/main/java/org/enso/interpreter/runtime/scope/ModuleScope.java

`ImportExportScope` serves as a proxy to the underlying module's scope. Can't reference the scope directly at the point when it is built because building of some modules might still be in progress. This also allowed to get rid off the horrible `withTypes` method that would create a temporary module scope.

It's unused and untested anyway.

Simplified the whole approach by avoiding calls to `built` and `build` to determine the state of `ModuleScope.Builder`. Instead, `ModuleScope.Builder` is only passed whenever a registration is necessary. Since builder is accessed in various places, while it may still, in theory, be constructor, this change introduces a simple, short-lived, proxy that delegates to accessors and satisfies the `ModuleScope` API.

New builder means new builder (minus types).

engine/runtime/src/main/java/org/enso/interpreter/runtime/scope/ModuleScope.java

JaroslavTulach

I don't think the PR gets us to a state where I'd like to be, but it is certainly making the system better. Ideally I want:

var b = module.newScopeBuilder(reuseYesOrNo);
b.register(...)
b.build() // andInstall();

where buildAndInstall() actually verifies the current module scope is the same as the scope when the builder was created and otherwise yields an inconsistency error.

JaroslavTulach · 2024-06-04T12:24:39Z

distribution/lib/Standard/Base/0.0.0-dev/src/Meta.enso

@@ -348,7 +340,7 @@ get_polyglot_language value = @Builtin_Method "Meta.get_polyglot_language"

   Arguments:
   - name: The name of the unresolved symbol.
-   - scope: The scope in which the symbol name is unresolved.
+   - symbol: The unresolved symbol from which to get the scope.
 create_unresolved_symbol : Text -> Any -> Unresolved_Symbol
 create_unresolved_symbol name scope = @Builtin_Method "Meta.create_unresolved_symbol"


The argument is still named scope.

JaroslavTulach · 2024-06-04T12:26:41Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/Module.java

-    } else {
-      return scope.withTypes(types);
-    }
+  public ModuleScope.Builder getScopeBuilder() {


Exposing current builder (created by whoever else), isn't really safe.

JaroslavTulach · 2024-06-04T12:27:01Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/Module.java

+    return scopeBuilder;
+  }
+
+  public ModuleScope.Builder newScopeBuilder() {


Creating new builder is OK. But storing it in this.scopeBuilder variable isn't correct.

JaroslavTulach · 2024-06-04T12:28:27Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/Module.java

+    return builder;
+  }
+
+  public void resetScope() {


I'd prefer newScopeBuilder(boolean inheritTypes) slightly...

JaroslavTulach · 2024-06-04T12:31:26Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/Module.java

  private ModuleSources sources;
+  private QualifiedName name;


Why isn't QualifiedName final? I don't see any write to it...

Three is a rename method.

I see. Thank you. However, in the name of exorcising mutability away from Module it may be reasonable to ask whether name shouldn't be a temporary property - e.g. be held by ModuleScope?

JaroslavTulach · 2024-06-04T12:54:52Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/Type.java

  private final String name;
-  private @CompilerDirectives.CompilationFinal ModuleScope definitionScope;
+  private @CompilerDirectives.CompilationFinal ModuleScope.Builder definitionScope;


However, please don't reference builder, reference Module.

JaroslavTulach · 2024-06-04T12:55:40Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/scope/ImportExportScope.java

+ * A proxy scope delegating to the underlying module's scope. Additionally, `ImportExportScope` may
+ * limit the number of types that are imported/exported.
+ */
+public class ImportExportScope implements EnsoObject {


Why don't this object extend EnsoObject? It shouldn't flow thru user data right?

Make final.

? EnsoObject is an interface, I don't understand the request.

Yes, EnsoObject is an interface with a purpose (described in its javadoc) of:

/** All non-primitive Enso types extends from {@code EnsoObject}. */

why do you believe ImportExportScope should be a non-primitive Enso type? Do you believe this object shall flow thru user program? Unless I am mistaken you have removed exposure of ModuleScope from Meta.enso claiming "it is dangerous" - why do you believe ImportExportScope should be exposed (and manipulated) by Enso programs?

Suggested change

public class ImportExportScope implements EnsoObject {

public final class ImportExportScope {

JaroslavTulach · 2024-06-04T12:56:55Z

...time/src/main/java/org/enso/interpreter/runtime/scope/TruffleCompilerModuleScopeBuilder.java

+
+import org.enso.compiler.context.CompilerContext;
+
+public class TruffleCompilerModuleScopeBuilder extends CompilerContext.ModuleScopeBuilder {


JaroslavTulach · 2024-06-04T13:10:19Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/TruffleCompilerContext.java

@@ -723,8 +734,7 @@ public void close() {
        module.module.setLoadedFromCache(loadedFromCache);
      }
      if (resetScope) {
-        module.module.ensureScopeExists();
-        module.module.getScope().reset();
+        module.module.resetScope();


I would expect this to call newBuilder().

resetScope does call newBuilder

JaroslavTulach · 2024-06-04T13:12:31Z

engine/runtime/src/main/java/org/enso/interpreter/node/expression/debug/EvalNode.java

@@ -86,7 +86,7 @@ RootCallTarget parseExpression(LocalScope scope, ModuleScope moduleScope, String
    var sco = newInlineContext.localScope().getOrElse(LocalScope::root);
    var mod = newInlineContext.getModule();
    var m = org.enso.interpreter.runtime.Module.fromCompilerModule(mod);
-    var toTruffle = new IrToTruffle(context, src, m.getScope(), compiler.getConfig());
+    var toTruffle = new IrToTruffle(context, src, m.getScopeBuilder(), compiler.getConfig());


I can imagine that this is a place where EvalNode wants to incrementally add something to the scope. Still having some sequence like:

var b = m.newScopeBuilder(reuseEverything=true) b.registerXyz(...) b.build() // and install into `Module`

is a way better than having a getter to existing scope builder.

Akirathan

Generally looks like a better design than the one we currently have. There might be some follow-up issues.

...ne/runtime/src/main/java/org/enso/interpreter/node/callable/dispatch/InvokeFunctionNode.java

engine/runtime/src/main/java/org/enso/interpreter/runtime/scope/ModuleScope.java

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/atom/AtomConstructor.java

engine/runtime/src/main/java/org/enso/interpreter/runtime/scope/ModuleScope.java

...ument-common/src/main/scala/org/enso/interpreter/instrument/job/UpsertVisualizationJob.scala

JaroslavTulach · 2024-06-05T11:57:08Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/scope/ModuleScope.java

+
+  public static class Builder {
+
+    @CompilerDirectives.CompilationFinal private ModuleScope moduleScope = null;


I doubt this is correct. How is a code using this moduleScope field in PE going to be invalidated when the field changes?

Anyway, I believe Builder shouldn't be used on fast path. Rather put CompilerAsserts.neverPartOfCompilation(); to every method of the Builder!

hubertp · 2024-06-05T15:38:38Z

There are few minor issues that I'd like to address separately, as this PR has already been dragging on. Most importantly remove the getScopeBuilder method and instead pass it around during compilation. But I will make the change in a follow up.

hubertp added 8 commits May 9, 2024 19:21

Minor refactorings

4da192d

wip

f2122ec

don't reset types in scopes

bfdf093

Make builders usage explicit

2484337

minor allocation optimizations

c49ae59

reduce lock scope

e6ec56d

remove redundant DelayedModuleScope

201d76e

nits

c7da06b

hubertp added the CI: No changelog needed Do not require a changelog entry for this PR. label May 13, 2024

JaroslavTulach approved these changes May 22, 2024

View reviewed changes

Akirathan requested changes May 22, 2024

View reviewed changes

enso-bot bot mentioned this pull request May 23, 2024

Cannot access "non-node" arguments in drop down widgets evaluation #9941

Closed

Merge branch 'develop' into wip/hubert/9736-visualizations

696d44f

hubertp commented May 24, 2024

View reviewed changes

engine/runtime/src/main/scala/org/enso/interpreter/runtime/IrToTruffle.scala Outdated Show resolved Hide resolved

Illustrate when immutable ModulesScope is created

a53bf6c

Added `built` method, apart from the existing `build` method, to return ModuleScope associated with the builder. This way it becomes clear when builder transforms into `ModuleScope`.

JaroslavTulach reviewed May 27, 2024

View reviewed changes

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

JaroslavTulach reviewed May 27, 2024

View reviewed changes

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

JaroslavTulach reviewed May 27, 2024

View reviewed changes

engine/runtime/src/main/java/org/enso/interpreter/runtime/scope/ModuleScope.java Outdated Show resolved Hide resolved

hubertp added 3 commits May 28, 2024 15:57

Ditch get_unresolved_symbol_scope

d88d27d

It's unused and untested anyway.

hubertp force-pushed the wip/hubert/9736-visualizations branch from e4e2f20 to 1f347d0 Compare June 2, 2024 20:27

hubertp added 5 commits June 3, 2024 17:59

ModuleScope.reset() no more

1f61c3f

New builder means new builder (minus types).

cleanup

02117cf

drop aux methods by delegating to proxy

10c48be

Ensure elements of ModuleScope are immutable

209c4a3

Merge branch 'develop' into wip/hubert/9736-visualizations

6cd3c36

hubertp marked this pull request as ready for review June 4, 2024 10:44

hubertp requested review from GregoryTravis, AdRiley, marthasharkey and 4e6 as code owners June 4, 2024 10:44

hubertp requested a review from Akirathan June 4, 2024 10:44

hubertp changed the title ~~WIP: Extract mutable builder from ModuleScope~~ Extract mutable builder from ModuleScope Jun 4, 2024

4e6 reviewed Jun 4, 2024

View reviewed changes

engine/runtime/src/main/java/org/enso/interpreter/runtime/scope/ModuleScope.java Outdated Show resolved Hide resolved

JaroslavTulach approved these changes Jun 4, 2024

View reviewed changes

hubertp added 3 commits June 4, 2024 18:16

Adapt tests

d5f811c

nit

ffbeb95

PR review

be6330f

enso-bot bot mentioned this pull request Jun 5, 2024

Benchmark Truffle "add" for arrow-language and Column.+ #10056

Closed

hubertp added 2 commits June 5, 2024 10:04

Type can only have a reference to Module

aa1286e

Drop caching of views

6b4a208

Akirathan approved these changes Jun 5, 2024

View reviewed changes

hubertp added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Jun 5, 2024

JaroslavTulach reviewed Jun 5, 2024

View reviewed changes

hubertp added 2 commits June 5, 2024 15:12

PR comments

7cf8099

Merge branch 'develop' into wip/hubert/9736-visualizations

790c0d8

AdRiley approved these changes Jun 5, 2024

View reviewed changes

4e6 approved these changes Jun 5, 2024

View reviewed changes

fix compilation issue

93d5d69

hubertp added CI: Ready to merge This PR is eligible for automatic merge and removed CI: Ready to merge This PR is eligible for automatic merge labels Jun 5, 2024

nit

ed875eb

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

mergify bot merged commit 1bc1425 into develop Jun 5, 2024
36 checks passed

mergify bot deleted the wip/hubert/9736-visualizations branch June 5, 2024 16:57

radeusgd added a commit that referenced this pull request Jun 10, 2024

integrate back changes from #9914 after merge commit

414aeb8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract mutable builder from `ModuleScope` #9914

Extract mutable builder from `ModuleScope` #9914

hubertp commented May 10, 2024 •

edited

JaroslavTulach left a comment

JaroslavTulach May 13, 2024

JaroslavTulach May 13, 2024

Akirathan May 22, 2024

Akirathan May 22, 2024

hubertp May 24, 2024 •

edited

JaroslavTulach May 13, 2024

Akirathan left a comment

Akirathan May 22, 2024

Akirathan May 22, 2024

Akirathan May 22, 2024

JaroslavTulach May 22, 2024

JaroslavTulach Jun 4, 2024

hubertp commented May 24, 2024 •

edited

JaroslavTulach commented May 27, 2024

JaroslavTulach left a comment

JaroslavTulach Jun 4, 2024

JaroslavTulach Jun 4, 2024

JaroslavTulach Jun 4, 2024

JaroslavTulach Jun 4, 2024

JaroslavTulach Jun 4, 2024

hubertp Jun 4, 2024

JaroslavTulach Jun 5, 2024 •

edited

JaroslavTulach Jun 4, 2024

JaroslavTulach Jun 4, 2024

hubertp Jun 4, 2024

JaroslavTulach Jun 5, 2024 •

edited

JaroslavTulach Jun 4, 2024

JaroslavTulach Jun 4, 2024

hubertp Jun 4, 2024

JaroslavTulach Jun 4, 2024

Akirathan left a comment

JaroslavTulach Jun 5, 2024

hubertp commented Jun 5, 2024

	public class ImportExportScope implements EnsoObject {
	public final class ImportExportScope {


		import org.enso.compiler.context.CompilerContext;

		public class TruffleCompilerModuleScopeBuilder extends CompilerContext.ModuleScopeBuilder {


		public static class Builder {

		@CompilerDirectives.CompilationFinal private ModuleScope moduleScope = null;

Extract mutable builder from ModuleScope #9914

Extract mutable builder from ModuleScope #9914

Conversation

hubertp commented May 10, 2024 • edited

Pull Request Description

Important Notes

Checklist

JaroslavTulach left a comment

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

Choose a reason for hiding this comment

hubertp May 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Akirathan left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hubertp commented May 24, 2024 • edited

JaroslavTulach commented May 27, 2024

JaroslavTulach left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaroslavTulach Jun 5, 2024 • edited

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

JaroslavTulach Jun 5, 2024 • edited

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

Choose a reason for hiding this comment

Akirathan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hubertp commented Jun 5, 2024

Extract mutable builder from `ModuleScope` #9914

Extract mutable builder from `ModuleScope` #9914

hubertp commented May 10, 2024 •

edited

hubertp May 24, 2024 •

edited

hubertp commented May 24, 2024 •

edited

JaroslavTulach Jun 5, 2024 •

edited

JaroslavTulach Jun 5, 2024 •

edited