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

Support for graal native-image #115

Closed
heuermh opened this issue Mar 4, 2020 · 40 comments
Closed

Support for graal native-image #115

heuermh opened this issue Mar 4, 2020 · 40 comments

Comments

@heuermh
Copy link

heuermh commented Mar 4, 2020

Hello,

I am tracking a runtime error with zstd-jni when attempting to build a GraalVM native-image that includes com.github.luben:zstd-jni:jar:1.4.4-7:runtime as a dependency.

heuermh/dishevelled-bio#24

I am still rather new to native-image, and may be missing some configuration or other obvious fix
https://github.com/oracle/graal/blob/master/substratevm/JNI.md

@luben
Copy link
Owner

luben commented Mar 5, 2020

I have not tried it with graal native images, I don't understand what the error in the linked issue means. Any help with deciphering it will be of great help.

@heuermh
Copy link
Author

heuermh commented Mar 5, 2020

Thanks for the comment, @luben!

I am waiting for advice on the native-image Slack channel, will keep this updated if I find anything that works.

@luben
Copy link
Owner

luben commented Mar 5, 2020

One thing you can try is extract the libzstd-jni for your architecutre from the jar and put it on your LD_LIBRARY_PATH, e.g. in /usr/local/lib - the java loader always try to fall-back to locally installed library. (your paths on mac can be different)

@danielkec
Copy link

Would be great if Native.load() was not invoked in static block, it's quite ugly to substitute that for Graal's native image oracle/graal#439 (comment)

@heuermh
Copy link
Author

heuermh commented Dec 7, 2020

Hello @danielkec, would such a change allow for a workaround?

I've since updated to version 1.4.5-12 and this is still an open issue.

@luben
Copy link
Owner

luben commented Dec 7, 2020

It's may be easy to replace the static Native.load with call from the constructor but it may be harder to do it in the Zstd that is all static methods. I will try.

@danielkec
Copy link

I did that lazy loading when ZstdOutputStream is constructed and it seems to be working well, but my use case is limited to Kafka client needs. https://github.com/oracle/helidon/pull/2555/files#diff-e753f841f896d9d0153523f5980d3bac292135793c9eacfab8fe2a082c41665bR76-R92

@luben
Copy link
Owner

luben commented Dec 8, 2020

Then I will make it lazy for all the non-static classes. This should cover most of the use-cases, though the Zstd class (collection of static methods) will not be covered - it will still load the native part on in static section. I think that should be fine - if you don't use the class it will not run the static section.

@danielkec
Copy link

@luben That would be super cool 👍

luben added a commit that referenced this issue Dec 12, 2020
Discussed in #115 - it may help supporting Graal.
@luben
Copy link
Owner

luben commented Dec 18, 2020

@danielkec , I have just released v1.4.7-1 and it moves the loading of the native library from the static sections to the constructors. Though it does not cover the Zstd class that just holds a bunch of static methods.

@luben
Copy link
Owner

luben commented Dec 18, 2020

Hmm, this breaks apache commons/compress. I will revert and release v1.4.7-2 and will explore independently.

@siwee
Copy link

siwee commented Dec 22, 2021

Hi, @luben is there a new update for this issue?

Thank you.

@luben
Copy link
Owner

luben commented Aug 2, 2022

So the lazy loading didn't work. Another option that I can thing of is to let you load manually the native library and tell zstd-jni.Native to assume it's loaded. Is this going to work for you?

Edit: added it - a3b5c4c , can you test and see if it lets you workaround the loading issue?

@linghengqian
Copy link

Warning: Could not resolve class com.github.luben.zstd.ZstdCompressCtx for reflection configuration. Reason: java.lang.ClassNotFoundException: com.github.luben.zstd.ZstdCompressCtx.
Warning: Could not resolve class com.github.luben.zstd.Zstd for reflection configuration. Reason: java.lang.ClassNotFoundException: com.github.luben.zstd.Zstd.

@luben
Copy link
Owner

luben commented Oct 24, 2022

I don't have any experience with GraalVM. I don't mind maintaining our own reachability data in the zstd-jni package, but I will need some help with the initial setup/version.

@linghengqian
Copy link

I don't have any experience with GraalVM. I don't mind maintaining our own reachability data in the zstd-jni package, but I will need some help with the initial setup/version.

@linghengqian
Copy link

