graal icon indicating copy to clipboard operation
graal copied to clipboard

[GR-40198] Provide public API for feature-based JNI / Resource / Proxy / Serialization registration.

Open graalvmbot opened this issue 3 years ago • 8 comments

This PR adds the missing APIs that will allow our users to not rely on our svm-internal APIs anymore.

graalvmbot avatar Aug 02 '22 16:08 graalvmbot

@olpaw Thanks for adding me to this. Is this work being planned for 22.3? As this would be related to https://github.com/oracle/graal/discussions/4792 do you have some details on what exactly is svm-internal API and, thus, will allow us to determine what will move?

jerboaa avatar Aug 08 '22 12:08 jerboaa

Is this work being planned for 22.3?

Yes. If you can help make sure that everything you need is in the first batch, that would speed up the overall migration process for all of us. :)

As this would be related to https://github.com/oracle/graal/discussions/4792 do you have some details on what exactly is svm-internal API and, thus, will allow us to determine what will move?

The migration should happen in a backward compatible way, so we don't plan to move anything just yet. This PR adds a new API to the Native Image part of the GraalVM SDK that frameworks are supposed to use in the future. Everything in com.oracle.svm is considered internal, while org.graalvm.nativeimage is the public API. So essentiall, we need to make sure, grep -R "com.oracle.svm" /the/quarkus/codebase does not return any results. :)

fniephaus avatar Aug 08 '22 12:08 fniephaus

@olpaw Thanks for adding me to this. Is this work being planned for 22.3?

Yes

As this would be related to #4792 do you have some details on what exactly is svm-internal API

Everything that is part of our SVM codebase that currently requires a --add-exports when the image-builder is running on module path (with GraalVM 22.2) (i.e. when the USE_NATIVE_IMAGE_JAVA_PLATFORM_MODULE_SYSTEM=false workaround is NOT used) is considered internal API. See add-exports in https://github.com/oracle/graal/blob/master/java-benchmarks/mx.java-benchmarks/mx_java_benchmarks.py#L374..L380

and, thus, will allow us to determine what will move?

As you can see this PR only adds API but does not remove anything. To be on the safe side projects that currently use svm-internal API should switch to using public API only (for example instead of using com.oracle.svm.core.jni.JNIRuntimeAccess.register use org.graalvm.nativeimage.hosted.RuntimeJNIAccess.register). Users that will continue using non-public API are at risk of experiencing breaking changes as we might modify non-public API as we see fit.

olpaw avatar Aug 08 '22 12:08 olpaw

One major package that I see missing from this batch is com.oracle.svm.core.annotate, not sure if you plan to make it public API in a different PR.

@zakkak, yes making the annotations available as public API is a different PR: https://github.com/oracle/graal/pull/4683

olpaw avatar Aug 08 '22 13:08 olpaw

  1. com.oracle.svm.util.ReflectionUtil#lookupMethod

Here I suggest to just copy our implementation (as a starting point). This is a simple helper-method that itself only depends on JDK code. Thus you are better off having your own variant of it in your codebase (i.e. you might want to use such a method in places that otherwise do not require native-image at all).

olpaw avatar Aug 08 '22 13:08 olpaw

9. com.oracle.svm.hosted.reflect.serialize.SerializationFeature, this is added as a required feature in one of the quarkus features.

Here we should make sure that all SVM internal feature handlers have priority (i.e. are executed before) any user-provided feature handler (e.g. Quarkus features). This way your code that specifies SerializationFeature as a required feature becomes obsolete and can be removed. cc @christianwimmer

olpaw avatar Aug 08 '22 13:08 olpaw

6. com.oracle.svm.core.jdk.localization.LocalizationFeature#prepareBundle

No need to use the internal prepareBundle. Use org.graalvm.nativeimage.hosted.RuntimeResourceAccess#addResourceBundles instead.

olpaw avatar Aug 08 '22 13:08 olpaw

  1. com.oracle.svm.core.jdk.UnsupportedFeatureError to report a more quarkus-friendly error

I agree. We should discuss a custom solution for this use of com.oracle.svm.core.jdk.UnsupportedFeatureError in quarkus.

BTW, thanks for the detailed list above @zakkak !

olpaw avatar Aug 08 '22 13:08 olpaw

9. com.oracle.svm.hosted.reflect.serialize.SerializationFeature, this is added as a required feature in one of the quarkus features.

@zakkak if you stop using @AutomaticFeature (by using --features <list of FQNs of quarkus features> instead) you should be able to get rid of requiring your feature to specify depending on SerializationFeature because @AutomaticFeatures are always registered before non-automatic ones and thus their hooks are always executed before the ones from user provided features.

olpaw avatar Aug 11 '22 13:08 olpaw

  1. com.oracle.svm.hosted.reflect.serialize.SerializationFeature, this is added as a required feature in one of the quarkus features.

@zakkak I'm quite curious why you need this feature dependency at all.

christianwimmer avatar Aug 11 '22 19:08 christianwimmer

@zakkak if you stop using @AutomaticFeature (by using --features <list of FQNs of quarkus features> instead) you should be able to get rid of requiring your feature to specify depending on SerializationFeature because @AutomaticFeatures are always registered before non-automatic ones and thus their hooks are always executed before the ones from user provided features.

Cool, we have already done that in https://github.com/quarkusio/quarkus/pull/25976 so in theory we no longer need this.

@zakkak I'm quite curious why you need this feature dependency at all.

The main reason according to https://github.com/quarkusio/quarkus/pull/15380 was that:

If serialization requirement is detected, generated AutoFeature depends on SerializationFeatue (there is a "cache" of registered serialized classes, which is required in runtime.

Another reason is discussed in https://github.com/quarkusio/quarkus/pull/25861#issuecomment-1146514806 and https://github.com/quarkusio/quarkus/pull/25861#issuecomment-1147127342.

Given that @AutomaticFeatures are always registered before non-automatic ones I will prepare a PR removing the dependency.

Update: The PR removing the dependency is https://github.com/quarkusio/quarkus/pull/27263

Thank you @olpaw and @christianwimmer

zakkak avatar Aug 12 '22 06:08 zakkak

We have some usages that I think are not yet covered by this (workarounds are very welcome):

  1. com.oracle.svm.core.SubstrateOptions
    • Used to get the value of SubstrateOptions.Class, as we have multiple entrypoints within a module, and need to switch config based on which one is used.
  2. com.oracle.svm.core.jdk.NativeLibrarySupport, com.oracle.svm.hosted.c.NativeLibraries and com.oracle.svm.core.jdk.JNIRegistrationUtil
    • Used to write library support in a similar way to JNIRegistrationJava.
    • The JNIRegistrationUtil superclass can be substituted by our own implementation, but without the other classes we have no way to do the static linking ourselves.
  3. com.oracle.svm.core.jdk.Resources#registerResource(java.lang.String, java.lang.String, java.io.InputStream)
    • Used in our custom support for the fontmanager library, to add the fontconfig.bfc file to the resources, to be used from the substitution.
    • We can possibly work around this by loading the resource in a static byte array instead in our substitutions.
  4. com.oracle.svm.hosted.FeatureImpl.DuringSetupAccessImpl#registerClassReachabilityListener
    • Used to add reachability handlers based on conditions not currently available. For us specifically a listener that fires for all classes with a specific annotation.
  5. com.oracle.svm.hosted.classinitialization.ConfigurableClassInitialization
    • Hack to conditionally add resource bundles during analysis, used in the same way as here.
    • Any workaround suggestions to add simple resource-bundles on the fly would be much appreciated.

kristofdho avatar Aug 24 '22 14:08 kristofdho

Hi @kristofdho

Thanks for your feedback!

This PR is in its final stages but there can be followup PRs to address the issues on your list if necessary.

We have some usages that I think are not yet covered by this (workarounds are very welcome):

1. `com.oracle.svm.core.SubstrateOptions`
   
   * Used to get the value of `SubstrateOptions.Class`, as we have multiple entrypoints within a module, and need to switch config based on which one is used.

I see your point. Note that the use of SubstrateOptions.Class is not safe for what you are doing anyway. Because what will be the mainClass depends on more than just SubstrateOptions.Class. See com.oracle.svm.hosted.NativeImageGeneratorRunner#buildImage at if (className.isEmpty()) { ....

But since it's you providing the arguments to the native-image command what prevents you from using native-image ... foo.MyMainClass -Dkristof.mainclass=foo.MyMainClass and then in the feature you use System.getProperty("kristof.mainclass") to switch the configs.

I will think a bit more about the other points you raised and get back to you next week.

olpaw avatar Aug 24 '22 15:08 olpaw

@olpaw Do I need to create a new issue with these remaining usages?

kristofdho avatar Sep 01 '22 13:09 kristofdho

@olpaw Do I need to create a new issue with these remaining usages?

That would be a good idea. Then it's easier to keep track of what is still open.

olpaw avatar Sep 01 '22 14:09 olpaw