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

Generate native image for engine-runner #3638

Merged
merged 21 commits into from
Sep 22, 2022
Merged

Conversation

hubertp
Copy link
Contributor

@hubertp hubertp commented Aug 9, 2022

Pull Request Description

This PR adds a possibility to generate native-image for engine-runner.
Note that due to on-demand loading of stdlib, programs that make use of it are currently not yet supported
(that will be resolved at a later point).
The purpose of this PR is only to make sure that we can generate a bare minimum runner because due to lack TruffleBoundaries or misconfiguration in reflection config, this can get broken very easily.
To generate a native image simply execute:

sbt> engine-runner-native/buildNativeImage
... (wait a few minutes)

The executable is called runner and can be tested via a simple test that is in the resources. To illustrate the benefits
see the timings difference between the non-native and native one:

>time built-distribution/enso-engine-0.0.0-dev-linux-amd64/enso-0.0.0-dev/bin/enso --no-ir-caches --in-project test/Tests/ --run engine/runner-native/src/test/resources/Factorial.enso 6
720

real	0m4.503s
user	0m9.248s
sys	0m1.494s
> time ./runner --run engine/runner-native/src/test/resources/Factorial.enso 6
720

real	0m0.176s
user	0m0.042s
sys	0m0.038s

Important Notes

Notice that due to a bug in GraalVM, which is already fixed in 22.x, and us still being on 21.x for the time being, I had to add a workaround to our sbt build to build a different fat jar for native image. To workaround it I had to exclude sqlite jar. Hence native image task is on engine-runner-native and not on engine-runner.

Will need to add the above command to CI.

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the
    Scala,
    Java,
    and
    Rust
    style guides.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH
      ./run ide dist and ./run ide watch.

@JaroslavTulach
Copy link
Member

JaroslavTulach commented Aug 11, 2022

What's the sbt command to invoke the build? I assume it is:

project engine-runner
buildNativeImage

@JaroslavTulach
Copy link
Member

The build says:

Use the option --initialize-at-run-time=io.methvin.watchservice.jna.CarbonAPI to explicitly request delayed initialization of this class.

where shall I put the option?

"runner",
staticOnLinux = true,
additionalOptions = Seq(
"-Dorg.apache.commons.logging.Log=org.apache.commons.logging.impl.NoOpLog",
Copy link
Member

Choose a reason for hiding this comment

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

Probably here one can put additional options.

}
}

@CompilerDirectives.TruffleBoundary
Copy link
Member

Choose a reason for hiding this comment

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

Without @TruffleBoundary getting errors like the one below:

Blocklisted method
  sun.nio.fs.UnixNativeDispatcher.unlinkat0(int, long, int)