linghengqian commented Oct 25, 2022

  • Maybe I'm understanding something wrong, maybe there's nothing that needs to be handled in zstd-jni when building a GraalVM Native Image. Netty itself uses the zstd-jni library, and the real reason for the warning may be that it generates the wrong GraalVM reachability metadata by itself. Refer to Maybe need to add more detailed metadata for io.netty:netty-* oracle/graalvm-reachability-metadata#89 (comment) . In fact, all applications based on Netty 4.1.84.Final are normal, such as building a GraalVM Native Image corresponding to a Webflux application based on Springboot OSS 3.0.0-RC1 and netty 4.1.84.Final.

@luben
Copy link
Owner

luben commented Oct 25, 2022

BTW, the gradle build is just for the Android aars, the JAR build is handled by the sbt.

If I understand correctly, the metadata is needed if somebody uses runtime reflection. In the zstd-jni we don't use any reflection, so may be you are right that netty (if it reflects over zstd-jni classes) should declare its reflection targets.

@linghengqian
Copy link

linghengqian commented Oct 25, 2022

  • Yes, GraalVM reachability metadata only expects to record runtime reflections, as https://www.graalvm.org/22.2/reference-manual/native-image/metadata/ says.

  • Of course, problems like log4j2 have had little to do with GraalVM reachability metadata, since it uses java.lang.invoke.LambdaMetafactory which GraalVM does not yet support.😆

  • From my point of view, there is nothing that can be done in zstd-jni library because GraalVM Native Build Tools does not provide sbt plugin. It looks like this issue can be closed as it seems that all the issues involved can be resolved outside of zstd-jni. All the problems I see come from the configuration of the GraalVM Native Build Tools in Netty itself. 😄

@Ampflower
Copy link

Ampflower commented Oct 27, 2022

For runtime initialised, simply including the binary for the platform it's running on and declaring in jni-config.json: srcPos and dstPos for the I/O streams without finalizers works.

If you're curious what that'd look like:

[
  {
    "name": "com.github.luben.zstd.ZstdOutputStreamNoFinalizer",
    "fields": [
      {
        "name": "dstPos"
      },
      {
        "name": "srcPos"
      }
    ]
  },
  {
    "name": "com.github.luben.zstd.ZstdInputStreamNoFinalizer",
    "fields": [
      {
        "name": "dstPos"
      },
      {
        "name": "srcPos"
      }
    ]
  }
]

Although, for build-time initialized, I've been working on reimplementing the JNI methods in Java for native-image use that only requires the system binary. Would there be any interest in upstreaming it and hooking it up with testing?

@linghengqian
Copy link

@KJP12 I can proactively submit some necessary GraalVM metadata to https://github.com/oracle/graalvm-reachability-metadata , for Gradle there is an Agent integration of Native Build Tools to handle unit tests. I've never used sbt, could you provide a unit test for this part of the metadata? Or do you intend to submit a PR?

@Ampflower
Copy link

I was thinking of maybe pushing the metadata here with a bit extra for not retaining the fields if the classes aren't reachable, since it'd probably be a decent place for that? Tho the central reachability repository should be fine too.

I'm not really experienced with getting testing done with native image, so I am not sure if I can PR that in immediately.

@luben
Copy link
Owner

luben commented Nov 10, 2022

Hi,

Sorry for the long delays on my side. I am open to PRs to add the metadata here. Having it in the https://github.com/oracle/graalvm-reachability-metadata will also work, please tag me if you send a PR there.

Regarding bindings directly to libzstd - I think this will be a fine approach but we have to drop support for old JVMs as far as I can tell. If we do that we may refactor and cleanup the whole library as it has accumulated a lot of cruft due to mistakes, changes in libzstd, and the goal to keep backward compatibility.

@heuermh
Copy link
Author

heuermh commented Nov 10, 2022

Same here! I'm willing to test whatever approaches might work.

In the original reported issue I am using zstd through commons-compress, which adds an additional layer to consider.

@Ampflower
Copy link

Regarding bindings directly to libzstd - I think this will be a fine approach but we have to drop support for old JVMs as far as I can tell. If we do that we may refactor and cleanup the whole library as it has accumulated a lot of cruft due to mistakes, changes in libzstd, and the goal to keep backward compatibility.

The method for Graal native-image using its C API wouldn't require any level of rewriting, beyond translating the JNI code to Java, or dropping support, just substitutions and calling APIs from there, which would not be active outside of native-image. If you feel it'd be best to rewrite it so that mistakes can be ironed out first, I could maintain it independently in the meantime.

Panama is a different thing that would require more intervention tho, and given that there is no LTS JDK with it included, that would have to hold off for a bit longer.

@luben
Copy link
Owner

luben commented Nov 15, 2022

@KJP12 , thanks for clarifying, yes, I was thinking about Panama. I am open to get make it work seamlessly on Graal native.

