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

Avoid AliasAnalysis for Hello_World.enso #10996

Merged
merged 6 commits into from
Sep 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import org.enso.compiler.pass.analyse.BindingAnalysis
import org.enso.compiler.pass.resolve.MethodDefinitions
import org.enso.persist.Persistance.Reference
import org.enso.pkg.QualifiedName
import org.enso.editions.LibraryName

import java.io.ObjectOutputStream
import scala.annotation.unused
Expand All @@ -26,8 +27,8 @@ import scala.collection.mutable.ArrayBuffer
* @param currentModule the module holding these bindings
*/
case class BindingsMap(
definedEntities: List[DefinedEntity],
currentModule: ModuleReference
private val _definedEntities: List[DefinedEntity],
private var _currentModule: ModuleReference
) extends IRPass.IRMetadata {
import BindingsMap._

Expand All @@ -37,11 +38,38 @@ case class BindingsMap(

/** Other modules, imported by [[currentModule]].
*/
var resolvedImports: List[ResolvedImport] = List()
private var _resolvedImports: List[ResolvedImport] = List()

def definedEntities: List[DefinedEntity] = {
ensureConvertedToConcrete()
_definedEntities
}
def currentModule: ModuleReference = {
ensureConvertedToConcrete()
_currentModule
}
def resolvedImports: List[ResolvedImport] = {
ensureConvertedToConcrete()
_resolvedImports
}
def resolvedImports_=(v: List[ResolvedImport]): Unit = {
_resolvedImports = v
}

/** Set to non-null after deserialization to signal that conversion to concrete values is needed */
private var pendingRepository: PackageRepository = null

/** Symbols exported by [[currentModule]].
*/
var exportedSymbols: Map[String, List[ResolvedName]] = Map()
private var _exportedSymbols: Map[String, List[ResolvedName]] = Map()

def exportedSymbols: Map[String, List[ResolvedName]] = {
ensureConvertedToConcrete()
_exportedSymbols
}
def exportedSymbols_=(v: Map[String, List[ResolvedName]]): Unit = {
_exportedSymbols = v
}

/** @inheritdoc */
override def prepareForSerialization(
Expand All @@ -54,18 +82,18 @@ case class BindingsMap(
override def restoreFromSerialization(
compiler: Compiler
): Option[BindingsMap] = {
val packageRepository = compiler.getPackageRepository
this.toConcrete(packageRepository.getModuleMap)
this.pendingRepository = compiler.getPackageRepository
Some(this)
Copy link
Member Author

@JaroslavTulach JaroslavTulach Sep 6, 2024

Choose a reason for hiding this comment

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

This is the core of the change. Delaying toConcrete until all modules are registered to package repository.

It is also potentially dangerous change. Previously, when the restoreFromSerialization failed, it would return None and that would trigger parsing. Right now we optimistically claim everything is all right and then we may get failures later when someone calls for example ResolvedModule.unsafeModule.

It is probably OK (everything is green; but we aren't testing broken scenarios much), but we should get ready for such reports in the future and fix them when they appear.

}

/** Convert this [[BindingsMap]] instance to use abstract module references.
*
* @return `this` with module references converted to abstract
*/
def toAbstract: BindingsMap = {
val copy = this.copy(currentModule = currentModule.toAbstract)
copy.resolvedImports = this.resolvedImports.map(_.toAbstract)
copy.exportedSymbols = this.exportedSymbols.map { case (key, value) =>
val copy = this.copy(_currentModule = _currentModule.toAbstract)
copy._resolvedImports = this._resolvedImports.map(_.toAbstract)
copy._exportedSymbols = this._exportedSymbols.map { case (key, value) =>
key -> value.map(name => name.toAbstract)
}
copy
Expand All @@ -77,23 +105,51 @@ case class BindingsMap(
* instances
* @return `this` with module references converted to concrete
*/
def toConcrete(moduleMap: ModuleMap): Option[BindingsMap] = {
val newMap = this.currentModule.toConcrete(moduleMap).map { module =>
this.copy(currentModule = module)
private def ensureConvertedToConcrete(): Option[BindingsMap] = {
val r = pendingRepository
if (r != null) {
toConcrete(r, r.getModuleMap).map { b =>
pendingRepository = null
this._currentModule = b._currentModule
this._exportedSymbols = b._exportedSymbols
this._resolvedImports = b._resolvedImports
this
}
} else {
Some(this)
}
}

private def toConcrete(
r: PackageRepository,
moduleMap: ModuleMap
): Option[BindingsMap] = {
val newMap = this._currentModule
.toConcrete(moduleMap)
.map { module =>
this._currentModule = module
this
}

val withImports: Option[BindingsMap] = newMap.flatMap { bindings =>
val newImports = this.resolvedImports.map(_.toConcrete(moduleMap))
val newImports = this._resolvedImports.map { imp =>
imp.targets.foreach { t =>
LibraryName
.fromModuleName(t.qualifiedName.toString())
.foreach(r.ensurePackageIsLoaded(_));
}
imp.toConcrete(moduleMap)
}
if (newImports.exists(_.isEmpty)) {
None
} else {
bindings.resolvedImports = newImports.map(_.get)
bindings._resolvedImports = newImports.map(_.get)
Some(bindings)
}
}

val withSymbols: Option[BindingsMap] = withImports.flatMap { bindings =>
val newSymbols = this.exportedSymbols.map { case (key, value) =>
val newSymbols = this._exportedSymbols.map { case (key, value) =>
val newValue = value.map(_.toConcrete(moduleMap))
if (newValue.exists(_.isEmpty)) {
key -> None
Expand All @@ -105,7 +161,7 @@ case class BindingsMap(
if (newSymbols.exists { case (_, v) => v.isEmpty }) {
None
} else {
bindings.exportedSymbols = newSymbols.map { case (k, v) =>
bindings._exportedSymbols = newSymbols.map { case (k, v) =>
k -> v.get
}
Some(bindings)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import org.enso.common.CompilationStage

import scala.collection.mutable
import java.io.IOException
import java.util.logging.Level

/** Runs imports resolution. Starts from a given module and then recursively
* collects all modules that are reachable from it.
Expand Down Expand Up @@ -46,7 +47,12 @@ final class ImportResolver(compiler: Compiler) extends ImportResolverForIR {
)
(ir, currentLocal)
} catch {
case _: IOException =>
case ex: IOException =>
context.logSerializationManager(
Level.WARNING,
"Deserialization of " + module.getName() + " failed",
ex
Copy link
Member Author

Choose a reason for hiding this comment

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

@hubertp, when this exception is logged, the stack trace isn't printed on the console. What's the recommended way of logging an exception with stacktrace via context.logSerializationManager?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think I'm surprised anymore. It looks like the included application.conf does not redefine the layout of logging, meaning it does not print exceptions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I will file a separate ticket to enable those. Right now the default and only appender is console. We could easily log those to the file but I'm still against printing those in the console.

)
context.updateModule(
current,
u => {
Expand Down Expand Up @@ -85,6 +91,7 @@ final class ImportResolver(compiler: Compiler) extends ImportResolverForIR {

currentLocal.resolvedImports =
resolvedImports ++ resolvedSyntheticImports

val newIr = ir.copy(imports = newImportIRs)
context.updateModule(
current,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ class ExportsResolution(private val context: CompilerContext) {
.flatten
.distinct

bindings.exportedSymbols = List(
val newSymbols = List(
ownEntities,
expSymbolsFromResolvedImps
).flatten.distinct
Expand All @@ -179,6 +179,7 @@ class ExportsResolution(private val context: CompilerContext) {
}
(symbolName, resolvedNames)
}
bindings.exportedSymbols = newSymbols
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
package org.enso.interpreter.caches;

import static org.junit.Assert.assertTrue;

import java.io.ByteArrayOutputStream;
import java.io.File;
import java.io.PrintStream;
import java.util.logging.Level;
import java.util.stream.Collectors;
import org.enso.common.RuntimeOptions;
import org.enso.test.utils.ContextUtils;
import org.graalvm.polyglot.Source;
import org.junit.Test;

public class HelloWorldCacheTest {

@Test
public void loadingHelloWorldTwiceUsesCaching() throws Exception {
var root = new File("../..").getAbsoluteFile();
assertTrue("build.sbt exists at " + root, new File(root, "build.sbt").exists());
var helloWorld = children(root, "test", "Benchmarks", "src", "Startup", "Hello_World.enso");
assertTrue("Hello_World.enso found", helloWorld.exists());

// the first run may or may not use caches
var firstMsgs = executeOnce(helloWorld);
assertTrue("Contains hello world:\n" + firstMsgs, firstMsgs.endsWith("Hello World"));
// after the first run the caches for Hello_World.enso must be generated

// the second run must read Hello_World from its .ir file!
var secondMsgs = executeOnce(helloWorld);
assertTrue("Contains hello world:\n" + secondMsgs, secondMsgs.contains("Hello World"));
assertTrue(
"Properly deserialized:\n" + secondMsgs,
secondMsgs.contains("Deserializing module Hello_World from IR file: true"));
}

private static String executeOnce(File src) throws Exception {
var backLog = new ByteArrayOutputStream();
var log = new PrintStream(backLog);

try (var ctx =
ContextUtils.defaultContextBuilder()
.out(log)
.err(log)
.logHandler(log)
.option(RuntimeOptions.LOG_LEVEL, Level.FINE.getName())
.option(RuntimeOptions.DISABLE_IR_CACHES, "false")
.option(RuntimeOptions.PROJECT_ROOT, findBenchmarks(src).getAbsolutePath())
.build()) {
var code = Source.newBuilder("enso", src).build();
var res = ContextUtils.evalModule(ctx, code, "main");
assertTrue("Result of IO.println is Nothing", res.isNull());
}
return backLog
.toString()
.lines()
.filter(l -> l.toUpperCase().contains("HELLO"))
.collect(Collectors.joining("\n"));
}

private static File children(File f, String... names) {
for (var n : names) {
f = new File(f, n);
}
return f;
}

private static File findBenchmarks(File f) {
for (; ; ) {
if (f.getName().equals("Benchmarks")) {
return f;
}
f = f.getParentFile();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,19 @@ public static Value evalModule(Context ctx, CharSequence src, String name, Strin
var b = Source.newBuilder("enso", src, name);
s = b.buildLiteral();
}
var module = ctx.eval(s);
return evalModule(ctx, s, methodName);
}

/**
* Evaluates the given source as if it was in a module with given name.
*
* @param ctx context to evaluate the module at
* @param src The source code of the module
* @param methodName name of main method to invoke
* @return The value returned from the main method of the unnamed module.
*/
public static Value evalModule(Context ctx, Source src, String methodName) {
var module = ctx.eval(src);
var assocType = module.invokeMember(Module.GET_ASSOCIATED_TYPE);
var method = module.invokeMember(Module.GET_METHOD, assocType, methodName);
return "main".equals(methodName) ? method.execute() : method.execute(assocType);
Expand Down
Loading