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

Refactoring the way we register classes for reflection, JNI, etc. #25975

Open
zakkak opened this issue Jun 6, 2022 · 10 comments
Open

Refactoring the way we register classes for reflection, JNI, etc. #25975

zakkak opened this issue Jun 6, 2022 · 10 comments
Assignees
Labels
area/housekeeping Issue type for generalized tasks not related to bugs or enhancements area/native-image

Comments

@zakkak
Copy link
Contributor

zakkak commented Jun 6, 2022

Description

Quarkus currently relies on gizmo to generate a rather large AutomaticFeature for performing a number of things by often invoking GraalVM internal APIs instead of following the "standard" approach of generating json configuration files for GraalVM to consume.

Namely, Quarkus, at the time of writing, seems to rely on the following internal APIs:

  1. com.oracle.svm.util.ReflectionUtil
  2. com.oracle.svm.core.jni.JNIRuntimeAccess
  3. com.oracle.svm.core.jdk.proxy.DynamicProxyRegistry
  4. com.oracle.svm.core.jdk.LocalizationFeature
  5. com.oracle.svm.core.jdk.localization.LocalizationFeature
  6. com.oracle.svm.core.configure.ResourcesRegistry
  7. com.oracle.svm.reflect.serialize.hosted.SerializationFeature
  8. com.oracle.svm.core.jdk.serialize.SerializationRegistry
  9. com.oracle.svm.reflect.serialize.SerializationSupport$StubForAbstractClass
  10. com.oracle.svm.reflect.serialize.SerializationSupport

To me this approach has the following issues:

  1. It makes it harder to add new quarkus features or fix bugs that require tampering with the generation of the AutoFeature. First you need to write code by using an assembler... which you then need to debug by extracting the generated by gizmo class and disassembling it to see if everything looks as expected or not.
  2. We rely on internal APIs with all the issues that brings (e.g. harder to maintain our code because of unexpected changes to internal API, need to ensure the internal APIs are somehow exposed to us, etc.)

Implementation ideas

I propose we start:

  1. Moving as much configuration as possible out of the AutomaticFeature and generating json files instead (which one can easily inspect and alter)
  2. Request upstream Graal to make public any internal APIs that we really need access to.

WDYT?

@zakkak zakkak added area/housekeeping Issue type for generalized tasks not related to bugs or enhancements area/native-image labels Jun 6, 2022
@zakkak
Copy link
Contributor Author

zakkak commented Jun 6, 2022

Regarding point 2 note also that in the Native Image Committer Community Meeting 2022-06-02 it was stated that:

JNI / Resource / Proxy / Serialization registration

Currently not exposed as public API, but really for no good reason. We have the reflection registration already in the API.

@geoand
Copy link
Contributor

geoand commented Oct 19, 2022

@zakkak has this now been implemented?

@jerboaa
Copy link
Contributor

jerboaa commented Oct 19, 2022

2. Request upstream Graal to make public any internal APIs that we really need access to.

I think most APIs in use by quarkus are publicly accessible ones in upcoming graal 22.3.

@zakkak
Copy link
Contributor Author

zakkak commented Oct 19, 2022

@zakkak has this now been implemented?

No. We have indeed switched to using mostly public APIs, but we still:

  1. rely on some internal ones (see Provide API for further use of non-API methods in Quarkus oracle/graal#5013)
  2. rely on gizmo to generate a Feature where we could instead generate json files.

Note, that there are still cases where we might not be able to avoid any of the above. But I still believe it would be good to try and minimize the generated bytecode.

@zakkak
Copy link
Contributor Author

zakkak commented Mar 6, 2023

  1. rely on gizmo to generate a Feature where we could instead generate json files.

This is now fixed by:

So at this point the only non-API call we appear to still use is org.graalvm.nativeimage.impl.RuntimeClassInitializationSupport#rerunInitialization. Since there is no alternative to this on the Quarkus side, we need some upstream changes to fix it.

@zakkak
Copy link
Contributor Author

zakkak commented Oct 17, 2023

oracle/graal@4880346#diff-2f2b04e4347237de74c38065683de2f463234ce2e8e3e37b56bbc66cf2d2d068 enabled by default the "new class initialization strategy that allows all classes to be used at image build time". As a result starting with Mandrel and GraalVM 23.1 Quarkus should no longer need to explicitly set classes to be runtime reinitialized.

I will prepare a patch to test this out.

@zakkak zakkak self-assigned this Feb 14, 2024
@zakkak
Copy link
Contributor Author

zakkak commented Feb 14, 2024

Well that took longer than expected...

The conclusion is that the new class initialization doesn't really affect Quarkus since we explicitly set classes to be build time initialized and request their re-initialization explicitly when necessary.

The new class initialization strategy only affects classes that are not explicitly marked for build time initialization, but happen to be build time initialized because they are dependencies of some other build time initialized class.

As a result, we still need some upstream changes to fix it.

@jerboaa
Copy link
Contributor

jerboaa commented Feb 14, 2024

As a result, we still need some upstream changes to fix it.

For clarity, Quarkus would need an API version for org.graalvm.nativeimage.impl.RuntimeClassInitializationSupport#rerunInitialization. AFAIK, that's the only non-API method still in use. Correct @zakkak?

@zakkak
Copy link
Contributor Author

zakkak commented Feb 14, 2024

Correct.

@zakkak
Copy link
Contributor Author

zakkak commented Feb 15, 2024

According to oracle/graal#5013 (comment):

@zakkak with the new class initialization policy, RuntimeClassInitializationSupport#rerunInitialization is the same as initializeAtRunTime.

With https://github.com/oracle/graal/pull/8323/files#diff-3c452e61cb9ddfbab251a9aa0a134b4c0be47a0ec39020eee896e97a3ef5e2f1R55 rerunInitialization will be deprecated and hard-code to initializeAtRunTime.

That means that we can just switch to the public API initializeAtRunTime (this is implemented in https://github.com/zakkak/quarkus/tree/2024-02-15-no-runtime-rerun and being tested in https://github.com/zakkak/quarkus/actions/runs/7916551827). However, this only works for 23.1 and onwards. I considered adding some conditional code in the Feature generation to check the Mandrel/GraalVM version and decide which method to use accordingly, but this would introduce a dependency on org.graalvm.home.Version to get the Mandrel/GraalVM version which is undesired.

I think for the time being we should stick to the non-API rerunInitialization method and switch to initializeAtRunTime once we drop support for Mandrel/GraalVM < 23.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/housekeeping Issue type for generalized tasks not related to bugs or enhancements area/native-image
Projects
None yet
Development

No branches or pull requests

3 participants