Avoid MethodTooLargeException when too many classes to register for reflection #29694

essobedo · 2022-12-05T18:59:58Z

Motivation

When we have too many classes to register for reflection, we get an error indicating that Feature.registerForReflection is too large.

Modifications:

Call the methods of type registerClasses by reflection to limit the size of the method
Move all the common logic into the generated method _registerClass
Manage the different classes to register by batches of CLASSES_TO_REGISTER_BATCH_SIZE (100) classes to limit the size of methods and the number of constant pool items of the class that causes ClassTooLargeException

Result

The extension can be built as expected and the integration tests of the scenario quarkus-qe/beefy-scenarios#324 works even in native mode

quarkus-bot · 2022-12-05T19:00:00Z

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

title should not contain an issue number (use Fix #1234 in the description instead)

This message is automatically generated by a bot.

gsmet · 2022-12-06T08:19:31Z

Pinging @mkouba as he did a lot of these splits.

mkouba · 2022-12-06T08:38:17Z

Call the methods of type registerClass by reflection to limit the size of the method

Well, this is something we'd probably want to avoid. But I'll need to check the reproducer first...

Manage the different classes to register by batches of 100 classes to limit the size of methods and the total amount of constants which causes ClassTooLargeException

@essobedo You did not mention the ClassTooLargeException in the issue description. What constants do you have in mind?

essobedo · 2022-12-06T08:44:49Z

Well, this is something we'd probably want to avoid. But I'll need to check the reproducer first...

@mkouba let me write an integration test

You did not mention the ClassTooLargeException in the issue description. What constants do you have in mind?

If you have too many methods in a class, you get a ClassTooLargeException. I did not mention it in my original ticket because if you fix the MethodTooLargeException, you still get a ClassTooLargeException. In other words, there are actually 2 problems to fix but the first one (MethodTooLargeException) hides the other one.

mkouba · 2022-12-06T08:52:53Z

If you have too many methods in a class, you get a ClassTooLargeException. I did not mention it in my original ticket because if you fix the MethodTooLargeException, you still get a ClassTooLargeException. In other words, there are actually 2 problems to fix but the first one (MethodTooLargeException) hides the other one.

I see. Well, we do generate one registerClassX for each class and the jvm limit is 65K so there must be other parts of the generation process that contribute to this problem.

zakkak

Hi @essobedo , thank you for the contribution.

I have added a couple of suggestions on how to improve this, please have a look and either implement the suggestions or let me know if you disagree.

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

essobedo · 2022-12-06T09:22:40Z

Any idea in which module I can add an integration test?

essobedo · 2022-12-06T10:42:40Z

Unless you know a better place to add my IT, by default I will add it to integration-tests/maven

mkouba · 2022-12-06T12:44:36Z

Unless you know a better place to add my IT, by default I will add it to integration-tests/maven

Do you plan to add the dependency to the swift library or what exactly would you like to test? I mean do you intend to test a use case with thousands of classes registered for reflection?

essobedo · 2022-12-06T13:04:35Z

Unless you know a better place to add my IT, by default I will add it to integration-tests/maven

Do you plan to add the dependency to the swift library or what exactly would you like to test? I mean do you intend to test a use case with thousands of classes registered for reflection?

Yes I plan to test a use case with thousands of classes registered for reflection but ideally without adding the swift library for the sake of simplicity if possible

mkouba · 2022-12-06T13:48:56Z

Yes I plan to test a use case with thousands of classes registered for reflection but ideally without adding the swift library for the sake of simplicity if possible

That sounds like something that should go outside of the quarkus core. Maybe https://github.com/quarkus-qe/beefy-scenarios or similar?

essobedo · 2022-12-06T17:12:11Z

@mkouba I have proposed a PR with the test case to reproduce quarkus-qe/beefy-scenarios#324

zakkak

LGTM, other than the suggestion I added.

Still not convinced we need to make the invocations to registerClasses* through reflection, but it's not a blocker for me.

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

zakkak · 2022-12-06T18:56:37Z

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

+                        Feature.BeforeAnalysisAccess.class);
+                currentRegisterClass.setModifiers(Modifier.PRIVATE | Modifier.STATIC);
+            }
+            boolean hasConstructorsToHandle = !entry.getValue().weak && !entry.getValue().constructors


@essobedo you still have the weak check in hasConstructorsToHandle. Why? If it is weak then we should immediately skip the tryBlock no matter if it hasMethodsToHandle etc., no?

No, if you check the original code, you will see that tryBlock is needed if entry.getValue().serialization is true whatever the value of entry.getValue().weak

essobedo · 2022-12-12T18:00:27Z

I just assumed that in Quarkus core we don't really want to have a dependency like Jakson but if it is not an issue for you, I can modify the code consequently. WDYT?

We don't need Jackson to generate a simpe json. We could just make this util class public.

But I'd really like to hear @zakkak's opinion because if I'm not mistaken he was working on a similar task.

@mkouba I've just proposed a 3rd approach that generates the JSON files directly in the format expected by GraalVM, except for the "weak reflection" that is more or less specific to Quarkus so I need to keep an internal format for it and a dedicated Feature to load and process it. Is it what you had in mind?

…eflection

quarkus-bot · 2022-12-13T14:08:52Z

⚠️ Unable to include the stracktraces as the report was too long. See annotations below for the details.

