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

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

Merged
merged 18 commits into from
Aug 29, 2022

Conversation

graalvmbot
Copy link
Collaborator

@graalvmbot graalvmbot commented Aug 2, 2022

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

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Aug 2, 2022
@graalvmbot graalvmbot force-pushed the github/paw/GR-40198 branch from 14d9d84 to 2cd157c Compare August 3, 2022 14:04
@olpaw olpaw added this to the 22.3 milestone Aug 3, 2022
@olpaw olpaw self-assigned this Aug 3, 2022
@graalvmbot graalvmbot force-pushed the github/paw/GR-40198 branch 4 times, most recently from 180912f to 1e3a5ea Compare August 8, 2022 10:04
@jerboaa
Copy link
Collaborator

jerboaa commented Aug 8, 2022

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

@fniephaus
Copy link
Member

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 #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. :)

@olpaw
Copy link
Member

olpaw commented Aug 8, 2022

@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.

Copy link
Collaborator

@zakkak zakkak left a comment

Choose a reason for hiding this comment

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

Hello Paul, thanks for initiating this refactoring.

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.

Other than the annotations, Quarkus is currently using the following internal APIs:

  1. com.oracle.svm.core.jdk.UnsupportedFeatureError to report a more quarkus-friendly error
  2. com.oracle.svm.core.jdk.Resources#registerResource
  3. com.oracle.svm.util.ReflectionUtil#lookupMethod
  4. com.oracle.svm.core.jni.JNIRuntimeAccess#register
  5. com.oracle.svm.core.jdk.proxy.DynamicProxyRegistry#addProxyClass
  6. com.oracle.svm.core.jdk.localization.LocalizationFeature#prepareBundle
  7. com.oracle.svm.core.configure.ResourcesRegistry#ignoreResources
  8. com.oracle.svm.core.configure.ResourcesRegistry#addResources
  9. com.oracle.svm.hosted.reflect.serialize.SerializationFeature, this is added as a required feature in one of the quarkus features.

From the above 1, 3, 6 and 9 seem not covered by this PR.

@olpaw
Copy link
Member

olpaw commented Aug 8, 2022

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: #4683

@olpaw
Copy link
Member

olpaw commented Aug 8, 2022

  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
Copy link
Member

olpaw commented Aug 8, 2022

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
Copy link
Member

olpaw commented Aug 8, 2022

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
Copy link
Member

olpaw commented Aug 8, 2022

  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 !

@graalvmbot graalvmbot force-pushed the github/paw/GR-40198 branch 2 times, most recently from 14bbe56 to 2406922 Compare August 8, 2022 17:31
@graalvmbot graalvmbot force-pushed the github/paw/GR-40198 branch from 2406922 to ca8f524 Compare August 10, 2022 15:58
@olpaw
Copy link
Member

olpaw commented Aug 11, 2022

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.

@christianwimmer
Copy link

  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.

@graalvmbot graalvmbot force-pushed the github/paw/GR-40198 branch from 7d455f8 to 69f7616 Compare August 28, 2022 20:27
@graalvmbot graalvmbot force-pushed the github/paw/GR-40198 branch from 69f7616 to 779ce73 Compare August 29, 2022 05:42
@graalvmbot graalvmbot merged commit 4115f50 into master Aug 29, 2022
@graalvmbot graalvmbot deleted the github/paw/GR-40198 branch August 29, 2022 09:01
@kristofdho
Copy link

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

@olpaw
Copy link
Member

olpaw commented Sep 1, 2022

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Complete the Third-Party API in Native Image
7 participants