Native Language Server integration with PM #11880

hubertp · 2024-12-16T15:43:45Z

Pull Request Description

The first attempt at

Building enso native-image that includes full Language Server
Integrating the executable as an experimental feature during project startup

The change (for now) assumes that enso executable appears in Enso's default engines directory.
To build and run the new integration one has to

engine-runner/buildNativeImage
Run PM with --native-language-server

This change also adds a copy of some of logback's code (a simple socket server) as it was impossible to debug serialization bugs without some additional logging. Potentially to be removed in the final PR.

This is by no means a final change in this area. We need to make it possible to include all other Standard libs, at least. But it's a step forward that allows devs to experiment.

Important Notes

There is a change in JsonConnectionController which only sets the controller as initialized when all resources are set up. Previously, it seems, the asynchronous setup could lead to some race conditions.

Exchange between LS and GUI (before):

(after)

As one can see, speedup in startup is rather massive.

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.

The first attempt at a) building `enso` native-image that includes full Language Server b) integrating the executable as an experimental feature during project startup The change (for now) assumes that `enso` executable appears in Enso's default `engines` directory. To build and run the new integration one has to a) `engine-runner/buildNativeImage` b) run PM with `--native-language-server` This change also adds a copy of some of logback's code (`SocketAppender` or a simple socket server`) as it was impossible to debug serialization bugs without some additional logging.

Otherwise Akka and other code is simply dead-code eliminated in native-image.

When message transport is missing, context should be properly built, even in AOT mode.

Problems revealed after a clean build

JaroslavTulach

I am glad to see language server startup improvement
that's what native image technology is good for and I am glad to see it delivers on its promise
for a while I was confused by the dependency changes
looks like it is mostly to access TruffleOptions.AOT from the non-Truffle part of our codebase
please keep Truffle API out of the dependencies of launcher, boot, etc. projects
use Boolean.getBoolean("com.oracle.graalvm.isaot") directly
I suggest to avoid introduction of --native-... CLI options
- we may not need them at all at this stage
- just prefer bin/enso or bin\enso.exe native binary when it exists
- there already is well functioning --jvm option
- we may want to expose it from project manager CLI, but
- it is already accessible via ENSO_OPTS and that may be enough for now
I suggest to separate separate *-config.json and put them into language-server source tree
I am glad to see the Launching Enso programs instantly #10121 work moving forward

JaroslavTulach · 2025-01-03T05:48:05Z

build.sbt

    Compile / moduleDependencies ++= {
      Seq(
        "org.graalvm.polyglot" % "polyglot"  % graalMavenPackagesVersion,
        "org.slf4j"            % "slf4j-api" % slf4jVersion
-      )
+      ) ++ GraalVM.modules.map(_.withConfigurations(Some(Runtime.name)))


The ScalaDoc of engine-common project which is available few lines above this change notes:

As such it needs to depend on org.graalvm.polyglot package. Otherwise
its dependencies shall be limited

As far as I can see GraalVM.modules contains Truffle API and that's not needed to boot Context - in fact that is an API that users (those not implementing a language) of the GraalVM shouldn't see at all.

What is the reason for these dependency changes?

JaroslavTulach · 2025-01-03T05:55:53Z

build.sbt

+        "commons-io"             % "commons-io"              % commonsIoVersion,
+        "com.google.flatbuffers" % "flatbuffers-java"        % flatbuffersVersion,
+        "org.eclipse.jgit"       % "org.eclipse.jgit"        % jgitVersion,
+        "org.netbeans.api"       % "org-openide-util-lookup" % netbeansApiVersion


I don't think we need org-openide-util-lookup in the runtime. There is:

Replace (NetBeans) Lookup to speed startup by ~100ms #10972

however the Lookup is used only in class initializer - e.g. it may be needed at build time of NI

hopefully this NetBeans API isn't needed during runtime then

What is the failure you see when the dependency is missing?

If we only need @ServiceProvider annotation, then the org-openide-util-lookup library should only be % "provided".

The dependency has already been part of libraryDependencies and marked with provided classifier, yes.

I was getting class not found exception.

Also, it's compile-time scope of moduleDependecies so that should be alright.

CCing @Akirathan. I am not really sure about "compile time only" behavior of Compile / moduleDependencies. As far as I know Compile / moduleDependencies puts the JAR on --module-path as unnamed module (because org-openide-util-lookup isn't Java platform module). I don't think there is a reason for that - having it on classpath should be enough if we only need its annotation processor. Moreover having it on classpath with provided status prevents it to be included in the runtime JARs then.

It is a minor issue, but given:

all the changes with ServiceProvider are only a workaround for [GR-49635] [GR-59954] Update ClassLoaderValue maps to properly support modular service loading oracle/graal#10202

maybe we could just create META-INF/services/org.enso.languageserver.boot.LanguageServerRunner manually

such a file would be easier to delete once we update to new version of GraalVM

JaroslavTulach · 2025-01-03T05:58:12Z

build.sbt

@@ -2280,16 +2283,21 @@ lazy val `language-server` = (project in file("engine/language-server"))
      "org.eclipse.jgit"    % "org.eclipse.jgit"        % jgitVersion,
      "org.apache.tika"     % "tika-core"               % tikaVersion               % Test
    ),
+    libraryDependencies ++= GraalVM.modules.map(


Isn't language-server also a "client of GraalVM" - e.g. it shouldn't really need dependency on Truffle APIs? Not even runtime one?

JaroslavTulach · 2025-01-03T05:59:19Z

build.sbt

@@ -3705,6 +3717,7 @@ lazy val `engine-runner` = project
              "-H:IncludeResources=.*Main.enso$",
              "-H:+AddAllCharsets",
              "-H:+IncludeAllLocales",
+              "-H:+UnlockExperimentalVMOptions",


What are the options we unlocking?

JaroslavTulach · 2025-01-03T06:03:42Z

engine/common/src/main/java/org/enso/common/ContextFactory.java

@@ -228,6 +232,19 @@ public Context build() {
          .allowCreateThread(true);
    }

+    if (engineOptions != null) {
+      // In AOT mode one must not use a shared engine; the latter causes issues when initializing
+      // message transport - it is set to `null`.


Interesting. I don't see a reason wny transport shouldn't work. Maybe it is a bug. Do we have a (small) reproducer that we could report to GraalVM guys and find out what they think?

...er/src/main/java/org/enso/interpreter/instrument/runtime/server/RuntimeServerInstrument.java

JaroslavTulach · 2025-01-03T07:03:30Z

...er/src/main/java/org/enso/interpreter/instrument/runtime/server/RuntimeServerInstrument.java

@@ -108,7 +108,7 @@ public void onContextClosed(TruffleContext context) {}
  @Override
  protected void onCreate(Env env) {
    this.env = env;
-    env.registerService(this);
+
    if (TruffleOptions.AOT) {


The usage of TruffleOptions.AOT is correct here as this is a Truffle instrument and it needs dependency on Truffle API.

JaroslavTulach · 2025-01-03T07:20:51Z

lib/scala/project-manager/src/main/scala/org/enso/projectmanager/boot/Cli.scala

+    val native: cli.Option = cli.Option.builder
+      .longOpt(NATIVE_OPTION)
+      .desc(
+        "(experimental) Attempts to use the native-image of any subprocess."


Can't we just use the bin/enso or bin/enso.exe if it is generated without any (even experimental) option?

Btw. there already is support for --jvm option. We should align with it. Maybe just expose --jvm as an option to project-manager and just pass it on to bin/enso?

Anyway the same can already be done with ENSO_OPTS environment variable and it may be enough for development purposes.

JaroslavTulach · 2025-01-03T07:30:55Z

...version-manager/src/main/scala/org/enso/runtimeversionmanager/runner/NativeJavaCommand.scala

+    val dm       = new DistributionManager(env)
+    val execName = OS.executableName("enso")
+    val fullExecPath =
+      dm.paths.engines.resolve(version).resolve("bin").resolve(execName)


I'd remove all the support of --native-image options and just checked for existence of this fullExecPath binary (probably check it is not a bash shell) and if present just invoke it. It should be good enough for development purposes, but please note that it is also correct for the future:

the discussion entry outlines we should replace launchers with binary ones

when --jvm option is specified the bin/enso or bin\enso.exe should switch to JVM mode themselves - already implemented since --jvm tries to find Java executable system-wide. #11500

E.g. preferring the enso binary if it exists is the right solution for the future.

JaroslavTulach · 2025-01-03T07:33:56Z

...la/runtime-version-manager/src/main/scala/org/enso/runtimeversionmanager/runner/Runner.scala

+            "--module-path",
+            componentPath.toString,
+            "-m",
+            "org.enso.runner/org.enso.runner.Main"


Once we rely on bin/enso native binaries, we will no longer be able to access Main directly. Such a "JVM switch" will be (and already is) handled by bin/enso when --jvm option is passed to them. E.g. this code should disappear at the end, and project manager shall always call into bin/enso.

Despite being advertised as such, native image doesn't set `com.oracle.graalvm.isaot` and it has to be provided separately if we don't want to use `TruffleOptions`.

JaroslavTulach · 2025-01-03T18:26:40Z

...r/src/main/resources/META-INF/native-image/org/enso/languageserver/serialization-config.json

@@ -0,0 +1,59 @@
+{


hubertp added the CI: No changelog needed Do not require a changelog entry for this PR. label Dec 16, 2024

hubertp added 13 commits December 16, 2024 17:28

nit

61ce655

Add missing reflect config

c4d6873

Otherwise Akka and other code is simply dead-code eliminated in native-image.

Ensure proper Context construction

def1ea7

When message transport is missing, context should be properly built, even in AOT mode.

Add missing Akka entries in reflect-config

339493c

nit

b6015e7

Merge branch 'develop' into wip/hubert/11721-language-server-native

cf8a3ed

Partial revert

96ae8c8

Fix service registration

8883d5c

Problems revealed after a clean build

Merge branch 'develop' into wip/hubert/11721-language-server-native

60f8ffe

Fix Base_Tests in native-image

28650dd

Fix compilation issues

cd0a8d8

Drop Truffle dependency

ebaf50c

fmt

08e1e4f

hubertp marked this pull request as ready for review January 2, 2025 09:18

hubertp requested review from 4e6, JaroslavTulach and Akirathan as code owners January 2, 2025 09:18

Merge branch 'develop' into wip/hubert/11721-language-server-native

5547b49

JaroslavTulach approved these changes Jan 3, 2025

View reviewed changes

hubertp added 2 commits January 3, 2025 15:35

Drop TruffleOptions dependency

1588cec

Despite being advertised as such, native image doesn't set `com.oracle.graalvm.isaot` and it has to be provided separately if we don't want to use `TruffleOptions`.

Move NI config to LS resources

55f6623

JaroslavTulach reviewed Jan 3, 2025

View reviewed changes

...r/src/main/resources/META-INF/native-image/org/enso/languageserver/serialization-config.json

@@ -0,0 +1,59 @@

{

Copy link

Member

JaroslavTulach Jan 3, 2025

Choose a reason for hiding this comment

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

+1

enso-bot bot mentioned this pull request Jan 4, 2025

Text.find slows down with the number of invocations #11859

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Native Language Server integration with PM #11880

Native Language Server integration with PM #11880

hubertp commented Dec 16, 2024 •

edited

Loading

JaroslavTulach left a comment

JaroslavTulach Jan 3, 2025

JaroslavTulach Jan 3, 2025

JaroslavTulach Jan 3, 2025

hubertp Jan 3, 2025

hubertp Jan 3, 2025 •

edited

Loading

JaroslavTulach Jan 4, 2025 •

edited

Loading

JaroslavTulach Jan 3, 2025

JaroslavTulach Jan 3, 2025 •

edited

Loading

JaroslavTulach Jan 3, 2025

JaroslavTulach Jan 3, 2025

JaroslavTulach Jan 3, 2025

JaroslavTulach Jan 3, 2025

JaroslavTulach Jan 3, 2025

JaroslavTulach Jan 3, 2025

Native Language Server integration with PM #11880

Are you sure you want to change the base?

Native Language Server integration with PM #11880

Conversation

hubertp commented Dec 16, 2024 • edited Loading

Pull Request Description

Important Notes

Checklist

JaroslavTulach left a 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

Choose a reason for hiding this comment

hubertp Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

JaroslavTulach Jan 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaroslavTulach Jan 3, 2025 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hubertp commented Dec 16, 2024 •

edited

Loading

hubertp Jan 3, 2025 •

edited

Loading

JaroslavTulach Jan 4, 2025 •

edited

Loading

JaroslavTulach Jan 3, 2025 •

edited

Loading