There are some mistakes, e.g. ZstdOutputStream vs ZstdOutputStreamNoFinalizer, but there is nothing major. I think the risk of rewrite from scratch would be greater than continue with the current structure. So if we can keep the backward compatibility, we don't need to rewrite.

@linghengqian
Copy link

sdk use java 22.3.r17-grl
cd ./zstd-jni/
sdk install scala 2.13.10
./gradlew -Pagent clean test
./gradlew metadataCopy --task test --dir src/main/resources/META-INF/native-image/com.github.luben/zstd-jni/1.5.2-5
  • image

@luben
Copy link
Owner

luben commented Nov 17, 2022

@linghengqian
I am not sure what you are talking about:

  • you should be able to use zstd-jni without bringing the testing code and dependencies - testing artifacts are large because we keep compatibility with Zstd-0.4 and we bring files compressed with these versions in order to be able to test against them.
  • yes, there is no graal reachability data generate yet - we are discussing what's the best home for it in this thread.
  • I don't understand the suggestion - I have zero interest in supporting graal, but I am open to accept contributions to help people that have different needs. If you are willing to contribute, please send a PR.

@linghengqian
Copy link

@luben

  • The reason I use tests is because the GraalVM reachability metadata is generated with proper unit tests. Neither my personal test nor the test of the main branch of zstd-jni can find the part of the metadata mentioned by @KJP12 .
[
  {
    "name": "com.github.luben.zstd.ZstdOutputStreamNoFinalizer",
    "fields": [
      {
        "name": "dstPos"
      },
      {
        "name": "srcPos"
      }
    ]
  },
  {
    "name": "com.github.luben.zstd.ZstdInputStreamNoFinalizer",
    "fields": [
      {
        "name": "dstPos"
      },
      {
        "name": "srcPos"
      }
    ]
  }
]
  • What I'm referring to is that this piece of GraalVM reachability metadata is not known to be required in unit tests. If we can run through the oracle tck plugin task without adding GraalVM reachability metadata, then obviously we don't need to submit it to https://github.com/oracle/graalvm-reachability-metadata .

@luben
Copy link
Owner

luben commented Nov 19, 2022

Sorry for the delay. I think it would be easier to write a few simple happy-path unit tests in Java and try to validate the GraalVM metadata with them. I am not sure what is the support for GraalVM in Scala.

@linghengqian
Copy link

@luben I'm just curious, besides https://github.com/luben/ZstdAndroidExample , does zstd-jni have a document or something for my reference? I don't seem to see it in the README.

@luben
Copy link
Owner

luben commented Nov 20, 2022

There are javadocs: https://www.javadoc.io/doc/com.github.luben/zstd-jni/latest/com/github/luben/zstd/package-summary.html

And if it's about writing tests, I think it would be easy to rewrite some of the Scala tests in Java

@linghengqian
Copy link

  • I'm sorry for what I said above, I absolutely believe it's because I have absolutely no idea how to use GraalVM Native Build Tools with Scala under Gradle, it looks like the Scala plugin for IntelliJ IDEA has no idea what's going on.
  • In fact, I have neither used Gradle nor Scala before contacting the current issue. So instead of trying to translate Scala to Java I wrote my own unit tests under Java.
  • The current issue is meaningless, let's go back to Add support for com.github.luben:zstd-jni:1.5.2-5 oracle/graalvm-reachability-metadata#121.

@luben
Copy link
Owner

luben commented Nov 21, 2022

Thanks a lot for your work! I put some comments on the PR - mostly how we can make the refection configs more generic so they could apply to other versions/OS-es

@linghengqian
Copy link

linghengqian commented Nov 21, 2022

Thanks a lot for your work! I put some comments on the PR - mostly how we can make the refection configs more generic so they could apply to other versions/OS-es

@luben
Copy link
Owner

luben commented Nov 22, 2022

@linghengqian, thanks a lot for your work! Let hope oracle/graalvm-reachability-metadata#121 is merged soon.

@linghengqian
Copy link

  • Oracle employees are used to reviewing PR batch by batch and then releasing it directly. Now can only wait.

@luben
Copy link
Owner

luben commented Jan 11, 2023

it just got merged: oracle/graalvm-reachability-metadata#121

@luben
Copy link
Owner

luben commented Jun 9, 2023

Closing as it is solved by oracle/graalvm-reachability-metadata#121

@luben luben closed this as completed Jun 9, 2023
@heuermh
Copy link
Author

heuermh commented Jun 12, 2023

Thank you, @luben! I'll try to follow the advice above to solve my initial issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants