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

Bytecode that access constructor modifiers leads to automatic reflection registration #8019

Closed
galderz opened this issue Dec 14, 2023 · 4 comments

Comments

@galderz
Copy link
Contributor

galderz commented Dec 14, 2023

We're seeing Quarkus JPA tests fail with the unreleased master source code. Recently Quarkus JPA layer has added Jackson integration for dealing with JSON content, but some tests are failing when combined with upstream master. They are failing because GraalVM is added Jackson's XML layer into the native image, even if XML is not in use by the test. Quarkus cares about making sure that libraries that can introduce big surface areas to the native binary (e.g. JAXB and AWT, or Jackson and XML) do not bring those unless the user is really going to use them. The test that is failing is just telling us that a test that is not using XML suddenly encounters Jackson's XML layer and all its dependencies in the native binary universe. This can increase native binary size considerably (e.g. from 74MB to 87MB, close to 20% increase). See quarkusio/quarkus#37498 for more Quarkus specific details.

The issue started to happen when 1ed2a58 was integrated. Although the original reason why the issue was happening at the time of this commit, and the reason why is still failing with the latest master today are slightly different, in both cases it has the following impact. Jackson's DOMSerializer constructor factory method is added as an entry point, which causes all of XML layer to be included in the universe:

├── entry com.oracle.svm.core.code.FactoryMethodHolder.DOMSerializer_constructor_ed8d8ed1c0ba7927c499a8c3ec42227bb4ec6043():com.fasterxml.jackson.databind.ext.DOMSerializer id=633 
│   └── directly calls com.fasterxml.jackson.databind.ext.DOMSerializer.<init>():void id=12495 @bci=1 

If we modify the upstream master code to capture the moment the factory method for DOMSerializer is created, the stacktrace looks like this:

