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

Importing (some of) Standard.Base works from NI runner #9866

Merged
merged 23 commits into from
May 17, 2024

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented May 6, 2024

Pull Request Description

  • part of Standard.Base is compiled into runner executable and can IO.println fast
  • proves Investigate compilation of Standard.Base library with Native Image #9774 is viable
  • achieved by updating list of classes needing reflection by re-running the NI agent for enso --run test/Base_Tests
  • the updated configuration .json files are part of this PR
  • including JAR files of Standard.Base library on -cp when building the engine/runner/buildNativeImage
  • got ENSO_JAVA=espresso ./runner working again by avoiding java.desktop via jlink
  • that requires changes to SnakeYaml - send to upsteam
  • to unblock this PR two Apache licensed SnakeYaml files are temporarily copied to our repository

Checklist

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

  • All code follows the
    Scala,
    Java,
  • Integration test have been updated to use Standard.Base library

@JaroslavTulach JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label May 6, 2024
@JaroslavTulach JaroslavTulach self-assigned this May 6, 2024
@JaroslavTulach JaroslavTulach added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label May 6, 2024
@Akirathan
Copy link
Member

Are you going to experiment with other standard libraries, like Standard.Table as well in this PR? Or are you going to postpone other libs in different PRs? In any case, it would be nice to start increasing test coverage of native-image-produced engine-runner. We used to have only Factorial.enso that did not import anything from Standard.Base.

build.sbt Outdated Show resolved Hide resolved
@JaroslavTulach
Copy link
Member Author

Are you going to experiment with other standard libraries, like Standard.Table as well in this PR? Or are you going to postpone other libs in different PRs?

If this PR gets green, I'd like to integrated it as it improves the usability a lot. However I don't claim this is a fix for #9774. The work will continue. I still want to create a Feature that will scan a library for polyglot java import and register such classes for reflection automatically.

In any case, it would be nice to start increasing test coverage of native-image-produced engine-runner.

Ideally, when we train the native image agent on test/Base_Tests we should be able to run the tests on NI version. Right now something is broken. We want to fix it.

We used to have only Factorial.enso that did not import anything from Standard.Base.

Now we have Factorial.enso that imports something from Standard.Base.

@JaroslavTulach
Copy link
Member Author

If this PR gets green

All's well including Factorial.enso invocation with regular NI runner, but...

there is a failure of Factorial.enso invocation with Espresso built NI runner.

Looks like our Espresso support in NI is broken.

Copy link
Contributor

@hubertp hubertp left a comment

Choose a reason for hiding this comment

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

Rejecting (for now) because I don't want this PR to accidentally get in with the wrong sbt definition.

build.sbt Outdated Show resolved Hide resolved
build.sbt Outdated Show resolved Hide resolved
project/NativeImage.scala Outdated Show resolved Hide resolved
*/
package org.yaml.snakeyaml.introspector;

// remove once https://github.com/snakeyaml/snakeyaml/pull/12 gets integrated
Copy link
Member Author

Choose a reason for hiding this comment

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

Remove this file once snakeyaml/snakeyaml#12 gets integrated.

Copy link
Member Author

Choose a reason for hiding this comment

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

E.g. when upgrading to SnakeYaml 2.3 to be released in ~August. Tracked as

project/GraalVM.scala Outdated Show resolved Hide resolved
project/NativeImage.scala Outdated Show resolved Hide resolved
fac number
main number=5 =
v = fac number
IO.println v
Copy link
Member Author

Choose a reason for hiding this comment

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

Using Standard.Base library calls like IO.println

@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented May 15, 2024

The important parts of this PR are green. Let's get ready for the integration.

We have experimentally demonstrated compiling Standard libraries with native-image gives us three times speedup when starting and allows us to use Standard.Base from ./runner executable to begin with. Let's integrate, unless there are objections!

Copy link
Contributor

@hubertp hubertp left a comment

Choose a reason for hiding this comment

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

I don't like the changes in sbt. It very much doesn't feel like sbt'ish (not your fault) and adding some shell scripts into the project directory does not help things.

Also, what I'm missing in the source code comments (or build documentation) is the concrete command on how to generate all native image config files. I know that can be found somehow via NI documentation but I'd like to reduce friction points.

project/smalljdk.sh Outdated Show resolved Hide resolved
- ditch bash script in favour of platform agnostic solution
- use proper dependencies between tasks to avoid dummy function calls
Copy link
Contributor

@hubertp hubertp left a comment

Choose a reason for hiding this comment

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

I had a go at SBT and the changes are now in a more acceptable state. I know that the previous solution worked but the build setup easily starts to become a spaghetti monster when one starts to add random bash scripts.

@JaroslavTulach please add docs with an exact command on how to regenerate NI configs.

)
}

val exec =
Copy link
Member Author

Choose a reason for hiding this comment

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

Exec in Scala! Amazing, thank you @hubertp!

@JaroslavTulach
Copy link
Member Author

I had a go at SBT and the changes are now in a more acceptable state.

Thank you Hubert.

@JaroslavTulach please add docs with an exact command on how to regenerate NI configs.

This commit contains the information 8cc1b2d

@JaroslavTulach JaroslavTulach merged commit 5c06535 into develop May 17, 2024
36 checks passed
@JaroslavTulach JaroslavTulach deleted the wip/jtulach/NIwithBase9774 branch May 17, 2024 12:42
@JaroslavTulach
Copy link
Member Author

To regenerated Substrate VM reflection metadata as this commit did, use:

JAVA_OPTS="-agentlib:native-image-agent=config-merge-dir=./engine/runner/src/main/resources/META-INF/native-image/org/enso/runner" enso --run test/Base_Tests/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: No changelog needed Do not require a changelog entry for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants