Move reflection and serialization configuration from Feature to json #29886

zakkak · 2022-12-15T11:08:27Z

This has the following benefits:

It's easier to debug/inspect reflection and serialization configuration by looking at a json file instead of having to decompile a generated Feature class and figure out what's going on
It implicitly avoids issues like MethodTooLargeException when too many classes to register for reflection #29693

Performance considerations

In a related zulip chat discussion some concerns about performance where brought up so I am adding some comments on that as well.

This patch should only affect native image generation times (not runtime). This PR only affects how we inform native-image about the classes we would like to register for reflection and serialization. With this PR we hand a json file to native-image and ask it to parse it and do the necessary registrations for us. Before this PR we were explicitly invoking the GRAAL VM API to do the registrations our selves.

According to my measurements the transition from explicitly invoking the API versus passing a json file doesn't have a significant impact on the native image generation times.

Results using the API:

reflect-ser-config-API.txt:Finished generating 'quarkus-integration-test-main-999-SNAPSHOT-runner' in 4m 35s.
reflect-ser-config-API.txt:Finished generating 'quarkus-integration-test-main-999-SNAPSHOT-runner' in 4m 34s.
reflect-ser-config-API.txt:Finished generating 'quarkus-integration-test-main-999-SNAPSHOT-runner' in 4m 21s.
reflect-ser-config-API.txt:Finished generating 'quarkus-integration-test-main-999-SNAPSHOT-runner' in 4m 20s.
reflect-ser-config-API.txt:Finished generating 'quarkus-integration-test-main-999-SNAPSHOT-runner' in 4m 33s.

reflect-ser-config-API.txt:                        13.6s (4.9% of total time) in 57 GCs | Peak RSS: 7.40GB | CPU load: 6.08
reflect-ser-config-API.txt:                        15.5s (5.6% of total time) in 60 GCs | Peak RSS: 7.47GB | CPU load: 6.05
reflect-ser-config-API.txt:                        15.5s (5.9% of total time) in 64 GCs | Peak RSS: 7.52GB | CPU load: 6.25
reflect-ser-config-API.txt:                        13.4s (5.1% of total time) in 60 GCs | Peak RSS: 7.45GB | CPU load: 6.19
reflect-ser-config-API.txt:                        16.5s (6.0% of total time) in 65 GCs | Peak RSS: 7.46GB | CPU load: 6.11

Results using the json file

reflect-ser-config-json.txt:Finished generating 'quarkus-integration-test-main-999-SNAPSHOT-runner' in 4m 23s.
reflect-ser-config-json.txt:Finished generating 'quarkus-integration-test-main-999-SNAPSHOT-runner' in 4m 21s.
reflect-ser-config-json.txt:Finished generating 'quarkus-integration-test-main-999-SNAPSHOT-runner' in 4m 18s.
reflect-ser-config-json.txt:Finished generating 'quarkus-integration-test-main-999-SNAPSHOT-runner' in 4m 19s.
reflect-ser-config-json.txt:Finished generating 'quarkus-integration-test-main-999-SNAPSHOT-runner' in 4m 22s.

reflect-ser-config-json.txt:                        15.8s (6.0% of total time) in 64 GCs | Peak RSS: 7.54GB | CPU load: 6.28
reflect-ser-config-json.txt:                        14.4s (5.5% of total time) in 62 GCs | Peak RSS: 7.37GB | CPU load: 6.21
reflect-ser-config-json.txt:                        13.8s (5.3% of total time) in 61 GCs | Peak RSS: 7.42GB | CPU load: 6.25
reflect-ser-config-json.txt:                        15.0s (5.8% of total time) in 63 GCs | Peak RSS: 7.52GB | CPU load: 6.29
reflect-ser-config-json.txt:                        14.5s (5.5% of total time) in 62 GCs | Peak RSS: 7.50GB | CPU load: 6.23

Relates to #25975
Fixes: #29693

cc @essobedo

essobedo · 2022-12-15T11:21:31Z

Thx for sharing 🙏 One small question: How did you fix the issues related to optional services which cause several native tests to fail (kafka and graphql)?

Indeed the difference between before and this PR, is the fact that a class was registered only if it could be initialized (indeed calling getDeclaredConstructors, getDeclaredMethods and getDeclaredFields force the initialization of the class) but with the JSON file approach the classes are registered without being initialized which causes side effects.

To workaround the problem, I propose to be able to define conditions essobedo@d7bbed3 WDYT? As you can see, the build is not yet over but all the native tests pass