[FMS] Creating serializer factory method for DOMSerializer_constructor_ed8d8ed1c0ba7927c499a8c3ec42227bb4ec6043...
java.lang.Exception: Stack trace
	at java.base/java.lang.Thread.dumpStack(Thread.java:2176)
	at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.code.FactoryMethodSupport.lambda$lookup$0(FactoryMethodSupport.java:71)
	at java.base/java.util.concurrent.ConcurrentHashMap.computeIfAbsent(ConcurrentHashMap.java:1710)
	at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.code.FactoryMethodSupport.lookup(FactoryMethodSupport.java:62)
	at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.reflect.ReflectionFeature.createAccessor(ReflectionFeature.java:223)
	at java.base/java.util.concurrent.ConcurrentHashMap.computeIfAbsent(ConcurrentHashMap.java:1710)
	at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.reflect.ReflectionFeature.getOrCreateAccessor(ReflectionFeature.java:143)
	at org.graalvm.nativeimage.builder/com.oracle.svm.core.reflect.target.ExecutableAccessorComputer.transform(ExecutableAccessorComputer.java:43)
	at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.substitute.ComputedValueField.computeValue(ComputedValueField.java:367)
	at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.substitute.ComputedValueField.readValue(ComputedValueField.java:337)
	at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.ameta.ReadableJavaField.readFieldValue(ReadableJavaField.java:58)
	at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.ameta.AnalysisConstantReflectionProvider.doReadValue(AnalysisConstantReflectionProvider.java:289)
	at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.ameta.AnalysisConstantReflectionProvider.readHostedFieldValue(AnalysisConstantReflectionProvider.java:258)
	at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.heap.SVMImageHeapScanner.readHostedFieldValue(SVMImageHeapScanner.java:142)
	at org.graalvm.nativeimage.pointsto/com.oracle.graal.pointsto.heap.ImageHeapScanner.lambda$createImageHeapInstance$6(ImageHeapScanner.java:308)
	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:572)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:317)
	at org.graalvm.nativeimage.pointsto/com.oracle.graal.pointsto.util.AnalysisFuture.ensureDone(AnalysisFuture.java:69)
	at org.graalvm.nativeimage.pointsto/com.oracle.graal.pointsto.heap.ImageHeapConstant.ensureReaderInstalled(ImageHeapConstant.java:112)
	at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.ameta.AnalysisConstantReflectionProvider.readValue(AnalysisConstantReflectionProvider.java:237)
	at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.ameta.AnalysisConstantReflectionProvider.readFieldValue(AnalysisConstantReflectionProvider.java:196)
	at jdk.graal.compiler/jdk.graal.compiler.nodes.util.ConstantFoldUtil$1.readValue(ConstantFoldUtil.java:55)
	at jdk.graal.compiler/jdk.graal.compiler.core.common.spi.JavaConstantFieldProvider.readConstantField(JavaConstantFieldProvider.java:78)
	at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.meta.SharedConstantFieldProvider.readConstantField(SharedConstantFieldProvider.java:59)
	at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.ameta.AnalysisConstantFieldProvider.readConstantField(AnalysisConstantFieldProvider.java:52)
	at jdk.graal.compiler/jdk.graal.compiler.nodes.util.ConstantFoldUtil.tryConstantFold(ConstantFoldUtil.java:51)
	at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.phases.ConstantFoldLoadFieldPlugin.tryConstantFold(ConstantFoldLoadFieldPlugin.java:68)
	at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.phases.ConstantFoldLoadFieldPlugin.handleLoadField(ConstantFoldLoadFieldPlugin.java:55)
	at jdk.graal.compiler/jdk.graal.compiler.replacements.PEGraphDecoder.canonicalizeFixedNode(PEGraphDecoder.java:1560)
	at org.graalvm.nativeimage.pointsto/com.oracle.graal.pointsto.phases.InlineBeforeAnalysisGraphDecoder.canonicalizeFixedNode(InlineBeforeAnalysisGraphDecoder.java:191)
	at jdk.graal.compiler/jdk.graal.compiler.nodes.SimplifyingGraphDecoder.handleFixedNode(SimplifyingGraphDecoder.java:193)
	at jdk.graal.compiler/jdk.graal.compiler.nodes.GraphDecoder.processNextNode(GraphDecoder.java:937)
	at org.graalvm.nativeimage.pointsto/com.oracle.graal.pointsto.phases.InlineBeforeAnalysisGraphDecoder.processNextNode(InlineBeforeAnalysisGraphDecoder.java:343)
	at jdk.graal.compiler/jdk.graal.compiler.nodes.GraphDecoder.decode(GraphDecoder.java:658)
	at jdk.graal.compiler/jdk.graal.compiler.replacements.PEGraphDecoder.decode(PEGraphDecoder.java:895)
	at org.graalvm.nativeimage.pointsto/com.oracle.graal.pointsto.phases.InlineBeforeAnalysis.decodeGraph(InlineBeforeAnalysis.java:73)
	at org.graalvm.nativeimage.pointsto/com.oracle.graal.pointsto.flow.MethodTypeFlowBuilder.parse(MethodTypeFlowBuilder.java:198)
	at org.graalvm.nativeimage.pointsto/com.oracle.graal.pointsto.flow.MethodTypeFlowBuilder.apply(MethodTypeFlowBuilder.java:608)
	at org.graalvm.nativeimage.pointsto/com.oracle.graal.pointsto.flow.MethodTypeFlow.createFlowsGraph(MethodTypeFlow.java:167)
	at org.graalvm.nativeimage.pointsto/com.oracle.graal.pointsto.flow.MethodTypeFlow.ensureFlowsGraphCreated(MethodTypeFlow.java:152)
	at org.graalvm.nativeimage.pointsto/com.oracle.graal.pointsto.flow.MethodTypeFlow.getOrCreateMethodFlowsGraphInfo(MethodTypeFlow.java:110)
	at org.graalvm.nativeimage.pointsto/com.oracle.graal.pointsto.typestate.DefaultSpecialInvokeTypeFlow.lambda$onObservedUpdate$0(DefaultSpecialInvokeTypeFlow.java:88)
	at org.graalvm.nativeimage.pointsto/com.oracle.graal.pointsto.util.LightImmutableCollection.forEach(LightImmutableCollection.java:90)
	at org.graalvm.nativeimage.pointsto/com.oracle.graal.pointsto.typestate.DefaultSpecialInvokeTypeFlow.onObservedUpdate(DefaultSpecialInvokeTypeFlow.java:87)
	at org.graalvm.nativeimage.pointsto/com.oracle.graal.pointsto.flow.TypeFlow.update(TypeFlow.java:628)
	at org.graalvm.nativeimage.pointsto/com.oracle.graal.pointsto.PointsToAnalysis$1.run(PointsToAnalysis.java:538)
	at org.graalvm.nativeimage.pointsto/com.oracle.graal.pointsto.util.CompletionExecutor.executeCommand(CompletionExecutor.java:169)
	at org.graalvm.nativeimage.pointsto/com.oracle.graal.pointsto.util.CompletionExecutor.lambda$executeService$0(CompletionExecutor.java:154)
	at java.base/java.util.concurrent.ForkJoinTask$RunnableExecuteAction.compute(ForkJoinTask.java:1726)
	at java.base/java.util.concurrent.ForkJoinTask$RunnableExecuteAction.compute(ForkJoinTask.java:1717)
	at java.base/java.util.concurrent.ForkJoinTask$InterruptibleTask.exec(ForkJoinTask.java:1641)
	at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:507)
	at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1486)
	at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:2077)
	at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:2028)
	at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:187)