called from
  sun.nio.fs.UnixNativeDispatcher.unlinkat(UnixNativeDispatcher.java:171)
  sun.nio.fs.UnixChannelFactory.open(UnixChannelFactory.java:289)
  sun.nio.fs.UnixChannelFactory.newFileChannel(UnixChannelFactory.java:143)
  sun.nio.fs.UnixChannelFactory.newFileChannel(UnixChannelFactory.java:156) -> sun.nio.fs.UnixFileSystemProvider.newByteChannel(UnixFileSystemProvider.java:217)
  java.nio.file.spi.FileSystemProvider.newOutputStream(FileSystemProvider.java:478)
  java.nio.file.Files.newOutputStream(Files.java:220)
  org.enso.downloader.archive.Archive$$anon$1.$anonfun$extractTo$1(Archive.scala:207) -> org.enso.downloader.archive.Archive$$anon$1$$Lambda$13092/0x00000007c3f13040.apply(Unknown Source)
  scala.collection.immutable.LazyList.scala$collection$immutable$LazyList$$state$lzycompute(LazyList.scala:259)
  scala.collection.immutable.LazyList.scala$collection$immutable$LazyList$$state(LazyList.scala:252) -> scala.collection.immutable.LazyList.isEmpty(LazyList.scala:269)
  scala.collection.IterableOnceOps.mkString(IterableOnce.scala:1128)
  scala.collection.IterableOnceOps.mkString$(IterableOnce.scala:1127) -> scala.collection.AbstractIterable.mkString(Iterable.scala:926) -> scala.collection.IterableOnceOps.mkString(IterableOnce.scala:1142) -> scala.collection.IterableOnceOps.mkString$(IterableOnce.scala:1142) -> scala.collection.AbstractIterable.mkString(Iterable.scala:926) -> io.circe.DecodingFailure.getMessage(Error.scala:62)
  java.lang.Throwable.getLocalizedMessage(Throwable.java:396)
  java.lang.Throwable.toString(Throwable.java:485)
  java.lang.Throwable.<init>(Throwable.java:316)
  java.lang.Exception.<init>(Exception.java:103) -> java.lang.RuntimeException.<init>(RuntimeException.java:96) -> java.util.concurrent.atomic.AtomicReferenceFieldUpdater$AtomicReferenceFieldUpdaterImpl.throwAccessCheckException(AtomicReferenceFieldUpdater.java:427)
  java.util.concurrent.atomic.AtomicReferenceFieldUpdater$AtomicReferenceFieldUpdaterImpl.accessCheck(AtomicReferenceFieldUpdater.java:410) -> java.util.concurrent.atomic.AtomicReferenceFieldUpdater$AtomicReferenceFieldUpdaterImpl.compareAndSet(AtomicReferenceFieldUpdater.java:440)
  scala.collection.concurrent.TrieMap.CAS_ROOT(TrieMap.scala:750) -> scala.collection.concurrent.TrieMap.RDCSS_Complete(TrieMap.scala:770)
  scala.collection.concurrent.TrieMap.RDCSS_READ_ROOT(TrieMap.scala:758)
  scala.collection.concurrent.TrieMap.lookuphc(TrieMap.scala:817)
  scala.collection.concurrent.TrieMap.get(TrieMap.scala:906)
  org.enso.compiler.PackageRepository$Default.getLoadedModule(PackageRepository.scala:530)
  org.enso.interpreter.runtime.scope.TopLevelScope.getModule(TopLevelScope.java:58)
  org.enso.interpreter.runtime.scope.TopLevelScope$InvokeMember.getModule(TopLevelScope.java:118)
  org.enso.interpreter.runtime.scope.TopLevelScope$InvokeMember.doInvoke(TopLevelScope.java:171)
  org.enso.interpreter.runtime.scope.TopLevelScopeGen$InteropLibraryExports$Cached.invokeMember(TopLevelScopeGen.java:71)
  com.oracle.truffle.polyglot.PolyglotValueDispatch$InteropValue$AbstractInvokeNode.executeShared(PolyglotValueDispatch.java:4409)
  com.oracle.truffle.polyglot.PolyglotValueDispatch$InteropValue$InvokeNoArgsNode.executeImpl(PolyglotValueDispatch.java:4473)
  com.oracle.truffle.polyglot.HostToGuestRootNode.execute(HostToGuestRootNode.java:127)
  org.graalvm.compiler.truffle.runtime.OptimizedCallTarget.executeRootNode(OptimizedCallTarget.java:650)
  org.graalvm.compiler.truffle.runtime.OptimizedCallTarget.profiledPERoot(OptimizedCallTarget.java:622)

@JaroslavTulach JaroslavTulach changed the title native image for runner native image for engine-runner Aug 22, 2022
@JaroslavTulach
Copy link
Member

JaroslavTulach commented Sep 12, 2022

If the b4292d3 commit is combined with #3696 (and also bugfix for oracle/graal#4200 - e.g. oracle/graal#4403) then the native image compiled runtime can execute:

fac n =
    facacc n v = if n <= 1 then v else @Tail_Call facacc n-1 n*v
    
    res = facacc n 1
    res
    
main =
    fac 6

and print result. Useful when testing the final runner executable in CI, I think.

@hubertp hubertp changed the title native image for engine-runner Generate native image for engine-runner Sep 21, 2022
@hubertp hubertp marked this pull request as ready for review September 21, 2022 08:07
@hubertp hubertp requested a review from 4e6 as a code owner September 21, 2022 08:07
@hubertp
Copy link
Contributor Author

hubertp commented Sep 21, 2022

Updated description and it's ready for final review. Will ping @mwu-tow separately to get that into our CI

Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

All the added @TruffleBoundary annotations are great to have. With them placed at proper places we can start measuring the performance for real. Btw. did you try to compare benchmarks? @TruffleBoundary may have some effect (negative as well as positive).

// sqlite-jdbc jar is problematic and had to be exluded for the purposes of building a native
// image of the runner.
lazy val `engine-runner-native` = project
.in(file("engine/runner-native"))
Copy link
Member

Choose a reason for hiding this comment

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

Creating new project is OK. Especially right now, when we need to tweak the dependencies & co.