zakkak · 2022-12-15T11:44:52Z

Thx for sharing pray One small question: How did you fix the issues related to optional services which cause several native tests to fail (kafka and graphql)?

I probably didn't. I only tested with a few integration tests, so I probably never crossed these issues.

Indeed the difference between before and this PR, is the fact that a class was registered only if it could be initialized

Hmmm, that's a good point. I guess we should either way remove classes that can't be initialized.

(indeed calling getDeclaredConstructors, getDeclaredMethods and getDeclaredFields force the initialization of the class) but with the JSON file approach the classes are registered without being initialized which causes side effects.

In Quarkus we ask anything reachable to be build-time initialized, so I am not sure that holds in Quarkus.

To workaround the problem, I propose to be able to define conditions essobedo@d7bbed3 WDYT? As you can see, the build is not yet over but all the native tests pass

Nice, I like the addition of the conditions but it also makes me wonder... Don't we know what's reachable and what's not at the deployment phase of Quarkus? Why do we need to offset this to native-image? Is there any way to have the class reachable in one native run and not in another?

essobedo · 2022-12-15T11:59:13Z

Nice, I like the addition of the conditions but it also makes me wonder... Don't we know what's reachable and what's not at the deployment phase of Quarkus? Why do we need to offset this to native-image? Is there any way to have the class reachable in one native run and not in another?

It is what I thought at first too, so before adding a class to the generated JSON file, I tried to initialize the class first but in practice it makes even more tests to fail due to a native compilation issue so obviously, there is a difference in terms of available classes between the deployment phase and the native compilation. TBH I did not dig much to understand the difference especially because I believed that it is still interesting to have a way to add a condition on class registration, we then get even more control than before.

essobedo · 2022-12-15T12:05:17Z

Maybe I missed something so let's try again ccee464 and see the results of the build

zakkak · 2022-12-15T12:24:36Z

Well, I am curious to see what makes the failing to register classes (e.g. org.apache.kafka.common.security.oauthbearer.secured.OAuthBearerValidatorCallbackHandler) to be requested in the first place and see if we can fix it there.

zakkak · 2022-12-15T12:41:09Z

Well, I am curious to see what makes the failing to register classes (e.g. org.apache.kafka.common.security.oauthbearer.secured.OAuthBearerValidatorCallbackHandler) to be requested in the first place and see if we can fix it there.

OK so it looks like such classes are added using generic getters like in

quarkus/extensions/kafka-client/deployment/src/main/java/io/quarkus/kafka/client/deployment/KafkaProcessor.java