Though some further debugging we discovered that the node that is causing this to trigger is LoadField#modifiers, which refers to bycode that attempts to access the class' constructor modifiers. Looking at the Jackson code we see this in the source code that can instantiate DOMSerializer:

    public static <T> Constructor<T> findConstructor(Class<T> cls, boolean forceAccess)
        throws IllegalArgumentException
    {
        try {
            Constructor<T> ctor = cls.getDeclaredConstructor();
            if (forceAccess) {
                checkAndFixAccess(ctor, forceAccess);
            } else {
                // Has to be public...
                if (!Modifier.isPublic(ctor.getModifiers())) {
                    throw new IllegalArgumentException("Default constructor for "+cls.getName()+" is not accessible (non-public?): not allowed to try modify access via Reflection: cannot instantiate type");
                }
            }
            return ctor;
        } catch (NoSuchMethodException e) {
            ;
        } catch (Exception e) {
            ClassUtil.unwrapAndThrowAsIAE(e, "Failed to find default constructor of class "+cls.getName()+", problem: "+e.getMessage());
        }
        return null;
    }

We've been speaking with @cstancu who has confirmed that accessing ctror.getModifiers() is what is triggering the factory method to be created. It also immediately registers it for reflection.

We have been able to reproduce this exact issue outside of Quarkus in this small project. This project mimics the scenario by having a serializer factory that can instantiate 2 types of serializers, AliceSerializer and BobSerializer. Which one you will need is not known at build time since it depends on a runtime argument. You can replicate the issue by just calling mvn clean install on the project. Then you can inspect the call tree and you see:

├── entry com.oracle.svm.core.code.FactoryMethodHolder.AliceSerializer_constructor_52f6a11a2ded63c5c025376fbade9cb94077c91d():org.acme.AliceSerializer id=35 
├── entry com.oracle.svm.core.code.FactoryMethodHolder.BobSerializer_constructor_f196f2173527878a08664c0f5447c5735d26c12c():org.acme.BobSerializer id=38 

Compared with the latest released version GraalVM for JDK 21, these factory methods will not be there unless the user specifically registers these classes for reflection.

@christianwimmer
Copy link

Thanks for the detailed report and evaluation, we will fix this before GraalVM for JDK 22 is released.

@cstancu
Copy link
Member

cstancu commented Dec 17, 2023

The reported issue is fixed by #8041. Now we do the reflection registration lazily, only when the accessor object is reachable, not when any of the constructor fields are accessed.

@cstancu cstancu closed this as completed Dec 17, 2023
@galderz
Copy link
Contributor Author

galderz commented Dec 19, 2023

I've quickly run things locally and things look good on my side. Nightly jobs will confirm this in the next few days.

@galderz
Copy link
Contributor Author

galderz commented Dec 19, 2023

Thanks a lot @cstancu for the fix :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants