Stateless parser API #11147

kazcw · 2024-09-20T21:50:00Z

Pull Request Description

Stateless (static) parser interface. Buffer-reuse optimization is now hidden within Parser implementation. Fixes #11121 and prevents similar bugs.

Important Notes

Also simplify EnsoParser API, exposing only a higher-level interface.

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.

Stateless (static) parser interface. Buffer-reuse optimization is now hidden behind JNI FFI implementation. Fixes #11121 and prevents similar bugs.

engine/runtime-parser/src/main/java/org/enso/compiler/core/EnsoParser.java

JaroslavTulach

Allowing use of the Rust parser from multiple threads at (some small cost) will simplify its (clueless) usage from JVM.

engine/runtime-parser/src/main/java/org/enso/compiler/core/EnsoParser.java

...time-instrument-common/src/main/scala/org/enso/interpreter/instrument/ChangesetBuilder.scala

engine/runtime-integration-tests/src/test/scala/org/enso/compiler/test/CompilerTest.scala

lib/rust/parser/generate-java/java/org/enso/syntax2/Parser.java

lib/rust/parser/jni/src/lib.rs

lib/rust/parser/generate-java/java/org/enso/syntax2/Parser.java

This reverts commit 285e2e6.

…parser

kazcw · 2024-09-23T19:54:58Z

I moved the thread-local buffer-recycling logic to the Java bindings, to keep the Rust code threading-agnostic in case we would like to compile it to WASM in the future.

lib/rust/parser/generate-java/java/org/enso/syntax2/Parser.java

…arserless-parser

engine/runtime-parser/src/test/java/org/enso/compiler/core/EnsoParserMultiThreadedTest.java

JaroslavTulach · 2024-09-26T06:21:32Z

engine/runtime-compiler/src/main/scala/org/enso/compiler/Compiler.scala

@@ -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()


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!

Reducing memory leaks in runtime-integration-tests #10793

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

JaroslavTulach · 2024-09-26T06:35:10Z

lib/rust/parser/generate-java/java/org/enso/syntax2/Parser.java

-    } catch (URISyntaxException ex) {
-      root = new File(".").getAbsoluteFile();
+  private static final FinalizationManager finalizationManager = new FinalizationManager();
+  private static final Thread finalizationThread = new Thread(finalizationManager.createRunner());


This thread is unfortunate overhead. These are some options to avoid it:

check the FinalizationManager queue before parsing starts - used by WeakHashMap.exprungeStaleEntries()

use Cleaner

leave the cleanup up to the user - e.g. continue to provide instances and close() operations

Own Thread adds overhead. The Cleaner might be better, but there seems to be a thread per each Cleaner anyway. exprungingStaledParser might be good, but may leave some of the latest parser(s) pending for cleanup. We do get a shutdown callback in the interpreter - e.g. we can explicitly request cleanup of EnsoContext.getCompiler() parser workers...

Best Option? Exprunging stale & Close

Having exprungeStaleParsers() check and a way to trigger it (or request shutdown of all existing) when EnsoContext.shutdown() would do the trick too without need for any special Thread or decisions of GC.

Can we name it? I often look into thread dump and hunting down unnamed ones is a real PITA.

hubertp · 2024-09-26T12:02:22Z

engine/runtime-compiler/src/main/scala/org/enso/compiler/Compiler.scala

@@ -700,7 +696,7 @@ class Compiler(
    * @return A Tree representation of `source`
    */
  def parseInline(source: CharSequence): Tree =
-    ensoCompiler.parse(source)
+    Parser.parse(source)


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?

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.

...time-instrument-common/src/main/scala/org/enso/interpreter/instrument/ChangesetBuilder.scala

kazcw · 2024-09-26T23:10:18Z

...on-tests/src/test/scala/org/enso/interpreter/test/instrument/RuntimeVisualizationsTest.scala

@@ -1351,6 +1351,17 @@ class RuntimeVisualizationsTest extends AnyFlatSpec with Matchers {
      )
    )

+    val attachVisualizationResponses =


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.

lib/rust/parser/generate-java/java/org/enso/syntax2/Parser.java

lib/rust/parser/generate-java/java/org/enso/syntax2/FinalizationManager.java

lib/rust/parser/generate-java/java/org/enso/syntax2/Parser.java

JaroslavTulach · 2024-09-27T06:37:48Z

lib/rust/parser/generate-java/java/org/enso/syntax2/Parser.java

-  private static native ByteBuffer parseTreeLazy(long state, ByteBuffer input);
+      @Override
+      public void run() {
+        freeState(state.getAndSet(0));


I see. This guarantees each Rust pointer is cleaned only once.

JaroslavTulach

I'd call runPendingFinalizers() more frequently. Anyway overall it looks fine.

JaroslavTulach · 2024-09-27T14:08:26Z

What happens when there is a Rust code working on a buffer and some other thread drops it?

kazcw · 2024-09-27T14:13:13Z

What happens when there is a Rust code working on a buffer and some other thread drops it?

The parsing thread "checks out" the state for the duration of the operation:
https://github.com/enso-org/enso/pull/11147/files#diff-e237fc34931809c65628b8bcde58e32add8959b8e66940c46a5f63f557530302R169-R181

So that particular parsing thread's buffer won't be dropped by the freeAll call, but will eventually dropped if the parser becomes unreferenced.

GitHub
Stateless parser API by kazcw · Pull Request #11147 · enso-org/enso
Pull Request Description Stateless (static) parser interface. Buffer-reuse optimization is now hidden within Parser implementation. Fixes #11121 and prevents similar bugs. Important Notes
Also sim...

kazcw self-assigned this Sep 20, 2024

kazcw added the CI: No changelog needed Do not require a changelog entry for this PR. label Sep 20, 2024

kazcw force-pushed the wip/kw/parserless-parser branch from ebd4caf to 234a2c4 Compare September 20, 2024 22:01

kazcw added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Sep 20, 2024

kazcw force-pushed the wip/kw/parserless-parser branch 3 times, most recently from 9c0a0fe to b67db01 Compare September 20, 2024 22:38

Stateless parser API

ce95e4e

Stateless (static) parser interface. Buffer-reuse optimization is now hidden behind JNI FFI implementation. Fixes #11121 and prevents similar bugs.

kazcw force-pushed the wip/kw/parserless-parser branch from e10fb43 to ce95e4e Compare September 21, 2024 00:00

Fix initialization

285e2e6

kazcw force-pushed the wip/kw/parserless-parser branch from f856b49 to 285e2e6 Compare September 21, 2024 04:25

Fix docs

23e594f

JaroslavTulach reviewed Sep 23, 2024

View reviewed changes

engine/runtime-parser/src/main/java/org/enso/compiler/core/EnsoParser.java Show resolved Hide resolved

JaroslavTulach reviewed Sep 23, 2024

View reviewed changes

kazcw added 4 commits September 23, 2024 08:41

Revert "Fix initialization"

94ac9a4

This reverts commit 285e2e6.

WIP

2292362

WIP

851fda9

Merge remote-tracking branch 'origin/develop' into wip/kw/parserless-…

4673a1d

…parser

kazcw marked this pull request as ready for review September 23, 2024 19:56

kazcw requested review from radeusgd, hubertp, Akirathan, farmaazon, vitvakatu, Frizi and AdRiley as code owners September 23, 2024 19:56

JaroslavTulach mentioned this pull request Sep 24, 2024

Parser crashing in native code due to multi-threaded access #11121

Closed

enso-bot bot mentioned this pull request Sep 24, 2024

Initialize JLine terminal asynchronously #8688

Closed

JaroslavTulach reviewed Sep 25, 2024

View reviewed changes

lib/rust/parser/generate-java/java/org/enso/syntax2/Parser.java Outdated Show resolved Hide resolved

Stress test to verify how EnsoParser behaves under multi threaded load

137abbc

JaroslavTulach mentioned this pull request Sep 26, 2024

Test parallelism in EnsoParser. Using synchronized to avoid it. #11174

Merged

2 tasks

JaroslavTulach added 2 commits September 26, 2024 08:03

Merge commit '137abbc237c52965b55311ebcd3624ac5bf46fae' into wip/kw/p…

8bfe195

…arserless-parser

Adjusting the EnsoParserMultiThreadedTest to current API

be6f9d2

JaroslavTulach reviewed Sep 26, 2024

View reviewed changes

engine/runtime-parser/src/test/java/org/enso/compiler/core/EnsoParserMultiThreadedTest.java Show resolved Hide resolved

JaroslavTulach reviewed Sep 26, 2024

View reviewed changes

hubertp reviewed Sep 26, 2024

View reviewed changes

Expunge parsers on allocate or shutdown

5358f32

kazcw marked this pull request as draft September 26, 2024 19:18

kazcw added 2 commits September 26, 2024 13:32

Fix NI

f81679a

Fix flaky test

6edac90

kazcw commented Sep 26, 2024

View reviewed changes

Fix test

732d1f0

kazcw marked this pull request as ready for review September 27, 2024 04:28

enso-bot bot mentioned this pull request Sep 27, 2024

It is impossible to catch Panics in the GUI #9402

Closed

JaroslavTulach reviewed Sep 27, 2024

View reviewed changes

lib/rust/parser/generate-java/java/org/enso/syntax2/Parser.java Outdated Show resolved Hide resolved

JaroslavTulach reviewed Sep 27, 2024

View reviewed changes

lib/rust/parser/generate-java/java/org/enso/syntax2/Parser.java Show resolved Hide resolved

JaroslavTulach reviewed Sep 27, 2024

View reviewed changes

lib/rust/parser/generate-java/java/org/enso/syntax2/FinalizationManager.java Show resolved Hide resolved

JaroslavTulach reviewed Sep 27, 2024

View reviewed changes

lib/rust/parser/generate-java/java/org/enso/syntax2/Parser.java Outdated Show resolved Hide resolved

JaroslavTulach reviewed Sep 27, 2024

View reviewed changes

JaroslavTulach approved these changes Sep 27, 2024

View reviewed changes

Review

d22c8af

kazcw added the CI: Ready to merge This PR is eligible for automatic merge label Sep 27, 2024

mergify bot merged commit 2891981 into develop Sep 27, 2024
42 checks passed

mergify bot deleted the wip/kw/parserless-parser branch September 27, 2024 15:58

kazcw mentioned this pull request Oct 3, 2024

Sporadic no data in visualizations on startup #11189

Open

JaroslavTulach mentioned this pull request Oct 4, 2024

Wip/kw/detect parser misuse #11137

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stateless parser API #11147

Stateless parser API #11147

kazcw commented Sep 20, 2024 •

edited

Loading

JaroslavTulach left a comment

kazcw commented Sep 23, 2024

JaroslavTulach Sep 26, 2024

JaroslavTulach Sep 26, 2024 •

edited

Loading

hubertp Sep 26, 2024

hubertp Sep 26, 2024

kazcw Sep 26, 2024 •

edited

Loading

kazcw Sep 26, 2024 •

edited

Loading

JaroslavTulach Sep 27, 2024

JaroslavTulach Sep 27, 2024

JaroslavTulach left a comment

JaroslavTulach commented Sep 27, 2024

kazcw commented Sep 27, 2024 •

edited by unfurl-links bot

Loading

Stateless parser API #11147

Stateless parser API #11147

Conversation

kazcw commented Sep 20, 2024 • edited Loading

Pull Request Description

Important Notes

Checklist

JaroslavTulach left a comment

Choose a reason for hiding this comment

kazcw commented Sep 23, 2024

JaroslavTulach Sep 26, 2024

Choose a reason for hiding this comment

JaroslavTulach Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Best Option? Exprunging stale & Close

hubertp Sep 26, 2024

Choose a reason for hiding this comment

hubertp Sep 26, 2024

Choose a reason for hiding this comment

kazcw Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

kazcw Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

JaroslavTulach Sep 27, 2024

Choose a reason for hiding this comment

JaroslavTulach Sep 27, 2024

Choose a reason for hiding this comment

JaroslavTulach left a comment

Choose a reason for hiding this comment

JaroslavTulach commented Sep 27, 2024

kazcw commented Sep 27, 2024 • edited by unfurl-links bot Loading

kazcw commented Sep 20, 2024 •

edited

Loading

JaroslavTulach Sep 26, 2024 •

edited

Loading

kazcw Sep 26, 2024 •

edited

Loading

kazcw Sep 26, 2024 •

edited

Loading

kazcw commented Sep 27, 2024 •

edited by unfurl-links bot

Loading