WIP: 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

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.

JaroslavTulach · 2024-05-13T12:06:51Z

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

-  public void addExport(ModuleScope scope) {
-    exports.add(scope);
-  }
-
  public Map<String, Type> getTypes() {
    return types;


We should make sure the returned Map is immutable. E.g. wrap it by Collections.unmodifiableMap either here or in the constructor.

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.

JaroslavTulach · 2024-05-13T12:13:37Z

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

+      conversions = new ConcurrentHashMap<>();
+      imports = new HashSet<>();
+      exports = new HashSet<>();
+      moduleScope = null;


This is inconsistent with the synchronized block in build - make whole reset synchronized? Remove synchronization and assert only single threaded usage of the builder?

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!

JaroslavTulach · 2024-05-22T09:31:24Z

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

@@ -145,10 +143,18 @@ public LocalScope getLocalScope() {
   *
   * @return the module scope for this node
   */
-  public ModuleScope getModuleScope() {
+  public ModuleScope.Builder getModuleScope() {


Ideally such a public getter of the builder should be avoided. The Builder should be private to the creator of it, not floating around being accessed by random parties.

Yes, I was hoping that would be the case as well, except that we are currently dealing with a situation when the builder's state is also inspected while stuff is added to it.

JaroslavTulach · 2024-05-22T09:32:19Z

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

@@ -55,7 +55,7 @@ public LocalScope getLocalScope() {
      return localScope;
    }

-    public ModuleScope getModuleScope() {
+    public ModuleScope.Builder getModuleScope() {


Why does ScopeInfo needs a Builder? Shouldn't all the scope already be built before execution even gets here?

Yes, I know this one is not obvious but this one is needed because of EvalNode's parseExpression which builds ClosureRootNode, which in turn requires the builder because that's the only thing that IrToTruffle has when it creates those as well. catch-22.

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.

Akirathan · 2024-05-22T10:27:19Z

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

@@ -56,7 +57,7 @@ public static Object[][] allPossibleEnsoInterpreterValues() throws Exception {
        data.add(new Object[] {raw, n});
      }
    }
-    data.add(new Object[] {UnresolvedSymbol.build("unknown_name", null), "Function"});
+    data.add(new Object[] {UnresolvedSymbol.build("unknown_name", (ModuleScope) null), "Function"});


Why is this change necessary? Casting null to ModuleScope?

build is currently overloaded.

Akirathan · 2024-05-22T10:34:26Z

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

+      Set<ModuleScope.Builder> imports = new HashSet<>(this.imports);
+      Set<ModuleScope.Builder> exports = new HashSet<>(this.exports);
+      this.types
+          .entrySet()


Suggested change

.entrySet()

.forEach((key, value) -> {

if (typeNames.contains(key)) {

requestedTypes.put(key, value);

}

});

for (var en : types.entrySet() is easier to step thru in debugger.

When you apply my suggestion, you don't have to call entry.getKey() and entry.getValue() in the subsequent code. That was the main reason for the suggestion.

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.

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.

hubertp · 2024-05-24T12:57:00Z

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

@@ -740,6 +724,7 @@ class IrToTruffle(
        moduleScope.registerConversionMethod(toType, fromType, function)
      }
    })
+    moduleScope.build()


Maybe this line got lost in the amount of changes but it is probably worth putting it in bold. This ensures that ModuleScope.Builder can't be modified.

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`.

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

Open

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

696d44f

hubertp commented May 24, 2024

View reviewed changes

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`.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Extract mutable builder from `ModuleScope` #9914

WIP: Extract mutable builder from `ModuleScope` #9914

hubertp commented May 10, 2024

JaroslavTulach left a comment

JaroslavTulach May 13, 2024

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

JaroslavTulach May 13, 2024

JaroslavTulach May 22, 2024

hubertp May 24, 2024

JaroslavTulach May 22, 2024

hubertp May 24, 2024

Akirathan left a comment

Akirathan May 22, 2024

hubertp May 24, 2024

Akirathan May 22, 2024

JaroslavTulach May 22, 2024

Akirathan May 22, 2024

Akirathan May 22, 2024

Akirathan May 22, 2024

Akirathan May 22, 2024

JaroslavTulach May 22, 2024

hubertp commented May 24, 2024 •

edited

hubertp May 24, 2024

-          .entrySet()
+          .forEach((key, value) -> {
+            if (typeNames.contains(key)) {
+              requestedTypes.put(key, value);
+            }
+          });

WIP: Extract mutable builder from ModuleScope #9914

Are you sure you want to change the base?

WIP: Extract mutable builder from ModuleScope #9914

Conversation

hubertp commented May 10, 2024

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

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

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

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

Choose a reason for hiding this comment

hubertp commented May 24, 2024 • edited

Choose a reason for hiding this comment

WIP: Extract mutable builder from `ModuleScope` #9914

WIP: Extract mutable builder from `ModuleScope` #9914

hubertp May 24, 2024 •

edited

hubertp commented May 24, 2024 •

edited