@@ -0,0 +1,20 @@
[
{
"name":"java.lang.Boolean",
Copy link
Member

Choose a reason for hiding this comment

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

Placing config files in runner-native is sane as well.

engine/runner-native/src/test/resources/Factorial.enso Outdated Show resolved Hide resolved
@hubertp
Copy link
Contributor Author

hubertp commented Sep 21, 2022

All the added @TruffleBoundary annotations are great to have. With them placed at proper places we can start measuring the performance for real. Btw. did you try to compare benchmarks? @TruffleBoundary may have some effect (negative as well as positive).

I will be checking that today

@hubertp hubertp added the CI: Ready to merge This PR is eligible for automatic merge label Sep 22, 2022
@hubertp
Copy link
Contributor Author

hubertp commented Sep 22, 2022

Run benchmarks - results were good, if not better

JaroslavTulach and others added 10 commits September 22, 2022 14:25
Some changes slipped through rebase. Mostly new/changed
TruffleBoundaries.
More reflection calls were added in the meantime and had to re-run
native image profiler. This also revealed more missing truffle
boundaries.
To workaround a bug in abstract classes that manifests itself in sqlite
dependency we have to exclude it from classpath when building the native
image.
Unfortunately assembly plugin does not allow for specifying this
particular setting only for native image generation, therefore had to
define a separate project just for that.
Once we upgrade we can move it back to `engine-runner`.
To invoke native image generation use
`sbt> engine-runner-native/buildNativeImage`
Once native image is generated, simply execute
```
> ./runner --run engine/runner-native/src/test/resources/Factorial.enso
```
Any additional arguments to `--run` will be passed to `main` function.
Additionally, we will perform an opportunistic conversion of arguments
to int, to avoid string conversion in Enso main. This is because atm we
can't import stdlib to do that.
@mergify mergify bot merged commit 096fcfe into develop Sep 22, 2022
@mergify mergify bot deleted the wip/hubert/engine-native branch September 22, 2022 14:45
```

The task that generates the Native Image, along with all the necessary
configuration, reside in a separate project due to a bug in the currently used
Copy link
Member

@JaroslavTulach JaroslavTulach Sep 26, 2022

Choose a reason for hiding this comment

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

The bug is oracle/graal#4200 - already fixed. As soon as we upgrade to 22.2+ - e.g. #3663 - we can avoid excluding SQLite dependencies.

and execute the binary on a sample factorial test program

```
> runner --run engine/runner-native/src/test/resources/Factorial.enso 6
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the runner be renamed to enso?

.unsafeWrapArray(additionalArgs)
.map(arg =>
try {
Integer.valueOf(arg)
Copy link
Member

@JaroslavTulach JaroslavTulach Sep 26, 2022

Choose a reason for hiding this comment

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

This is surprising, non-Unix like behavior. Turns out we are OK with strings, following snippet works without Integer conversion:

polyglot java import java.lang.Integer

fac n =
    facacc n v = if n <= 1 then v else @Tail_Call facacc n-1 n*v
    
    res = facacc n 1
    res
    
main number =
    fac <| Integer.valueOf number

java.lang.Integer seems to be registered in reflect-config.json already. Anyway this concept of arguments needs a bit of re-think. I get following when providing wrong number of arguments:

$ ./runner --run engine/runner-native/src/test/resources/Factorial.enso
org.enso.interpreter.runtime.callable.function.Function@179b94be
$ ./runner --run engine/runner-native/src/test/resources/Factorial.enso 41 1
Execution finished with an error: Type error: expected a function, but got Integer.

probably we should pass in Vector[String].

@@ -82,9 +82,6 @@ object EnvironmentCheck {
(state: State) => {
val newState = oldTransition(state)
val logger = newState.log
val graalOk = graalVersionOk(graalVersion, javaVersion, logger)
Copy link
Member

@JaroslavTulach JaroslavTulach Nov 21, 2022

Choose a reason for hiding this comment

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

I was just playing with an upgrade of GraalVM - #3663 and got a failure:

[error] /engine/runtime/src/main/scala/org/enso/compiler/SerializationManager.scala:54:11: value createSystemThread is not a member of com.oracle.truffle.api.TruffleLanguage.Env
[error]       env.createSystemThread(runnable)
[error]           ^
[error] /engine/runtime/src/main/scala/org/enso/compiler/SerializationManager.scala:44:15: private val env in class SerializationManager is never used
[error]   private val env = compiler.context.getEnvironment
[error]               ^
[error] /engine/runtime/src/main/scala/org/enso/interpreter/runtime/scope/LocalScope.scala:145:40: type Builder is not a member of object com.oracle.truffle.api.frame.FrameDescriptor
[error]     descriptorBuilder: FrameDescriptor.Builder
[error]                                        ^
[error] /engine/runtime/src/main/scala/org/enso/interpreter/runtime/scope/LocalScope.scala:158:45: value newBuilder is not a member of object com.oracle.truffle.api.frame.FrameDescriptor
[error]     val descriptorBuilder = FrameDescriptor.newBuilder()
[error]                                             ^
[error] four errors found
[error] (runtime / Compile / compileIncremental) Compilation failed

Obviously caused by me running on 21.3 GraalVM and not GraalVM 22.3.

Looking for the cause I realized the GraalVM check has been disabled by this PR. What was the reason, @hubertp? CCing @Akirathan

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking for the cause I realized the GraalVM check has been disabled by this PR. What was the reason, @hubertp? CCing @Akirathan

Can't really remember TBH. I probably wouldn't touch it unless I had to, though @JaroslavTulach.
Definitely worth trying to bring it back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants