Run Enso tests with assertions enabled #7882

Akirathan · 2023-09-25T09:11:14Z

Closes:

Enable Assertions in Enso Tests #5585
Invalid Sharing of AST nodes in EpbContext #5087
Simplify EpbLanguage by using TruffleLanguage.ParserRequest.getArgumentNames + support Ruby & co. #5184 by implementing GenericForeignNode

Pull Request Description

This PR removes most of the Enso homemade Proxy functionality in EpbLanguage. Rather than that is uses new features in GraalVM that allow access to OtherContextTruffleObject. That fixes various assert checks in EpbLanguage and runtime-with-polyglot tests. Now the whole Enso system can be executed with -ea checks on.

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

All code follows the
Scala,
Java,
style guides.
All code has been tested:
- Unit tests have been written where possible.

build/build/src/enso.rs

Akirathan · 2023-09-25T13:04:29Z

Encountered AssertionErrors in https://github.com/enso-org/enso/actions/runs/6297283538/job/17093926525?pr=7882:

Examples_Tests: Multiple failures. For example: Array.java:41
Table_Tests: Invalid sharing of AST nodes detected

GitHub
Run Enso tests with assertions enabled · enso-org/enso@a8d28a4
Hybrid visual and textual functional programming. Contribute to enso-org/enso development by creating an account on GitHub.

lib/scala/bench-processor/src/main/java/org/enso/benchmarks/processor/BenchProcessor.java

…nso-Tests

…nso-Tests # Conflicts: # lib/scala/bench-processor/src/main/java/org/enso/benchmarks/processor/BenchProcessor.java

Akirathan · 2023-12-04T09:41:36Z

As of today, I am abandoning the PR once again. I am now stuck on Multi-threaded access requested by thread ... but is not allowed for js.

I have pushed my latest investigation in 09fa463 that contains some testing instruments that might be useful. The simplest way to reproduce the error is just with runtime-with-polyglot> testOnly *RuntimeVisualizationsTest -- -z [1], or runtime-with-polyglot> withDebug --debugger testOnly -- *RuntimeVisualizationsTest -- -z [1]. I have tried to do a global lock of EpbContext but with no success.

cc @JaroslavTulach

JaroslavTulach · 2023-12-12T07:41:48Z

I have pushed my latest investigation in 09fa463 that contains some testing instruments that might be useful. The simplest way to reproduce the error is just with runtime-with-polyglot> testOnly *RuntimeVisualizationsTest -- -z [1], or runtime-with-polyglot> withDebug --debugger testOnly -- *RuntimeVisualizationsTest -- -z [1].

I can simulate the failure. According to last week's discussion with @4e6 - there should be no multi-threaded access to the context. Yet there is. Here is the list of the three getSeenThreads():

