Add Standard.Tableau to native-image #12204

hubertp · 2025-01-31T12:04:35Z

Pull Request Description

Checklist

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

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.

``` [warn] there's a key that's not used by any other settings/tasks: [warn] [warn] * runtime-compiler-dump / javaModuleName [warn] +- /home/hubert/tmp/enso/build.sbt:3425 [warn] [warn] note: a setting might still be used by a command; to exclude a key from this `lintUnused` check [warn] either append it to `Global / excludeLintKeys` or call .withRank(KeyRanks.Invisible) on the key ```

JaroslavTulach · 2025-01-31T17:33:58Z

70MB in resources!? Are we distributing another .so or .dll library?

JaroslavTulach

tableauhperapi library should be extracted from the JAR into polyglot/lib directory.

std-bits/tableau/src/main/resources/META-INF/native-image/org/enso/tableau/resource-config.json

This change extracts `Tableau`'s Hyper shared library from the jar. Unlike `Standard.Image`, had to provide a custom classloader that manages to find requested library - HyperAPI used a custom JNA logic for that.

hubertp · 2025-02-03T14:16:28Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/scope/TopLevelScope.java

+          return libPath;
+        }
+      }
+      throw new UnsupportedOperationException("unable to find library " + libname);


Note: I'm not entirely sure this is the right way but RuntimeException didn't feel right either.
Maybe a PanicException or a simple null?

Open for suggestions.

This method is called only from HyperReader. I suggested implementing finding native library this way, because HyperReader is in std-bits and, therefore, has no (direct) access to EnsoContext and its NativeLibraryFinder. I don't have a strong opinion about the exception. But I suggest you append additional test for it inside NativeLibraryFinderTest.

If we are exposing this for the Enso code, then:

ctx.nothing() might be a reasonable value.

PanicException is a good alternative as well

UnsupportedMessageException is a poor's man solution as well

Certainly not java.lang.UnsupportedOperationException - that would mean crash of the interpreter!

hubertp · 2025-02-03T14:36:07Z

70MB in resources!? Are we distributing another .so or .dll library?

Yes.

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

Akirathan · 2025-02-03T15:50:23Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/scope/TopLevelScope.java

+          return libPath;
+        }
+      }
+      throw new UnsupportedOperationException("unable to find library " + libname);


This method is called only from HyperReader. I suggested implementing finding native library this way, because HyperReader is in std-bits and, therefore, has no (direct) access to EnsoContext and its NativeLibraryFinder. I don't have a strong opinion about the exception. But I suggest you append additional test for it inside NativeLibraryFinderTest.

enso-bot · 2025-02-03T19:41:08Z

Hubert Plociniczak reports a new STANDUP for the provided date (2025-01-31):

Progress: Created PR with Standard.Tableau integration. Looking into false positive log failures reported when closing project. It should be finished by 2025-02-01.

Next Day: Next day I will be working on the #12204 task. Continue investigating the issue.

JaroslavTulach · 2025-02-04T07:01:33Z

std-bits/tableau/src/main/java/org/enso/tableau/HyperReader.java

