-
Notifications
You must be signed in to change notification settings - Fork 334
Use Dual JVM mode to run StdLib Native tests #13570
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
Conversation
Don't think we can merge this until we have a test which runs in JVM mode as well (at least until the new mode is default). |
The removal of jvm: true flag is causing CI failures: All of the tests failing in |
Boundary between
|
To use the Generic JDBC driver, |
|
But Postgres and SQLite are used from Is native mode faster to run? Or just to load? If it is faster to run then I would assume we want as much as possible to run in native mode.
It could load the JDBC driver, but the JDBC driver is used directly by code in What are the abilities and limitations of the bridge between the two JVMs? What can be shared between them, and what can be passed back and forth? |
Native mode (e.g. AOT) vs. JIT
The native mode has different performance characteristics. ![]()
|
if (this.findLibraries != null) { | ||
try { | ||
var iop = InteropLibrary.getUncached(); | ||
var mayBePath = iop.execute(this.findLibraries, libName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With 6af4a5a changeset we can execute:
sbt:enso> runEngineDistribution --jvm
--vm.D=polyglot.enso.classLoading=Standard.Base:hosted,guest
--run test/Image_Tests
174 tests succeeded.
0 tests failed.
0 tests skipped.
1 groups skipped.
because now the "dual JVM" can OpenCV.loadShared()
without any issues.
Snowflake Tests
Friday UpdateThere are
|
…senting ColumnStorage implementation
_ : NullType -> Value_Type.Null | ||
_ : AnyObjectType -> Value_Type.Mixed | ||
proxy -> | ||
if proxy.isNumeric.not then Error.throw "Unknown object "+proxy.to_text else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- temporary serde to mitigate the only failure in tests
[FAILED] [Snowflake] (Upload_Spec) Uploading an in-memory Table: [10/11]
- [FAILED] should not create any table if upload fails [1593ms]
Reason: An unexpected panic was thrown: (Inexhaustive_Pattern_Match.Error IntegerType[bits=BITS_64])
at <enso> Storage.to_value_type(distribution/lib/Standard/Table/0.0.0-dev/src/Internal/Storage.enso:34-52)
at <enso> In_Memory_Column_Implementation.inferred_precise_value_type(distribution/lib/Standard/Table/0.0.0-dev/src/Internal/In_Memory_Column_Implementation.enso:633:9-42)
at <enso> Column.inferred_precise_value_type(distribution/lib/Standard/Table/0.0.0-dev/src/Column.enso:2233:9-60)
- having official serde would be better, @jdunkerley
- something like
StorageType.from(proxy.toString())
We seem to run /runner/_work/enso/enso/target/test-results/Table_Tests/JUnit.xml 7134✅ 26❌ 178⚪ 1116s either we need to get the dual mode working for Hopefully solved by 47f94a0. |
h x = Warning.attach "h("+x.to_text+")" "{x="+x.to_text+"}" | ||
i x = Warning.attach "i("+x.to_text+")" Nothing | ||
f x = | ||
Warning.attach "f("+x.to_text+")" <| Pair.new "A" x+10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this have any semantic effect or is it formatting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is easier to put there a breakpoint in VSCode when the body is on separate line. The semantics remain unchanged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To the best of my knowledge, this should only be formatting. They are still just methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am giving you a blind check approval. I failed to keep up with the whole dual JVM framework implementation. Once I find some decent time and energy, I will give you some post integration feedback.
ENSO_LAUNCHER=test
native image- To effectively exchange huge chunks of memory among the _"dual JVMs"_ ... - introduced by #13570 - ... as needed by _In-Memory table_ implementations, let's rely on _direct byte buffers_ - with this PR we have a way for the two _"dual JVMs"_ to exchange and share the same chunk of memory # Important Notes - care must be taken to work properly with GC & free - holding just the [ByteBuffer instance](#13904 (comment)) doesn't prevent other JVM to GC and release its memory region - holding a pointer to the original `Value` object while working with the `ByteBuffer` should be enough to prevent GC
core ++ stdLibsJars ++ extraNITestLibs.value | ||
}, | ||
extraNITestLibs := Def.taskDyn { | ||
if (GraalVM.EnsoLauncher.test) Def.task { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- those mysterious failures in Update to GraalVM 25 #14019
- could be caused by the dual JVM mode used when testing
- that could be verified by reverting back these lines
- then the test helpers would be included again in
enso
executable whenENSO_LAUNCHER=test
mode is on (hopefully)
Pull Request Description
enso
native image binaryENSO_LAUNCHER=test
will only add-ea
optionENSO_LAUNCHER=test
will not modify classpathtest/Base_Test/polyglot/java/*.jar
are going to be loaded by in this "dual JVM mode"test/Base_Tests
is a good progress to makeStandard.Base
should use hosted JVM and all other libraries should use the guest JVMChecklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,