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.

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

Extract mutable builder from ModuleScope #9914

Merged
merged 27 commits into from
Jun 5, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
4da192d
Minor refactorings
hubertp Apr 30, 2024
f2122ec
wip
hubertp May 2, 2024
bfdf093
don't reset types in scopes
hubertp May 9, 2024
2484337
Make builders usage explicit
hubertp May 9, 2024
c49ae59
minor allocation optimizations
hubertp May 10, 2024
e6ec56d
reduce lock scope
hubertp May 10, 2024
201d76e
remove redundant DelayedModuleScope
hubertp May 10, 2024
c7da06b
nits
hubertp May 10, 2024
696d44f
Merge branch 'develop' into wip/hubert/9736-visualizations
hubertp May 24, 2024
a53bf6c
Illustrate when immutable ModulesScope is created
hubertp May 24, 2024
08c13ee
Introduce ImportExportScope proxy
hubertp May 28, 2024
d88d27d
Ditch `get_unresolved_symbol_scope`
hubertp May 28, 2024
1f347d0
Drop built/build check for ModuleScope.Builder
hubertp Jun 2, 2024
1f61c3f
ModuleScope.reset() no more
hubertp Jun 3, 2024
02117cf
cleanup
hubertp Jun 3, 2024
10c48be
drop aux methods by delegating to proxy
hubertp Jun 3, 2024
209c4a3
Ensure elements of ModuleScope are immutable
hubertp Jun 4, 2024
6cd3c36
Merge branch 'develop' into wip/hubert/9736-visualizations
hubertp Jun 4, 2024
d5f811c
Adapt tests
hubertp Jun 4, 2024
ffbeb95
nit
hubertp Jun 4, 2024
be6330f
PR review
hubertp Jun 4, 2024
aa1286e
Type can only have a reference to Module
hubertp Jun 5, 2024
6b4a208
Drop caching of views
hubertp Jun 5, 2024
7cf8099
PR comments
hubertp Jun 5, 2024
790c0d8
Merge branch 'develop' into wip/hubert/9736-visualizations
hubertp Jun 5, 2024
93d5d69
fix compilation issue
hubertp Jun 5, 2024
ed875eb
nit
hubertp Jun 5, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ static Set<String> localVarNames(int localVarsCnt) {
* Creates a main method, generates some local variables, and fills their identifiers in the given
* set.
*
* @param localVarsCnt How many local variables should be initialized in the main method
* @param localVarNames local variables that should be initialized in the main method
* @return Body of the main method
*/
static InlineSource createMainMethodWithLocalVars(Context ctx, Set<String> localVarNames)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ public TruffleLogger getLogger() {
public FunctionCallInstrumentationNode.FunctionCall prepareFunctionCall(
Module module, String typeName, String methodName)
throws TypeNotFoundException, MethodNotFoundException {
ModuleScope scope = module.compileScope(context);
ModuleScope scope = module.compileScope(context).build();
Type type =
scope
.getType(typeName)
Expand Down Expand Up @@ -777,7 +777,7 @@ public static FunctionPointer fromFunction(Function function) {

switch (rootNode) {
case MethodRootNode methodNode -> {
moduleName = methodNode.getModuleScope().getModule().getName();
moduleName = methodNode.getModule().getName();
typeName = methodNode.getType().getQualifiedName();
functionName = methodNode.getMethodName();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,72 +65,62 @@ class UpsertVisualizationJob(
val lockTimestamp =
ctx.locking.acquireContextLock(config.executionContextId)
try {
val writeLockTimestamp = ctx.locking.acquireWriteCompilationLock()
try {
val maybeCallable =
UpsertVisualizationJob.evaluateVisualizationExpression(
config.visualizationModule,
config.expression
val maybeCallable =
UpsertVisualizationJob.evaluateVisualizationExpression(
config.visualizationModule,
config.expression
)

maybeCallable match {
case Left(ModuleNotFound(moduleName)) =>
ctx.endpoint.sendToClient(
Api.Response(Api.ModuleNotFound(moduleName))
)
None

maybeCallable match {
case Left(ModuleNotFound(moduleName)) =>
ctx.endpoint.sendToClient(
Api.Response(Api.ModuleNotFound(moduleName))
)
None
case Left(EvaluationFailed(message, result)) =>
replyWithExpressionFailedError(
config.executionContextId,
visualizationId,
expressionId,
message,
result
)
None

case Left(EvaluationFailed(message, result)) =>
replyWithExpressionFailedError(
config.executionContextId,
case Right(EvaluationResult(module, callable, arguments)) =>
val visualization =
UpsertVisualizationJob.updateAttachedVisualization(
visualizationId,
expressionId,
message,
result
module,
config,
callable,
arguments
)
None

case Right(EvaluationResult(module, callable, arguments)) =>
val visualization =
UpsertVisualizationJob.updateAttachedVisualization(
visualizationId,
val stack = ctx.contextManager.getStack(config.executionContextId)
val runtimeCache = stack.headOption
.flatMap(frame => Option(frame.cache))
val cachedValue = runtimeCache
.flatMap(c => Option(c.get(expressionId)))
UpsertVisualizationJob.requireVisualizationSynchronization(
stack,
expressionId
)
cachedValue match {
case Some(value) =>
ProgramExecutionSupport.executeAndSendVisualizationUpdate(
config.executionContextId,
runtimeCache.getOrElse(new RuntimeCache),
stack.headOption.get.syncState,
visualization,
expressionId,
module,
config,
callable,
arguments
value
)
val stack = ctx.contextManager.getStack(config.executionContextId)
val runtimeCache = stack.headOption
.flatMap(frame => Option(frame.cache))
val cachedValue = runtimeCache
.flatMap(c => Option(c.get(expressionId)))
UpsertVisualizationJob.requireVisualizationSynchronization(
stack,
expressionId
)
cachedValue match {
case Some(value) =>
ProgramExecutionSupport.executeAndSendVisualizationUpdate(
config.executionContextId,
runtimeCache.getOrElse(new RuntimeCache),
stack.headOption.get.syncState,
visualization,
expressionId,
value
)
None
case None =>
Some(Executable(config.executionContextId, stack))
}
}
} finally {
ctx.locking.releaseWriteCompilationLock()
logger.log(
Level.FINEST,
s"Kept write compilation lock [UpsertVisualizationJob] for ${System
.currentTimeMillis() - writeLockTimestamp} milliseconds"
)
None
case None =>
Some(Executable(config.executionContextId, stack))
}
}
} finally {
ctx.locking.releaseContextLock(config.executionContextId)
Expand Down Expand Up @@ -299,7 +289,6 @@ object UpsertVisualizationJob {
): Either[EvaluationFailure, AnyRef] = {
Either
.catchNonFatal {
ctx.locking.assertWriteCompilationLock()
ctx.executionService.evaluateExpression(module, argumentExpression)
}
.leftFlatMap {
Expand Down Expand Up @@ -370,7 +359,6 @@ object UpsertVisualizationJob {
.catchNonFatal {
expression match {
case Api.VisualizationExpression.Text(_, expression, _) =>
ctx.locking.assertWriteCompilationLock()
ctx.executionService.evaluateExpression(
expressionModule,
expression
Expand Down Expand Up @@ -519,7 +507,16 @@ object UpsertVisualizationJob {
callback,
arguments
)
invalidateCaches(visualization)
val writeLockTimestamp = ctx.locking.acquireWriteCompilationLock()
try {
invalidateCaches(visualization)
} finally {
ctx.locking.releaseWriteCompilationLock()
ctx.executionService.getLogger.log(
Level.FINEST,
s"Kept write compilation lock [UpsertVisualizationJob] for ${System.currentTimeMillis() - writeLockTimestamp} milliseconds"
)
}
ctx.contextManager.upsertVisualization(
visualizationConfig.executionContextId,
visualization
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import java.math.BigInteger;
import org.enso.interpreter.node.callable.resolver.HostMethodCallNode;
import org.enso.interpreter.runtime.callable.UnresolvedSymbol;
import org.enso.interpreter.runtime.scope.ModuleScope;
import org.enso.interpreter.test.TestBase;
import org.graalvm.polyglot.Context;
import org.junit.AfterClass;
Expand All @@ -33,7 +34,7 @@ public static void closeCtx() {
public void javaBigIntegerDispatch() {
var big = new BigInteger("4324908174321000432143143778956741");
var val = unwrapValue(ctx, ctx.asValue(big));
var sym = UnresolvedSymbol.build("+", null);
var sym = UnresolvedSymbol.build("+", (ModuleScope) null);
var typ = HostMethodCallNode.getPolyglotCallType(val, sym, InteropLibrary.getUncached());
assertEquals(HostMethodCallNode.PolyglotCallType.CONVERT_TO_BIG_INT, typ);
assertFalse(typ.isInteropLibrary());
Expand All @@ -42,7 +43,7 @@ public void javaBigIntegerDispatch() {
@Test
public void textDispatch() {
var val = "a text";
var sym = UnresolvedSymbol.build("+", null);
var sym = UnresolvedSymbol.build("+", (ModuleScope) null);
var typ = HostMethodCallNode.getPolyglotCallType(val, sym, InteropLibrary.getUncached());
assertEquals(HostMethodCallNode.PolyglotCallType.CONVERT_TO_TEXT, typ);
assertFalse(typ.isInteropLibrary());
Expand All @@ -51,7 +52,7 @@ public void textDispatch() {
@Test
public void longDispatch() {
var val = 4L;
var sym = UnresolvedSymbol.build("+", null);
var sym = UnresolvedSymbol.build("+", (ModuleScope) null);
var typ = HostMethodCallNode.getPolyglotCallType(val, sym, InteropLibrary.getUncached());
assertEquals(HostMethodCallNode.PolyglotCallType.CONVERT_TO_BIG_INT, typ);
assertFalse(typ.isInteropLibrary());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import org.enso.interpreter.runtime.callable.UnresolvedSymbol;
import org.enso.interpreter.runtime.error.DataflowError;
import org.enso.interpreter.runtime.library.dispatch.TypeOfNode;
import org.enso.interpreter.runtime.scope.ModuleScope;
import org.enso.interpreter.test.TestBase;
import org.enso.interpreter.test.ValuesGenerator;
import org.graalvm.polyglot.Context;
Expand Down Expand Up @@ -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"});
hubertp marked this conversation as resolved.
Show resolved Hide resolved
data.add(new Object[] {UnresolvedConstructor.build(null, "Unknown_Name"), "Function"});
return data.toArray(new Object[0][]);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ public FunctionCallInfo extractFunctionCallInfo(Object functionCallObj) {
if (functionCallObj instanceof FunctionCall functionCall) {
RootNode rootNode = functionCall.getFunction().getCallTarget().getRootNode();
if (rootNode instanceof MethodRootNode methodNode) {
String moduleName = methodNode.getModuleScope().getModule().getName().toString();
String moduleName = methodNode.getModule().getName().toString();
String typeName = methodNode.getType().getQualifiedName().toString();
String functionName = methodNode.getMethodName();
return new FunctionCallInfo(moduleName, typeName, functionName);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ public interface IR {
* @return the external identifier for this IR node
*/
default Option<@ExternalID UUID> getExternalId() {
return location().flatMap(l -> l.id().map(id -> id));
return location().flatMap(l -> l.id());
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,8 @@ object CallArgument {
|""".toSingleLine

/** @inheritdoc */
override def children: List[IR] = name.toList :+ value
override def children: List[IR] =
name.map(List(_, value)).getOrElse(List(value))

/** @inheritdoc */
override def showCode(indent: Int): String = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ protected ExecutableNode parse(InlineParsingRequest request) throws InlineParsin
null);
}

var module = ensoRootNode.getModuleScope().getModule();
var module = ensoRootNode.getModule();
var localScope = ensoRootNode.getLocalScope();
var outputRedirect = new ByteArrayOutputStream();
var redirectConfigWithStrictErrors =
Expand Down Expand Up @@ -265,7 +265,10 @@ protected ExecutableNode parse(InlineParsingRequest request) throws InlineParsin
var m = org.enso.interpreter.runtime.Module.fromCompilerModule(mod);
var toTruffle =
new IrToTruffle(
context, request.getSource(), m.getScope(), redirectConfigWithStrictErrors);
context,
request.getSource(),
m.getScopeBuilder(),
Copy link
Member

Choose a reason for hiding this comment

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

This should request newScopeBuilder(), fill it and build it (install it) to the Module.

redirectConfigWithStrictErrors);
exprNode = toTruffle.runInline(ir, sco, "<inline_source>");
} else {
exprNode = null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ static Type findTypeUncached(Object obj) {
}

final Function findSymbol(String symbol, Type type) {
var unresolved = UnresolvedSymbol.build(symbol, type.getDefinitionScope());
var unresolved = UnresolvedSymbol.build(symbol, type.getDefinitionScopeBuilder());
var found = unresolved.resolveFor(this, type);
if (found != null) {
return found.getLeft();
Expand Down Expand Up @@ -187,7 +187,7 @@ private Object doDispatch(
InteropConversionCallNode convertNode,
InvokeFunctionNode invokeNode)
throws PanicException {
var convert = UnresolvedConversion.build(thatType.getDefinitionScope());
var convert = UnresolvedConversion.build(thatType.getDefinitionScopeBuilder());

var ctx = EnsoContext.get(this);
var state = State.create(ctx);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public class ClosureRootNode extends EnsoRootNode {
ClosureRootNode(
EnsoLanguage language,
LocalScope localScope,
ModuleScope moduleScope,
ModuleScope.Builder moduleScope,
ExpressionNode body,
SourceSection section,
String name,
Expand Down Expand Up @@ -54,7 +54,7 @@ public class ClosureRootNode extends EnsoRootNode {
public static ClosureRootNode build(
EnsoLanguage language,
LocalScope localScope,
ModuleScope moduleScope,
ModuleScope.Builder moduleScope,
Copy link
Member

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?

ExpressionNode body,
SourceSection section,
String name,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import org.enso.compiler.context.LocalScope;
import org.enso.interpreter.EnsoLanguage;
import org.enso.interpreter.runtime.EnsoContext;
import org.enso.interpreter.runtime.Module;
import org.enso.interpreter.runtime.error.DataflowError;
import org.enso.interpreter.runtime.scope.ModuleScope;
import org.enso.interpreter.util.ScalaConversions;
Expand All @@ -21,7 +22,7 @@ public abstract class EnsoRootNode extends RootNode {
private final int sourceStartIndex;
private final int sourceLength;
private final LocalScope localScope;
private final ModuleScope moduleScope;
private final ModuleScope.Builder moduleScope;
private final Source inlineSource;

/**
Expand All @@ -36,7 +37,7 @@ public abstract class EnsoRootNode extends RootNode {
protected EnsoRootNode(
EnsoLanguage language,
LocalScope localScope,
ModuleScope moduleScope,
ModuleScope.Builder moduleScope,
String name,
SourceSection sourceSection) {
super(language, buildFrameDescriptor(localScope));
Expand Down Expand Up @@ -119,10 +120,7 @@ static SourceSection findSourceSection(final RootNode n, int sourceStartIndex, i
if (rootNode.sourceStartIndex == NO_SOURCE) {
return null;
} else {
return rootNode
.getModuleScope()
.getModule()
.createSection(sourceStartIndex, sourceLength);
return rootNode.getModule().createSection(sourceStartIndex, sourceLength);
}
} else {
return rootNode.inlineSource.createSection(sourceStartIndex, sourceLength);
Expand All @@ -145,10 +143,18 @@ public LocalScope getLocalScope() {
*
* @return the module scope for this node
*/
public ModuleScope getModuleScope() {
public ModuleScope.Builder getModuleScope() {
hubertp marked this conversation as resolved.
Show resolved Hide resolved
return moduleScope;
Copy link
Member

Choose a reason for hiding this comment

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

Is it correct that getModuleScope() remains here? Shouldn't we have just getModule().getModuleScope() - or is it expected that when a ModuleScope changes a new EnsoRootNode is created?

}

public ModuleScope getMaterializedScope() {
return moduleScope.build();
}

public Module getModule() {
return moduleScope.getModule();
}

/**
* Marks this node as instrumentable by Truffle instruments API.
*
Expand Down