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.

Sign up for GitHub

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

Stateless parser API #11147

Merged
merged 18 commits into from
Sep 27, 2024
Merged

Stateless parser API #11147

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
ce95e4e
Stateless parser API
kazcw Sep 20, 2024
285e2e6
Fix initialization
kazcw Sep 21, 2024
23e594f
Fix docs
kazcw Sep 21, 2024
94ac9a4
Revert "Fix initialization"
kazcw Sep 23, 2024
2292362
WIP
kazcw Sep 23, 2024
851fda9
WIP
kazcw Sep 23, 2024
4673a1d
Merge remote-tracking branch 'origin/develop' into wip/kw/parserless-…
kazcw Sep 23, 2024
b948b2e
Rust memory reclamation
kazcw Sep 25, 2024
8491ce1
Java 11
kazcw Sep 26, 2024
6908129
fmt
kazcw Sep 26, 2024
137abbc
Stress test to verify how EnsoParser behaves under multi threaded load
JaroslavTulach Sep 26, 2024
8bfe195
Merge commit '137abbc237c52965b55311ebcd3624ac5bf46fae' into wip/kw/p…
JaroslavTulach Sep 26, 2024
be6f9d2
Adjusting the EnsoParserMultiThreadedTest to current API
JaroslavTulach Sep 26, 2024
5358f32
Expunge parsers on allocate or shutdown
kazcw Sep 26, 2024
f81679a
Fix NI
kazcw Sep 26, 2024
6edac90
Fix flaky test
kazcw Sep 26, 2024
732d1f0
Fix test
kazcw Sep 27, 2024
d22c8af
Review
kazcw Sep 27, 2024
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
5 changes: 3 additions & 2 deletions engine/runner/src/main/java/org/enso/runner/EnsoLibraryFeature.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import java.nio.file.Path;
import java.util.LinkedHashSet;
import java.util.TreeSet;
import org.enso.compiler.core.EnsoParser;
import org.enso.compiler.core.ir.module.scope.imports.Polyglot;
import org.enso.pkg.PackageManager$;
import org.graalvm.nativeimage.hosted.Feature;
Expand Down Expand Up @@ -45,14 +46,14 @@ public void beforeAnalysis(BeforeAnalysisAccess access) {
*/

var classes = new TreeSet<String>();
try (var parser = new org.enso.compiler.core.EnsoParser()) {
try {
for (var p : libs) {
var result = PackageManager$.MODULE$.Default().loadPackage(p.toFile());
if (result.isSuccess()) {
var pkg = result.get();
for (var src : pkg.listSourcesJava()) {
var code = Files.readString(src.file().toPath());
var ir = parser.compile(code);
var ir = EnsoParser.compile(code);
for (var imp : asJava(ir.imports())) {
if (imp instanceof Polyglot poly && poly.entity() instanceof Polyglot.Java entity) {
var name = new StringBuilder(entity.getJavaName());
Expand Down
14 changes: 5 additions & 9 deletions engine/runtime-compiler/src/main/scala/org/enso/compiler/Compiler.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import org.enso.compiler.phase.exports.{
ExportsResolution
}
import org.enso.syntax2.Tree
import org.enso.syntax2.Parser

import java.io.PrintStream
import java.util.concurrent.{
Expand Down Expand Up @@ -69,7 +70,6 @@ class Compiler(
if (config.outputRedirect.isDefined)
new PrintStream(config.outputRedirect.get)
else context.getOut
private lazy val ensoCompiler: EnsoParser = new EnsoParser()
Copy link
Member

Choose a reason for hiding this comment

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

Looks like the original code was buggy. The Compiler should have implemented Closable interface and call ensoCompiler.close() on the EnsoParser. I am not sure what's the overhead of a single EnsoParser instance and its natively allocated memory, but this must have leaked a lot during unit test execution!

@hubertp did reduce the memory, but the Rust parser native part might now even have been visible in the JVM heap dump - only in RSS...


/** Java accessor */
def getConfig(): CompilerConfig = config
Expand Down Expand Up @@ -598,11 +598,8 @@ class Compiler(
)

val src = context.getCharacters(module)
val idMap = context.getIdMap(module)
val tree = ensoCompiler.parse(src)
val expr =
if (idMap == null) ensoCompiler.generateIR(tree)
else ensoCompiler.generateModuleIr(tree, idMap.values)
val idMap = Option(context.getIdMap(module))
val expr = EnsoParser.compile(src, idMap.map(_.values).orNull)

val exprWithModuleExports =
if (context.isSynthetic(module))
Expand Down Expand Up @@ -685,9 +682,8 @@ class Compiler(
inlineContext: InlineContext
): Option[(InlineContext, Expression)] = {
val newContext = inlineContext.copy(freshNameSupply = Some(freshNameSupply))
val tree = ensoCompiler.parse(srcString)

ensoCompiler.generateIRInline(tree).map { ir =>
EnsoParser.compileInline(srcString).map { ir =>
val compilerOutput = runCompilerPhasesInline(ir, newContext)
runErrorHandlingInline(compilerOutput, newContext)
(newContext, compilerOutput)
Expand All @@ -700,7 +696,7 @@ class Compiler(
* @return A Tree representation of `source`
*/
def parseInline(source: CharSequence): Tree =
ensoCompiler.parse(source)
Parser.parse(source)
Copy link
Contributor

Choose a reason for hiding this comment

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

Having EnsoParser a few lines above and here Parser it really begs a question: "What's the difference?"/"Which one should I use?" Wouldn't it better for EnsoParser.parse to simply forward to Parser.parse to hide that?

Copy link
Contributor Author

@kazcw kazcw Sep 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I prefer not to expose two different levels of abstraction from EnsoParser, so that EnsoParser methods output IR but not Tree. Backend parser-users work with IR exclusively, except the single caller of this parseInline method. The caller uses the Tree API to validate whether an input is inline. If we were to rewrite those few lines of code to use the IR API, we would be able to treat the entire Tree API as an internal implementation detail of the parser.


/** Enhances the provided IR with import/export statements for the provided list
* of fully qualified names of modules. The statements are considered to be "synthetic" i.e. compiler-generated.
Expand Down
6 changes: 1 addition & 5 deletions engine/runtime-compiler/src/main/scala/org/enso/compiler/phase/BuiltinsIrBuilder.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@ import org.enso.compiler.data.CompilerConfig
import org.enso.common.CompilationStage
import org.enso.compiler.phase.exports.ExportsResolution

import scala.util.Using

/** A phase responsible for initializing the builtins' IR from the provided
* source.
*/
Expand Down Expand Up @@ -44,9 +42,7 @@ object BuiltinsIrBuilder {
freshNameSupply = Some(freshNameSupply),
compilerConfig = CompilerConfig(warningsEnabled = false)
)
val initialIr = Using(new EnsoParser) { compiler =>
compiler.compile(module.getCharacters)
}.get
val initialIr = EnsoParser.compile(module.getCharacters)
val irAfterModDiscovery = passManager.runPassesOnModule(
initialIr,
moduleContext,
Expand Down
15 changes: 6 additions & 9 deletions ...e-instrument-common/src/main/scala/org/enso/interpreter/instrument/ChangesetBuilder.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import org.enso.text.editing.{IndexedSource, TextEditor}

import java.util.UUID
import scala.collection.mutable
import scala.util.Using

/** The changeset of a module containing the computed list of invalidated
* expressions.
Expand Down Expand Up @@ -97,14 +96,12 @@ final class ChangesetBuilder[A: TextEditor: IndexedSource](
}

val source = Source.newBuilder("enso", value, null).build
Using(new EnsoParser) { compiler =>
compiler
.generateIRInline(compiler.parse(source.getCharacters()))
.flatMap(_ match {
case ir: Literal => Some(ir.setLocation(oldIr.location))
case _ => None
})
}.get
EnsoParser
JaroslavTulach marked this conversation as resolved.
Show resolved Hide resolved
.compileInline(source.getCharacters())
.flatMap(_ match {
case ir: Literal => Some(ir.setLocation(oldIr.location))
case _ => None
})
}

oldIr match {
Expand Down
42 changes: 4 additions & 38 deletions ...e/runtime-instrument-common/src/test/scala/org/enso/compiler/test/CompilerTestSetup.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,42 +21,6 @@ import org.enso.common.CompilationStage
trait CompilerTestSetup {
// === IR Utilities =========================================================

/** An extension method to allow converting string source code to IR as a
* module.
*
* @param source the source code to convert
*/
implicit private class ToIrModule(source: String) {

/** Converts program text to a top-level Enso module.
*
* @return the [[IR]] representing [[source]]
*/
def toIrModule: Module = {
val compiler = new EnsoParser()
try compiler.compile(source)
finally compiler.close()
}
}

/** An extension method to allow converting string source code to IR as an
* expression.
*
* @param source the source code to convert
*/
implicit private class ToIrExpression(source: String) {

/** Converts the program text to an Enso expression.
*
* @return the [[IR]] representing [[source]], if it is a valid expression
*/
def toIrExpression: Option[Expression] = {
val compiler = new EnsoParser()
try compiler.generateIRInline(compiler.parse(source))
finally compiler.close()
}
}

/** Provides an extension method allowing the running of a specified list of
* passes on the provided IR.
*
Expand Down Expand Up @@ -112,7 +76,7 @@ trait CompilerTestSetup {
* @return IR appropriate for testing the alias analysis pass as a module
*/
def preprocessModule(implicit moduleContext: ModuleContext): Module = {
source.toIrModule.runPasses(passManager, moduleContext)
EnsoParser.compile(source).runPasses(passManager, moduleContext)
}

/** Translates the source code into appropriate IR for testing this pass
Expand All @@ -123,7 +87,9 @@ trait CompilerTestSetup {
def preprocessExpression(implicit
inlineContext: InlineContext
): Option[Expression] = {
source.toIrExpression.map(_.runPasses(passManager, inlineContext))
EnsoParser
.compileInline(source)
.map(_.runPasses(passManager, inlineContext))
}
}

Expand Down
17 changes: 1 addition & 16 deletions engine/runtime-integration-tests/src/test/java/org/enso/compiler/test/CompilerTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,25 +11,10 @@
import org.enso.compiler.core.EnsoParser;
import org.enso.compiler.core.IR;
import org.enso.compiler.core.ir.Module;
import org.junit.AfterClass;
import org.junit.BeforeClass;

public abstract class CompilerTests {

protected static EnsoParser ensoCompiler;

@BeforeClass
public static void initEnsoParser() {
ensoCompiler = new EnsoParser();
}

@AfterClass
public static void closeEnsoParser() throws Exception {
ensoCompiler.close();
}

protected static Module parse(CharSequence code) {
Module ir = ensoCompiler.compile(code);
Module ir = EnsoParser.compile(code);
assertNotNull("IR was generated", ir);
return ir;
}
Expand Down
21 changes: 2 additions & 19 deletions ...time-integration-tests/src/test/java/org/enso/compiler/test/VectorArraySignatureTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,25 +19,10 @@
import org.enso.compiler.core.ir.Name;
import org.enso.compiler.core.ir.expression.Comment;
import org.enso.compiler.core.ir.module.scope.Definition;
import org.junit.AfterClass;
import org.junit.BeforeClass;
import org.junit.Test;
import scala.Function1;

public class VectorArraySignatureTest {
private static EnsoParser ensoCompiler;

@BeforeClass
public static void initEnsoParser() {
ensoCompiler = new EnsoParser();
}

@AfterClass
public static void closeEnsoParser() throws Exception {
ensoCompiler.close();
ensoCompiler = null;
}

@Test
public void testParseVectorAndArray() throws Exception {
var p = Paths.get("../../distribution/").toFile().getCanonicalFile();
Expand Down Expand Up @@ -81,8 +66,7 @@ public FileVisitResult postVisitDirectory(Path t, IOException ioe) throws IOExce
var vectorSrc = Files.readString(vectorAndArray[1]);

var arrayIR =
ensoCompiler
.compile(arraySrc)
EnsoParser.compile(arraySrc)
.preorder()
.filter(
(v) -> {
Expand All @@ -95,8 +79,7 @@ public FileVisitResult postVisitDirectory(Path t, IOException ioe) throws IOExce
})
.head();
var vectorIR =
ensoCompiler
.compile(vectorSrc)
EnsoParser.compile(vectorSrc)
.preorder()
.filter(
(v) -> {
Expand Down
42 changes: 4 additions & 38 deletions engine/runtime-integration-tests/src/test/scala/org/enso/compiler/test/CompilerTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,42 +31,6 @@ trait CompilerTest extends AnyWordSpecLike with Matchers with CompilerRunner
trait CompilerRunner {
// === IR Utilities =========================================================

/** An extension method to allow converting string source code to IR as a
* module.
*
* @param source the source code to convert
*/
implicit class ToIrModule(source: String) {
kazcw marked this conversation as resolved.
Show resolved Hide resolved

/** Converts program text to a top-level Enso module.
*
* @return the [[IR]] representing [[source]]
*/
def toIrModule: Module = {
val compiler = new EnsoParser()
try compiler.compile(source)
finally compiler.close()
}
}

/** An extension method to allow converting string source code to IR as an
* expression.
*
* @param source the source code to convert
*/
implicit class ToIrExpression(source: String) {

/** Converts the program text to an Enso expression.
*
* @return the [[IR]] representing [[source]], if it is a valid expression
*/
def toIrExpression: Option[Expression] = {
val compiler = new EnsoParser()
try compiler.generateIRInline(compiler.parse(source))
finally compiler.close()
}
}

/** Provides an extension method allowing the running of a specified list of
* passes on the provided IR.
*
Expand Down Expand Up @@ -137,7 +101,7 @@ trait CompilerRunner {
* @return IR appropriate for testing the alias analysis pass as a module
*/
def preprocessModule(implicit moduleContext: ModuleContext): Module = {
source.toIrModule.runPasses(passManager, moduleContext)
EnsoParser.compile(source).runPasses(passManager, moduleContext)
}

/** Translates the source code into appropriate IR for testing this pass
Expand All @@ -148,7 +112,9 @@ trait CompilerRunner {
def preprocessExpression(implicit
inlineContext: InlineContext
): Option[Expression] = {
source.toIrExpression.map(_.runPasses(passManager, inlineContext))
EnsoParser
.compileInline(source)
.map(_.runPasses(passManager, inlineContext))
}
}

Expand Down
4 changes: 2 additions & 2 deletions ...ntegration-tests/src/test/scala/org/enso/compiler/test/pass/resolve/GlobalNamesTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ package org.enso.compiler.test.pass.resolve

import org.enso.compiler.Passes
import org.enso.compiler.context.{FreshNameSupply, ModuleContext}
import org.enso.compiler.core.IR
import org.enso.compiler.core.{EnsoParser, IR}
import org.enso.compiler.core.Implicits.AsMetadata
import org.enso.compiler.core.ir.Expression
import org.enso.compiler.core.ir.Function
Expand Down Expand Up @@ -84,7 +84,7 @@ class GlobalNamesTest extends CompilerTest {
|add_one x = x + 1
|
|""".stripMargin
val parsed = code.toIrModule
val parsed = EnsoParser.compile(code)
val moduleMapped = passManager.runPassesOnModule(parsed, ctx, group1)
ModuleTestUtils.unsafeSetIr(both._2, moduleMapped)

Expand Down
29 changes: 17 additions & 12 deletions ...tests/src/test/scala/org/enso/interpreter/test/instrument/RuntimeVisualizationsTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1351,6 +1351,17 @@ class RuntimeVisualizationsTest extends AnyFlatSpec with Matchers {
)
)

val attachVisualizationResponses =
Copy link
Contributor Author

@kazcw kazcw Sep 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This test is failing in CI. It looks to me like it is a nondeterministic failure related to sequentialExecution = false command delays, and not related to this changeset. The observed failure mode is for all execution updates to show the result of executing the post-modification module. I think this would occur if delaying is applied to the attachVisualization commands, and not to the textEdit command, so that the edit is performed before the visualizations are attached.

Copy link
Member

Choose a reason for hiding this comment

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

CCing @4e6

context.receiveNIgnoreExpressionUpdates(2)

attachVisualizationResponses.filter(
_.payload.isInstanceOf[Api.VisualizationAttached]
) shouldEqual List(
Api.Response(requestId, Api.VisualizationAttached()),
Api.Response(requestId, Api.VisualizationAttached())
)

// Modify the file
context.send(
Api.Request(
Api.EditFileNotification(
Expand All @@ -1367,23 +1378,17 @@ class RuntimeVisualizationsTest extends AnyFlatSpec with Matchers {
)
)

val responses =
context.receiveNIgnoreExpressionUpdates(7)
val editFileResponses =
context.receiveNIgnoreExpressionUpdates(5)

responses should contain allOf (
Api.Response(requestId, Api.VisualizationAttached()),
editFileResponses should contain(
context.executionComplete(contextId)
)

responses.filter(
_.payload.isInstanceOf[Api.VisualizationAttached]
) shouldEqual List(
Api.Response(requestId, Api.VisualizationAttached()),
Api.Response(requestId, Api.VisualizationAttached())
)

val visualizationUpdatesResponses =
responses.filter(_.payload.isInstanceOf[Api.VisualizationUpdate])
(attachVisualizationResponses ::: editFileResponses).filter(
_.payload.isInstanceOf[Api.VisualizationUpdate]
)
val expectedExpressionId = context.Main.idMainX
val visualizationUpdates = visualizationUpdatesResponses.map(
_.payload.asInstanceOf[Api.VisualizationUpdate]
Expand Down
Loading
Loading