Thread[#43,pool-1-thread-1-ScalaTest-running-RuntimeVisualizationsTest,5,main]=com.oracle.truffle.polyglot.PolyglotThreadInfo@64dd5b37[thread=Thread[#43,pool-1-thread-1-ScalaTest-running-RuntimeVisualizationsTest,5,main], enteredCount=0, cancelled=false, leaveAndEnterInterrupted=false], 

Thread[#66,sequential-command-pool-1,5,main]=com.oracle.truffle.polyglot.PolyglotThreadInfo@7ec8efc7[thread=Thread[#66,sequential-command-pool-1,5,main], enteredCount=1, cancelled=false, leaveAndEnterInterrupted=false], 

Thread[#65,job-pool-1,5,main]=com.oracle.truffle.polyglot.PolyglotThreadInfo@61cd1f20[thread=Thread[#65,job-pool-1,5,main], enteredCount=1, cancelled=false, leaveAndEnterInterrupted=false]}

that block access to JavaScript (which claims to be single threaded). If there was just a single thread, the JavaScript initialization would succeed.

engine/runtime-language-epb/src/main/java/org/enso/interpreter/epb/node/ForeignEvalNode.java

…instruments project

JaroslavTulach

I am fine with the changes. Please review as well. Once the CI gets stable, I'd like to integrate.

JaroslavTulach · 2023-12-14T08:18:13Z

running std libs benchmarks to verify influence of such change on startup.

Previous benchmarks timed out (on endless loop in Sieve's Java_Script_Filter test involving is_same_object - e.g 3fa992a). New run scheduled.

hubertp · 2023-12-14T08:24:59Z

...ne/runtime-compiler/src/main/scala/org/enso/compiler/pass/desugar/GenerateMethodBodies.scala

@@ -161,7 +160,7 @@ case object GenerateMethodBodies extends IRPass {
          case lam @ Function.Lambda(_, body, _, _, _, _)
              if findForeignDefinition(
                body,
-                lang = Some(ForeignLanguage.JS)
+                lang = Some("js")


Can we keep that as a constant somewhere, please?

Too much overhead to do so. We would need a new project sharable between all that were using ForeginLanguage.JS. But I want to get rid of the heavyweight polyglot-api dependency (in EpbLanguage at least). I don't want to create new project just to host js constant.

Btw. the real constant is defined in graal-js and we don't compile against such module. E.g. there was never any guarantee (except tests) that we are using the right constant.

engine/runtime-language-epb/src/main/java/org/enso/interpreter/epb/EpbContext.java

hubertp · 2023-12-14T08:30:04Z

engine/runtime-language-epb/src/main/java/org/enso/interpreter/epb/EpbLanguage.java

 @TruffleLanguage.Registration(
-    id = ForeignLanguage.ID,
+    id = "epb",


EpbLanguage constant?

Needless. There is only one use of such a constant. Others are in modules that don't compile against this module:

They wouldn't be able to use the constant anyway. The fact that this all holds together is ensured by tests.

engine/runtime-language-epb/src/main/java/org/enso/interpreter/epb/EpbLanguage.java

engine/runtime-language-epb/src/main/java/org/enso/interpreter/epb/ForeignEvalNode.java

hubertp · 2023-12-14T08:41:27Z

engine/runtime-language-epb/src/main/java/org/enso/interpreter/epb/PyForeignNode.java

+    if (iop.hasMetaObject(res)) {
+      var meta = iop.getMetaObject(res);
+      var name = iop.getMetaQualifiedName(meta);
+      if ("module".equals(name)) {


So we can't use iop.isNull ?

...glot/src/test/java/org/enso/interpreter/test/instrument/VerifyJavaScriptIsAvailableTest.java

Akirathan

OK. Would be nice to get rid of some hardcoded strings though.

build.sbt

project/WithDebugCommand.scala

engine/runtime/src/main/scala/org/enso/interpreter/runtime/IrToTruffle.scala

engine/runtime/src/main/java/org/enso/interpreter/runtime/EnsoContext.java

engine/runtime-language-epb/src/main/java/org/enso/interpreter/epb/EpbContext.java

engine/runtime-language-epb/src/main/java/org/enso/interpreter/epb/JsForeignNode.java

...glot/src/test/java/org/enso/interpreter/test/instrument/VerifyJavaScriptIsAvailableTest.java

JaroslavTulach · 2023-12-14T09:45:28Z

There is a failure in Table_Tests which can be simulated locally as:

enso$ JAVA_OPTS=-ea ./built-distribution/enso-engine-0.0.0-dev-linux-amd64/enso-0.0.0-dev/bin/enso --in-project test/Table_Tests --run test/Table_Tests/src/Common_Table_Operations/Date_Time_Spec.enso

Don't forget that the CI runs with -ea now and you may need to turn it on locally to see the same failure!

radeusgd · 2023-12-14T14:51:38Z

engine/runtime-language-epb/src/main/java/org/enso/interpreter/epb/EpbLanguage.java

-/**
- * An internal language that serves as a bridge between Enso and other supported languages.
- *
- * <p>Truffle places a lot of emphasis on safety guarantees, which also means that single-threaded
- * languages cannot easily be called from multiple threads. We circumvent this by using two separate
- * {@link com.oracle.truffle.api.TruffleContext}s, one (often referred to as "outer") is allowed to
- * run Enso, Host Java, and possibly other thread-ready languages. Languages that cannot safely run
- * in a multithreaded environment are relegated to the other context (referred to as "inner"). The
- * inner context provides a GIL capability, ensuring that access to the single-threaded languages is
- * serialized.
- *
- * <p>This imposes certain limitations on data interchange between the contexts. In particular, it
- * is impossible to execute origin language's code when executing in the other context. Therefore
- * outer context values need to be specially wrapped before being passed (e.g. as arguments) to the
- * inner context, and inner context values need rewrapping for use in the outer context. See {@link
- * org.enso.interpreter.epb.runtime.PolyglotProxy} and {@link ContextRewrapNode} for details of how
- * and when this wrapping is done.
- *
- * <p>With the structure outlined above, EPB is the only language that is initialized in both inner
- * and outer contexts and thus it is very minimal. Its only role is to manage both contexts and
- * provide context-switching facilities.
- */
+/** An internal language that serves as a bridge between Enso and other supported languages. */
 @TruffleLanguage.Registration(


Why is this whole block of documentation removed?

Done by @Akirathan, but I'd say it no longer reflects reality. Times changed and we no longer have to implement this ourselves, but rather rely on Truffle to do it for us.

Btw. @kustosz asked why do we need this EpbLanguage anymore? True, possibly we could do without it.

jdunkerley

Just reviewed the enso files.

JaroslavTulach · 2023-12-14T18:29:37Z

Another benchmarks run scheduled just to be sure - no problems expected however.

JaroslavTulach · 2023-12-15T13:39:46Z

...e/runtime-with-polyglot/src/test/java/org/enso/interpreter/test/ForeignMethodInvokeTest.java

+    assertEquals(12, res2.getArrayElement(2).asInt());
+  }
+
+  @Ignore


I've reported oracle/graal#8033 to describe why this test has to be @Ignored right now.

With onDeniedThreadAccess as proposed by oracle/graal#8266 we have a way to act like an Ethernet on collision and successfully orchestrate the two threads accessing the same context to run into completion.

More info on Ethernet collision detection and behavior - most important "Calculate and wait the random backoff".

Continue discussion at...

JaroslavTulach · 2024-01-05T10:07:34Z

Looks like we have a significant regression in Sieve_Java_Script_Filter benchmark:

Caused by something in between b5c995a...940b8f7 and the most likely cause is this PR. Alas.

Sieve_Java_Script_Natural benchmark hasn't regressed, so the cause isn't switching from Enso to JavaScript (done a lot to increment a number in the Sieve_Java_Script_Natural benchmark), but probably accessing JavaScript objects from Enso (which is what Sieve_Java_Script_Filter does).

Run Enso tests with assertions enabled

a8d28a4

Akirathan self-assigned this Sep 25, 2023

Akirathan commented Sep 25, 2023

View reviewed changes

build/build/src/enso.rs Show resolved Hide resolved

Akirathan added the CI: No changelog needed Do not require a changelog entry for this PR. label Sep 25, 2023

Akirathan mentioned this pull request Sep 26, 2023

Implement Enso-specific assert #7883

Merged

5 tasks

JaroslavTulach and others added 2 commits October 2, 2023 17:53

Avoid assert when using ArrayLikeHelpers.allocate

219ff91

Fix BenchProcessor locating root directory

429c9de

Akirathan commented Nov 17, 2023

View reviewed changes

lib/scala/bench-processor/src/main/java/org/enso/benchmarks/processor/BenchProcessor.java Outdated Show resolved Hide resolved

Akirathan added 3 commits November 17, 2023 18:27

Throw AbstractTruffleException from ArrayLikeAtNode

dc16932

ForeignEvalNode does not enter inner TruffleContext

29d7963

Remove epb PolyglotProxy and co.

f4e5a82

This was linked to issues Nov 17, 2023

Invalid Sharing of AST nodes in EpbContext #5087

Closed

Enable Assertions in Enso Tests #5585

Closed

Akirathan added 3 commits November 23, 2023 12:25

Merge branch 'develop' into wip/akirathan/5585-Enable-Assertions-in-E…

f23fef3

…nso-Tests

Merge branch 'develop' into wip/akirathan/5585-Enable-Assertions-in-E…

5bea01a

…nso-Tests # Conflicts: # lib/scala/bench-processor/src/main/java/org/enso/benchmarks/processor/BenchProcessor.java

[WIP] Try to do global locking on EpbContext

09fa463

JaroslavTulach added 2 commits December 12, 2023 07:59

Merge with develop

5177e0b

Call super.init before accessing executionContext.context

189b335

JaroslavTulach reviewed Dec 12, 2023

View reviewed changes

engine/runtime-language-epb/src/main/java/org/enso/interpreter/epb/node/ForeignEvalNode.java Show resolved Hide resolved

JaroslavTulach added 7 commits December 12, 2023 12:01

Formatting

2302fc8

Still require inner context for JavaScript

6ce3a1b

Just three failures in runtime-with-polyglot

c32d980

Don't modify InstrumentTestContext to avoid failures in runtime-with-…

9d96365

…instruments project

Introducing EpbProxyValue to switch to the right context

88d8db2

Log conditionally

630bf2d

Only dispatch the InteropLibrary messages

f5ba767

enso-bot bot mentioned this pull request Dec 13, 2023

Invalid Sharing of AST nodes in EpbContext #5087

Closed

2 tasks

JaroslavTulach self-requested a review December 14, 2023 07:31

JaroslavTulach approved these changes Dec 14, 2023

View reviewed changes

Avoid using Meta.is_same_object in Sieve implementations

3fa992a

hubertp approved these changes Dec 14, 2023

View reviewed changes

Don't forget to import site to initialize Python's venv

0e0dc6e

Akirathan commented Dec 14, 2023

View reviewed changes

JaroslavTulach added 5 commits December 14, 2023 12:44

Avoid NPE in assert

0e8dc8e

No need for arity

3f63d9e

Disable pre-initialization of JavaScript for now

ef1c798

Reverting debug only comment

8fda9f5

No special test options are needed

8f08e61

mwu-tow approved these changes Dec 14, 2023

View reviewed changes

GregoryTravis approved these changes Dec 14, 2023

View reviewed changes

radeusgd reviewed Dec 14, 2023

View reviewed changes

jdunkerley approved these changes Dec 14, 2023

View reviewed changes

radeusgd approved these changes Dec 14, 2023

View reviewed changes

JaroslavTulach added 2 commits December 14, 2023 19:15

Concentrate parsing into ForeingEvalNode

78d603b

Merging with develop branch

faaffc1

JaroslavTulach added 2 commits December 15, 2023 07:14

Don't convert long to EnsoBigInteger as that asserts

b019877

Associate InstrumentorEvalNode with instance of EnsoLanguage

61422e0

JaroslavTulach force-pushed the wip/akirathan/5585-Enable-Assertions-in-Enso-Tests branch from a7d18c0 to 61422e0 Compare December 15, 2023 11:25

JaroslavTulach merged commit 4b65e44 into develop Dec 15, 2023
29 of 30 checks passed

JaroslavTulach deleted the wip/akirathan/5585-Enable-Assertions-in-Enso-Tests branch December 15, 2023 12:31

JaroslavTulach mentioned this pull request Dec 15, 2023

Let languages control multi threaded access ... is not allowed for language(s) js oracle/graal#8033

Open

JaroslavTulach reviewed Dec 15, 2023

View reviewed changes

JaroslavTulach mentioned this pull request Jan 11, 2024

Arrow language #8512

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run Enso tests with assertions enabled #7882

Run Enso tests with assertions enabled #7882

Akirathan commented Sep 25, 2023 •

edited by JaroslavTulach

Akirathan commented Sep 25, 2023 •

edited by unfurl-links bot

Akirathan commented Dec 4, 2023

JaroslavTulach commented Dec 12, 2023

JaroslavTulach left a comment

JaroslavTulach commented Dec 14, 2023

hubertp Dec 14, 2023

JaroslavTulach Dec 14, 2023

hubertp Dec 14, 2023

JaroslavTulach Dec 14, 2023

hubertp Dec 14, 2023

Akirathan left a comment

JaroslavTulach commented Dec 14, 2023

radeusgd Dec 14, 2023

JaroslavTulach Dec 14, 2023

JaroslavTulach Dec 14, 2023 •

edited

jdunkerley left a comment

JaroslavTulach commented Dec 14, 2023

JaroslavTulach Dec 15, 2023

JaroslavTulach Jan 30, 2024 •

edited

JaroslavTulach commented Jan 5, 2024

Run Enso tests with assertions enabled #7882

Run Enso tests with assertions enabled #7882

Conversation

Akirathan commented Sep 25, 2023 • edited by JaroslavTulach

Pull Request Description

Important Notes

Checklist

Akirathan commented Sep 25, 2023 • edited by unfurl-links bot

Akirathan commented Dec 4, 2023

JaroslavTulach commented Dec 12, 2023

JaroslavTulach left a comment

Choose a reason for hiding this comment

JaroslavTulach commented Dec 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Akirathan left a comment

Choose a reason for hiding this comment

JaroslavTulach commented Dec 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaroslavTulach Dec 14, 2023 • edited

Choose a reason for hiding this comment

jdunkerley left a comment

Choose a reason for hiding this comment

JaroslavTulach commented Dec 14, 2023

Choose a reason for hiding this comment

JaroslavTulach Jan 30, 2024 • edited

Choose a reason for hiding this comment

JaroslavTulach commented Jan 5, 2024

Akirathan commented Sep 25, 2023 •

edited by JaroslavTulach

Akirathan commented Sep 25, 2023 •

edited by unfurl-links bot

JaroslavTulach Dec 14, 2023 •

edited

JaroslavTulach Jan 30, 2024 •

edited