+    }
+
+    @Override
+    public InputStream getResourceAsStream(String name) {


What is the caller stack trace? What the tableu code is trying to achieve? Why does it need InputStream? Can we prevent it to ask for input stream?

We can't prevent it. It's part of hyper initialization.

My investigation suggests there is a way to prevent the call and skip the "extraction of the JNI library" completely.

JaroslavTulach · 2025-02-04T07:07:40Z

std-bits/tableau/src/main/java/org/enso/tableau/HyperReader.java

+      int dotIdx = name.indexOf(".");
+      if (libIdx != -1 && dotIdx != -1) {
+        var libName = name.substring(libIdx + 3, dotIdx);
+        var bindings = Context.getCurrent().getBindings("enso");


Calling org.graalvm.polyglot from ClassLoader - that's wild.

The only other option how to get reference to the NativeLibraryFinder functionality is to move the NativeLibraryFinder out of org.enso.runtime into some module that is shared with std-bits. The approach via Context.getCurrent().getBindings("enso") is what I have suggested to @hubertp as the "best" solution that I can think of. I am open to other suggestions of course.

Care to expand on what's so wild about it?

what's so wild about it?

ClassLoader is a low level JVM mechanism. Calling from such a level into a Truffle language implemented on top of Truffle framework written in Java is just wild. It's like calling from a Linux kernel driver into X11 paint method and waiting for a result.

Doing something like this is a request to get a deadlock in internal JVM classloading locks.

#yolo

Jokes aside, it would be best to suggest some improvements upstream as those jars are not integration-friendly. But that can take a lot of time and has unknown chances of success.

To avoid this "kernel to graphics" dependency, I'd like to point your attention to NetBeans translate support:

it provides a way to instruct the classloader to do something else

by having a special file that provides a mapping

all NetBeans ClassLoaders support this translating

We could let the Enso tableau.jar provide such a file to instruct the HostClassLoader to return the FileInputStream from getResourceAsStream when a particular resource cannot be found.

The difference to current state is that we don't need to call into polyglot context from a ClassLoading code.

JaroslavTulach · 2025-02-04T07:08:43Z

std-bits/tableau/src/main/java/org/enso/tableau/HyperReader.java

@@ -88,7 +91,7 @@ private static HyperProcess getProcess() throws IOException {
    if (process == null || !process.isOpen()) {
      var contextClassLoader = Thread.currentThread().getContextClassLoader();
      try {
-        Thread.currentThread().setContextClassLoader(HyperReader.class.getClassLoader());
+        Thread.currentThread().setContextClassLoader(new TableauClassLoader());


There seems to be:

$ javap -cp built-distribution/enso-engine-*/enso-*/lib/Standard/Tableau/0.0.0-dev/polyglot/java/tableauhyperapi.jar com.tableau.hyperapi.impl.SharedLibraryProvider Compiled from "SharedLibraryProvider.java" public interface com.tableau.hyperapi.impl.SharedLibraryProvider { public abstract java.lang.String getSharedLibraryBaseName(); }

cannot it be used for loading the JNI library without these classloading tricks? Looks like the code checks jna.nounpack property:

private static void registerJNA(); Code: 0: ldc #52 // String jna.nounpack 2: invokestatic #53 // Method java/lang/Boolean.getBoolean:(Ljava/lang/String;)Z 5: ifeq 17 8: invokestatic #54 // Method getLibraryBaseName:()Ljava/lang/String; 11: invokestatic #55 // Method com/sun/jna/Native.register:(Ljava/lang/String;)V

With a proper value provided, the code should be able to skip this extracting step completely.

Yes, I saw that. Sadly I couldn't make it to work that way; JNA gets in the way.

OK, thanks.

JaroslavTulach

Let me approve in the name of pragmatism. Since my previous review comment this PR managed to:

make the distribution smaller:

avoid duplicating the library between JAR and native executable

Only the

makes the startup faster (no extraction of a resource on start)

remains open. That's probably good enough for now - especially when it is unlikely many people use Tableau to begin with.

I still don't like the "kernel to graphics call", but I can live with it. Moreover I am going to make another proposal to solve it and stay in the kernel space.

Seems to be causing problems on CI. Extraction logic had to be extended to allow for: - different prefixes (e.g, `linux-86-64` instead of `linux-86_64) - classes being present on the prefix should be included in the thin jar; we cannot assume that extraction prefix is unique

enso-bot · 2025-02-05T22:47:14Z

Hubert Plociniczak reports a new STANDUP for the provided date (2025-02-03):

Progress: Addressing PR review - extracting native libs from Tableau's jars. Most problems come from discrepancies between different OS/Architecture. It should be finished by 2025-02-05.

Next Day: Next day I will be working on the #12204 task. Continue investigating the issue.

Alternative approach to deal with Windows/Mac failures on CI where `jnidispatch` native library somehow could not be identified. Linux worked just fine with the previous approach. This change enhances custom classloader to identify the desired library. That way we can provide the path to it, as specified in the implementation of JNA.

enso-bot · 2025-02-05T23:57:58Z

Hubert Plociniczak reports a new STANDUP for the provided date (2025-02-04):

Progress: Continued fighting with native library loading - Tableau relies on JNA which brings its own set of problems. Solved for Linux but Windows and Mac are problematic. Dealing with some CI failures. It should be finished by 2025-02-05.

Next Day: Next day I will be working on the #12204 task. Continue investigating the issue.

enso-bot · 2025-02-06T00:02:44Z

Hubert Plociniczak reports a new STANDUP for yesterday (2025-02-05):

Progress: Previous approach worked on linux because it was loading the library from jna-wrapper, failed to identify the reasons for failures for mac or windows due to setup issues. Attempting to hardcode jna boot dir path to fix the problem. Reviewing builtins PR. It should be finished by 2025-02-05.

Next Day: Next day I will be working on the #12204 task. Continue investigating the issue.

Add Standard.Tableau to native-image

4c8f598

Closes #12194.

hubertp requested review from jdunkerley, radeusgd, GregoryTravis, AdRiley, marthasharkey, 4e6, JaroslavTulach and Akirathan as code owners January 31, 2025 12:04

hubertp added 2 commits January 31, 2025 13:04

Test NI in nightlies

b0d3abe

Akirathan approved these changes Jan 31, 2025

View reviewed changes

JaroslavTulach requested changes Jan 31, 2025

View reviewed changes

std-bits/tableau/src/main/resources/META-INF/native-image/org/enso/tableau/resource-config.json Outdated Show resolved Hide resolved

enso-bot bot mentioned this pull request Feb 3, 2025

Calling a method that returns 'itself' on a multi-value should not lose multi-value structure? #12143

Closed

Rewrite library loading for NI's Tableau

c648328

This change extracts `Tableau`'s Hyper shared library from the jar. Unlike `Standard.Image`, had to provide a custom classloader that manages to find requested library - HyperAPI used a custom JNA logic for that.

hubertp requested review from JaroslavTulach and Akirathan February 3, 2025 14:03

hubertp added 2 commits February 3, 2025 15:13

Merge branch 'develop' into wip/hubert/12194-ni-tableau

f83ca29

fmt

528ae20

hubertp commented Feb 3, 2025

View reviewed changes

hubertp added the CI: No changelog needed Do not require a changelog entry for this PR. label Feb 3, 2025

fmt

c5a0c89

Akirathan reviewed Feb 3, 2025

View reviewed changes

jdunkerley approved these changes Feb 3, 2025

View reviewed changes

JaroslavTulach reviewed Feb 4, 2025

View reviewed changes

hubertp added 2 commits February 4, 2025 11:09

Add tests

3c1dadb

Provide more info to CI failures

7594da0

JaroslavTulach self-requested a review February 4, 2025 13:19

JaroslavTulach approved these changes Feb 4, 2025

View reviewed changes

hubertp added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Feb 4, 2025

hubertp added 3 commits February 4, 2025 17:19

Fix Windows and MacOS builds

3d959c6

Having a single name for OS would be too simple

b96bb7b

JaroslavTulach mentioned this pull request Feb 5, 2025

CI should run Standard Library Tests in both JVM and Native modes #12123

Open

One more attempt to make Windows happy

d6d6d52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Standard.Tableau to native-image #12204

Add Standard.Tableau to native-image #12204

hubertp commented Jan 31, 2025

JaroslavTulach commented Jan 31, 2025

JaroslavTulach left a comment

hubertp Feb 3, 2025

Akirathan Feb 3, 2025

JaroslavTulach Feb 4, 2025

hubertp commented Feb 3, 2025

Akirathan Feb 3, 2025

enso-bot bot commented Feb 3, 2025

JaroslavTulach Feb 4, 2025

hubertp Feb 4, 2025

JaroslavTulach Feb 4, 2025 •

edited

Loading

JaroslavTulach Feb 4, 2025

Akirathan Feb 4, 2025

hubertp Feb 4, 2025

JaroslavTulach Feb 4, 2025 •

edited

Loading

hubertp Feb 4, 2025

JaroslavTulach Feb 4, 2025 •

edited

Loading

JaroslavTulach Feb 4, 2025 •

edited

Loading

hubertp Feb 4, 2025

JaroslavTulach Feb 4, 2025

JaroslavTulach left a comment •

edited

Loading

enso-bot bot commented Feb 5, 2025

enso-bot bot commented Feb 5, 2025

enso-bot bot commented Feb 6, 2025

Add Standard.Tableau to native-image #12204

Are you sure you want to change the base?

Add Standard.Tableau to native-image #12204

Conversation

hubertp commented Jan 31, 2025

Pull Request Description

Checklist

JaroslavTulach commented Jan 31, 2025

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

hubertp commented Feb 3, 2025

Choose a reason for hiding this comment

enso-bot bot commented Feb 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaroslavTulach Feb 4, 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

JaroslavTulach Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaroslavTulach Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

JaroslavTulach Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaroslavTulach left a comment • edited Loading

Choose a reason for hiding this comment

enso-bot bot commented Feb 5, 2025

enso-bot bot commented Feb 5, 2025

enso-bot bot commented Feb 6, 2025

JaroslavTulach Feb 4, 2025 •

edited

Loading

JaroslavTulach Feb 4, 2025 •

edited

Loading

JaroslavTulach Feb 4, 2025 •

edited

Loading

JaroslavTulach Feb 4, 2025 •

edited

Loading

JaroslavTulach left a comment •

edited

Loading