Line 433 in e12ebe1

    
           for (ClassInfo loginModule : index.getIndex().getAllKnownImplementors(LOGIN_MODULE)) {

The class exists in the Quarkus index (i.e. it's reachable), but Quarkus doesn't know it can't be loaded due to missing dependencies, so there indeed doesn't seem to exist a smart way to avoid getting it in the candidates list in the first place.

essobedo · 2022-12-15T13:23:42Z

Well, I am curious to see what makes the failing to register classes (e.g. org.apache.kafka.common.security.oauthbearer.secured.OAuthBearerValidatorCallbackHandler) to be requested in the first place and see if we can fix it there.

Please note that in theory, AFAIU, the class OAuthBearerValidatorCallbackHandler is only meant to be used for the inter-broker communication so it should not be registered at all but adding a condition make it more flexible

zakkak · 2022-12-15T13:36:10Z

Please note that in theory, AFAIU, the class OAuthBearerValidatorCallbackHandler is only meant to be used for the inter-broker communication so it should not be registered at all but adding a condition make it more flexible

It's not clear to me what the best approach would be here. The condition is indeed a generic that can allow us to overcome such issues, but on the other hand Quarkus should avoid registering blindly classes even if they can be build-time initialized as this is adding overhead to the application.

I think as far as this PR is concerned I am going to cherry pick your commit with the conditional registration.

mkouba · 2022-12-15T13:43:28Z

The Native Tests - Messaging1 job fails with java.lang.NoClassDefFoundError: org/jose4j/keys/resolvers/VerificationKeyResolver which is very likely related to the build-time initialization discussion, right?

essobedo · 2022-12-15T13:57:18Z

The Native Tests - Messaging1 job fails with java.lang.NoClassDefFoundError: org/jose4j/keys/resolvers/VerificationKeyResolver which is very likely related to the build-time initialization discussion, right?

Yeah we know, it is what I mention in this comment #29886 (comment)

essobedo · 2022-12-15T14:20:52Z

Here is the kind of error you get if you initialize the class during the build deployment phase https://github.com/essobedo/quarkus/actions/runs/3704534875/jobs/6277704690#step:12:482

essobedo · 2022-12-15T14:24:22Z

core/deployment/src/main/java/io/quarkus/deployment/steps/NativeImageReflectConfigStep.java

            this.constructors = constructors;
-            this.weak = weak;


Good idea 👍

essobedo

👍 Awesome! Just some cosmetic remarks

core/deployment/src/main/java/io/quarkus/deployment/steps/NativeImageReflectConfigStep.java

...deployment/src/main/java/io/quarkus/deployment/steps/NativeImageSerializationConfigStep.java

essobedo · 2022-12-16T07:57:34Z

The build passed so I guess that the PR is ready for review

zakkak · 2022-12-16T09:46:29Z

cc @dmlloyd @n1hility @Sanne in case they can forsee issues with this change.

...ment/src/main/java/io/quarkus/deployment/builditem/nativeimage/ReflectiveClassBuildItem.java

...main/java/io/quarkus/deployment/builditem/nativeimage/ReflectiveClassConditionBuildItem.java

core/deployment/src/main/java/io/quarkus/deployment/steps/ReflectiveHierarchyStep.java

As of GraalVM 21.1 finalFieldsWritable is no longer needed when registering fields for reflection.

zakkak · 2023-01-30T09:20:36Z

FYI all the PRs have been merged

All except for the apicurio one.

#30633, taking care of this, is now merged as well.

quarkus-bot · 2023-01-30T15:38:20Z

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Follow up to quarkusio#29886

zakkak requested a review from mkouba December 15, 2022 11:08

quarkus-bot bot added area/core area/jaeger area/reactive-messaging area/smallrye labels Dec 15, 2022

zakkak force-pushed the 2022-12-05-move-reflective-config-to-json branch from e12ebe1 to 67a776e Compare December 15, 2022 14:20

quarkus-bot bot added area/graphql area/kafka labels Dec 15, 2022

essobedo reviewed Dec 15, 2022

View reviewed changes

essobedo approved these changes Dec 15, 2022

View reviewed changes

zakkak force-pushed the 2022-12-05-move-reflective-config-to-json branch from 67a776e to 9d76d5c Compare December 15, 2022 14:55

This comment has been minimized.

Sign in to view

mkouba reviewed Dec 16, 2022

View reviewed changes

...ment/src/main/java/io/quarkus/deployment/builditem/nativeimage/ReflectiveClassBuildItem.java Show resolved Hide resolved

mkouba reviewed Dec 16, 2022

View reviewed changes

...main/java/io/quarkus/deployment/builditem/nativeimage/ReflectiveClassConditionBuildItem.java Show resolved Hide resolved

mkouba reviewed Dec 16, 2022

View reviewed changes

core/deployment/src/main/java/io/quarkus/deployment/steps/ReflectiveHierarchyStep.java Show resolved Hide resolved

zakkak force-pushed the 2022-12-05-move-reflective-config-to-json branch 2 times, most recently from 8fc87f4 to 9a71acf Compare December 16, 2022 10:45

n1hility approved these changes Jan 26, 2023

View reviewed changes

zakkak and others added 3 commits January 30, 2023 11:18

Move reflection and serialization configuration from Feature to json

dfd1429

Drop finalFieldsWritable fields as they are no longer needed

bf43f63

As of GraalVM 21.1 finalFieldsWritable is no longer needed when registering fields for reflection.

Add conditions to optional classes to register

9023ce7

zakkak force-pushed the 2022-12-05-move-reflective-config-to-json branch from e02e08d to 9023ce7 Compare January 30, 2023 09:19

zakkak added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jan 30, 2023

This comment has been minimized.

Sign in to view

zakkak merged commit ac9e7ee into quarkusio:main Jan 30, 2023

quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jan 30, 2023

zakkak deleted the 2022-12-05-move-reflective-config-to-json branch January 30, 2023 16:04

quarkus-bot bot added this to the 2.17 - main milestone Jan 30, 2023

quarkus-bot bot added the kind/bugfix label Jan 30, 2023

rsvoboda mentioned this pull request Feb 2, 2023

Add a scenario with an extension registering many classes quarkus-qe/beefy-scenarios#324

Merged

zakkak added a commit to zakkak/quarkus that referenced this pull request Feb 15, 2023

Move native JNI configuration from Feature to json

9937825

Follow up to quarkusio#29886

zakkak mentioned this pull request Feb 15, 2023

Move native JNI configuration from Feature to json #31179

Merged

zakkak added a commit to zakkak/quarkus that referenced this pull request Feb 15, 2023

Move native Proxy configuration from Feature to json

43fb66c

Follow up to quarkusio#29886

zakkak mentioned this pull request Feb 15, 2023

Move native Proxy configuration from Feature to json #31180

Merged

zakkak added a commit to zakkak/quarkus that referenced this pull request Feb 15, 2023

Move native Resource configuration from Feature to json

d15c4f2

Follow up to quarkusio#29886

zakkak mentioned this pull request Feb 15, 2023

Move native Resource configuration from Feature to json #31185

Merged

Sanne pushed a commit to Sanne/quarkus that referenced this pull request Feb 16, 2023

Move native Proxy configuration from Feature to json

058e24e

Follow up to quarkusio#29886

zakkak added a commit to zakkak/quarkus that referenced this pull request Feb 17, 2023

Move native Resource configuration from Feature to json

80cf0d3

Follow up to quarkusio#29886

zakkak added a commit to zakkak/quarkus that referenced this pull request Feb 17, 2023

Move native Resource configuration from Feature to json

d4f7605

Follow up to quarkusio#29886

zakkak added a commit to zakkak/quarkus that referenced this pull request Feb 17, 2023

Move native Resource configuration from Feature to json

54da87a

Follow up to quarkusio#29886

zakkak added a commit to zakkak/quarkus that referenced this pull request Feb 17, 2023

Move native Resource configuration from Feature to json

982b734

Follow up to quarkusio#29886

zakkak added a commit to zakkak/quarkus that referenced this pull request Feb 20, 2023

Move native Resource configuration from Feature to json

b7f49dd

Follow up to quarkusio#29886

This was referenced Mar 6, 2023

io.quarkus.runner.AutoFeature hides exceptions thrown while registering resources and classes for JNI and reflective access #26162

Closed

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

Open

Karm mentioned this pull request Apr 6, 2023

Adapts AWT extension to the new dynamic .so loading [JDK 17, 19, 20] #32432

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move reflection and serialization configuration from Feature to json #29886

Move reflection and serialization configuration from Feature to json #29886

zakkak commented Dec 15, 2022

essobedo commented Dec 15, 2022 •

edited

Loading

zakkak commented Dec 15, 2022

essobedo commented Dec 15, 2022 •

edited

Loading

essobedo commented Dec 15, 2022 •

edited

Loading

zakkak commented Dec 15, 2022 •

edited

Loading

zakkak commented Dec 15, 2022

essobedo commented Dec 15, 2022

zakkak commented Dec 15, 2022

mkouba commented Dec 15, 2022

essobedo commented Dec 15, 2022

essobedo commented Dec 15, 2022

essobedo Dec 15, 2022

essobedo left a comment

This comment has been minimized.

essobedo commented Dec 16, 2022

zakkak commented Dec 16, 2022

zakkak commented Jan 30, 2023

This comment has been minimized.

quarkus-bot bot commented Jan 30, 2023

Move reflection and serialization configuration from Feature to json #29886

Move reflection and serialization configuration from Feature to json #29886

Conversation

zakkak commented Dec 15, 2022

Performance considerations

Results using the API:

Results using the json file

essobedo commented Dec 15, 2022 • edited Loading

zakkak commented Dec 15, 2022

essobedo commented Dec 15, 2022 • edited Loading

essobedo commented Dec 15, 2022 • edited Loading

zakkak commented Dec 15, 2022 • edited Loading

zakkak commented Dec 15, 2022

essobedo commented Dec 15, 2022

zakkak commented Dec 15, 2022

mkouba commented Dec 15, 2022

essobedo commented Dec 15, 2022

essobedo commented Dec 15, 2022

essobedo Dec 15, 2022

Choose a reason for hiding this comment

essobedo left a comment

Choose a reason for hiding this comment

This comment has been minimized.

essobedo commented Dec 16, 2022

zakkak commented Dec 16, 2022

zakkak commented Jan 30, 2023

This comment has been minimized.

quarkus-bot bot commented Jan 30, 2023

essobedo commented Dec 15, 2022 •

edited

Loading

essobedo commented Dec 15, 2022 •

edited

Loading

essobedo commented Dec 15, 2022 •

edited

Loading

zakkak commented Dec 15, 2022 •

edited

Loading