Failing Jobs - Building `faa5949`

Status	Name	Step	Failures	Logs	Raw logs
✖	Gradle Tests - JDK 11	`Build`	Failures	Logs	Raw logs
✖	Gradle Tests - JDK 11 Windows	`Build`	Failures	Logs	Raw logs
✖	JVM Tests - JDK 11	`Build`	Failures	Logs	Raw logs
✖	JVM Tests - JDK 11 Windows	`Build`	Failures	Logs	Raw logs
✖	JVM Tests - JDK 17	`Build`	Failures	Logs	Raw logs
✖	JVM Tests - JDK 17 MacOS M1	`Build`	Failures	Logs	Raw logs
✖	JVM Tests - JDK 18	`Build`	Failures	Logs	Raw logs
✖	Native Tests - HTTP	`Build`	Failures	Logs	Raw logs
✖	Native Tests - Messaging1	`Build`	Failures	Logs	Raw logs
✖	Native Tests - Misc3	`Build`	Failures	Logs	Raw logs
✖	Quickstarts Compilation - JDK 17	`Compile Quickstarts`	Failures	Logs	Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ Gradle Tests - JDK 11 #

- Failing: integration-tests/gradle

📦 integration-tests/gradle

✖ io.quarkus.gradle.BasicJavaLibraryModuleTest.testBasicMultiModuleBuild line 25 - More details - Source on GitHub

✖ io.quarkus.gradle.devmode.BasicJavaLibraryModuleDevModeTest.main line 23 - More details - Source on GitHub

📦 integration-tests/gradle/target/classes/basic-java-library-module/application

✖ org.acme.ApplicationConfigResourceTest.testAppConfigEndpoint() - More details - Source on GitHub

⚙️ Gradle Tests - JDK 11 Windows #

- Failing: integration-tests/gradle

📦 integration-tests/gradle

✖ io.quarkus.gradle.BasicJavaLibraryModuleTest.testBasicMultiModuleBuild line 25 - More details - Source on GitHub

✖ io.quarkus.gradle.devmode.BasicJavaLibraryModuleDevModeTest.main line 23 - More details - Source on GitHub

📦 integration-tests/gradle/target/classes/basic-java-library-module/application

✖ org.acme.ApplicationConfigResourceTest.testAppConfigEndpoint() - More details - Source on GitHub

⚙️ JVM Tests - JDK 11 #

- Failing: integration-tests/kubernetes-client integration-tests/kubernetes-client-devservices integration-tests/kubernetes/quarkus-standard-way and 1 more

📦 integration-tests/kubernetes-client

✖ io.quarkus.it.kubernetes.client.ConfigMapPropertiesTest.testPropertiesReadFromConfigMap - More details - Source on GitHub

✖ io.quarkus.it.kubernetes.client.KubernetesClientTest.testInteractionWithAPIServer - More details - Source on GitHub

✖ io.quarkus.it.kubernetes.client.KubernetesNewClientTest.testInteractionWithAPIServer - More details - Source on GitHub

✖ io.quarkus.it.kubernetes.client.KubernetesTestServerOnProfileTest.testConfiguration - More details - Source on GitHub

✖ io.quarkus.it.kubernetes.client.KubernetesTestServerTest.testConfiguration - More details - Source on GitHub

✖ io.quarkus.it.kubernetes.client.NamespacedConfigMapPropertiesTest.testPropertiesReadFromConfigMap - More details - Source on GitHub

✖ io.quarkus.it.kubernetes.client.OpenShiftTestServerTest.testInjectionDefaultsToCrud - More details - Source on GitHub

✖ io.quarkus.it.kubernetes.client.SecretPropertiesTest.testPropertiesReadFromConfigMap - More details - Source on GitHub

📦 integration-tests/kubernetes-client-devservices

✖ io.quarkus.kubernetes.client.devservices.it.DevServicesKubernetesITest.shouldReturnAllKeys - More details - Source on GitHub

📦 integration-tests/kubernetes/quarkus-standard-way

✖ io.quarkus.it.kubernetes.KnativeWithSecretConfigTest. - More details - Source on GitHub

✖ io.quarkus.it.kubernetes.KubernetesConfigWithSecretsTest. - More details - Source on GitHub

✖ io.quarkus.it.kubernetes.KubernetesWithMetricsTest. - More details - Source on GitHub

✖ io.quarkus.it.kubernetes.KubernetesWithRbacAndNamespaceTest. - More details - Source on GitHub

✖ io.quarkus.it.kubernetes.OpenshiftWithKubernetesConfigTest. - More details - Source on GitHub

✖ io.quarkus.it.kubernetes.WithKubernetesClientAndExistingResourcesTest. - More details - Source on GitHub

✖ io.quarkus.it.kubernetes.WithKubernetesClientAndRbacDisabledTest. - More details - Source on GitHub

✖ io.quarkus.it.kubernetes.WithKubernetesClientTest. - More details - Source on GitHub

📦 integration-tests/openshift-client

✖ io.quarkus.it.openshift.client.OpenShiftClientTest.createRoute - More details - Source on GitHub

⚙️ JVM Tests - JDK 11 Windows #

- Failing: integration-tests/kubernetes-client integration-tests/kubernetes-client-devservices integration-tests/kubernetes/quarkus-standard-way and 1 more