-
Notifications
You must be signed in to change notification settings - Fork 316
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.
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
WIP: Extract mutable builder from ModuleScope
#9914
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
private Map<Type, Map<Type, Function>> conversions; | ||
private Set<ModuleScope> imports; | ||
private Set<ModuleScope> exports; | ||
private final Map<String, Object> polyglotSymbols; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, this is great. Having all fields in ModuleScope
final, is the goal.
public void addExport(ModuleScope scope) { | ||
exports.add(scope); | ||
} | ||
|
||
public Map<String, Type> getTypes() { | ||
return types; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should make sure the returned Map
is immutable. E.g. wrap it by Collections.unmodifiableMap
either here or in the constructor.
|
||
public ModuleScope build() { | ||
if (moduleScope == null) { | ||
synchronized (this) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that I have removed all the ConcurrentHashMap
stuff in my recently merged PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
conversions = new ConcurrentHashMap<>(); | ||
imports = new HashSet<>(); | ||
exports = new HashSet<>(); | ||
moduleScope = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is inconsistent with the synchronized block in build
- make whole reset
synchronized? Remove synchronization and assert only single threaded usage of the builder?
@@ -131,7 +131,7 @@ import scala.jdk.OptionConverters._ | |||
class IrToTruffle( | |||
val context: EnsoContext, | |||
val source: Source, | |||
val moduleScope: ModuleScope, | |||
val moduleScope: ModuleScope.Builder, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks promising!
@@ -145,10 +143,18 @@ public LocalScope getLocalScope() { | |||
* | |||
* @return the module scope for this node | |||
*/ | |||
public ModuleScope getModuleScope() { | |||
public ModuleScope.Builder getModuleScope() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -55,7 +55,7 @@ public LocalScope getLocalScope() { | |||
return localScope; | |||
} | |||
|
|||
public ModuleScope getModuleScope() { | |||
public ModuleScope.Builder getModuleScope() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does ScopeInfo
needs a Builder
? Shouldn't all the scope already be built before execution even gets here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
@@ -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"}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this change necessary? Casting null
to ModuleScope
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
build
is currently overloaded.
engine/runtime/src/main/java/org/enso/interpreter/runtime/scope/ModuleScope.java
Outdated
Show resolved
Hide resolved
|
||
public ModuleScope build() { | ||
if (moduleScope == null) { | ||
synchronized (this) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -54,7 +54,7 @@ public class ClosureRootNode extends EnsoRootNode { | |||
public static ClosureRootNode build( | |||
EnsoLanguage language, | |||
LocalScope localScope, | |||
ModuleScope moduleScope, | |||
ModuleScope.Builder moduleScope, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
private final String name; | ||
private @CompilerDirectives.CompilationFinal ModuleScope definitionScope; | ||
private @CompilerDirectives.CompilationFinal ModuleScope.Builder definitionScope; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DOes it make sense to declare fields of data as @CompilationFinal
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@Akirathan @JaroslavTulach I understand the sentiment that builder shouldn't be visible outside of |
@@ -740,6 +724,7 @@ class IrToTruffle( | |||
moduleScope.registerConversionMethod(toType, fromType, function) | |||
} | |||
}) | |||
moduleScope.build() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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`.
Don't we have to support some form of mutability to allow recompilation of
... provide reference to uninitialized |
private ModuleSources sources; | ||
private QualifiedName name; | ||
private final ModuleScope.Builder scopeBuilder; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reference to builder - eh, I really cannot make any (static) judgements about such a design. Having a builder here doesn't make any logical sense to me.
Source source = getSource(); | ||
if (source == null) return; | ||
scope.reset(); | ||
scopeBuilder.reset(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the builder really immutable once .build()
? What does .reset()
then do?
|
||
public void reset() { | ||
polyglotSymbols = new LinkedHashMap<>(); | ||
// can't clear types because on recompilation methods etc will be assigned to the new one |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As suggested elsewhere, I would use Module
as eventually consistent view of ModuleScope
. I'd add:
class Module {
/** may return different instance of scope (when reparsing happened), but always consistenly filled from the latest successfull (re)parse.
ModuleScope getModuleScope();
/** Start building new {@link #getModuleScope()} for this Module. Records current module state for later
* {@code build()} call on the builder.
*
* @return new builder to create and set new ModuleScope into this Module. Reuses references to existing {@code types}.
*/
ModuleScope.Builder newBuilder();
}
/** Creates new {@code ModuleScope} and atomically sets it to associated {@code Module}. Builder is made
* unusable after calling its {@code build()} method.
* @throws ConcurrentModificationException if the {@code Module} was modified since the creation of the builder by some other builder
*/
void ModuleScope.Builder.build();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recall why I couldn't do it this way - when constructing truffle nodes (IrToTruffle
) we occasionally reference modules. Those modules are still wip, meaning that their ModuleScope
is not available, yet, but we need it to resolve methods. I will still try to figure out if we can get something vaguely similar to the above requirement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those modules are still wip, meaning that their ModuleScope is not available
What do you mean by wip?
- is
Module
instance available? Then keep pointer to it, it will getModuleScope
eventually... - ok,
ModuleScope
isn't yet ready, but it will be eventually - "we need it to resolve methods" - how can you resolve methods without finishing resolution of
ModuleScope
?
Is the code resolving methods based on partially resolved ModuleScope
? That seems buggy to even consider...
`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.
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:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.