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

Fix native compilation for Netty and Mutiny #39830

Merged
merged 1 commit into from Apr 3, 2024

Conversation

jponge
Copy link
Member

@jponge jponge commented Apr 2, 2024

See #39819 and #39788

@jponge jponge force-pushed the fix/netty-mutiny-native-39819 branch from b90033b to b35ae00 Compare April 2, 2024 11:43
@jponge
Copy link
Member Author

jponge commented Apr 2, 2024

Note: updated with a substitution for UnsafeRefArrayAccess / ignoring it could yield to segfaults as the field offset couldn't be computed (it uses a non-classic Unsafe access pattern).

@zakkak
Copy link
Contributor

zakkak commented Apr 2, 2024

Note: updated with a substitution for UnsafeRefArrayAccess / ignoring it could yield to segfaults as the field offset couldn't be computed (it uses a non-classic Unsafe access pattern).

@jponge I had a look at org.graalvm.nativeimage.hosted.Feature.BeforeAnalysisAccess#registerAsUnsafeAccessed and it looks like it's mostly being used by Truffle.

Unsafe accesses seem to be automatically detected and registered by GraalVM, e.g. https://github.com/oracle/graal/blob/c95781d03099ebd1dad495ac1fefca863178b358/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/MethodTypeFlowBuilder.java#L304

Does this mean we can follow this pattern and completely remove UnsafeAccessedFieldBuildItems in favor of RecomputeFieldValue?

@zakkak
Copy link
Contributor

zakkak commented Apr 2, 2024

@jponge please have a look at #39831 as well before merging this. It makes the mutiny changes redundant.

@jponge
Copy link
Member Author

jponge commented Apr 2, 2024

Does this mean we can follow this pattern and completely remove UnsafeAccessedFieldBuildItems in favor of RecomputeFieldValue?

AFAIK most Unsafe patterns indeed get detected now, but one such as UnsafeRefArrayAccess.REF_ELEMENT_SHIFT is not, hence the need for either a substitution or a UnsafeAccessedFieldBuildItem that does not involved build-time init

@quarkus-bot
Copy link

quarkus-bot bot commented Apr 2, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit b35ae00.

Failing Jobs

Status Name Step Failures Logs Raw logs Build scan
✔️ JVM Tests - JDK 17 Logs Raw logs 🚧
JVM Tests - JDK 17 Windows Build Failures Logs Raw logs 🚧
✔️ JVM Tests - JDK 21 Logs Raw logs 🚧

Failures

⚙️ JVM Tests - JDK 17 Windows #

- Failing: integration-tests/grpc-hibernate-reactive 

📦 integration-tests/grpc-hibernate-reactive

Failed to execute goal io.quarkus:quarkus-maven-plugin:999-SNAPSHOT:build (default) on project quarkus-integration-test-grpc-hibernate-reactive: Failed to build quarkus application


Flaky tests - Develocity

⚙️ Maven Tests - JDK 17 Windows

📦 integration-tests/maven

io.quarkus.maven.it.TestMojoIT.testThatTheTestsAreReRunMultiModule - History

  • Condition with Lambda expression in io.quarkus.maven.it.continuoustesting.TestModeContinuousTestingMavenTestUtils was not fulfilled within 3 minutes. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in io.quarkus.maven.it.continuoustesting.TestModeContinuousTestingMavenTestUtils was not fulfilled within 3 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:26)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:975)
	at io.quarkus.maven.it.continuoustesting.TestModeContinuousTestingMavenTestUtils.waitForNextCompletion(TestModeContinuousTestingMavenTestUtils.java:49)
	at io.quarkus.maven.it.LaunchMojoTestBase.testThatTheTestsAreReRunMultiModule(LaunchMojoTestBase.java:56)

io.quarkus.maven.it.TestMojoIT.testThatTheTestsAreReRunMultiModule - History

  • Condition with Lambda expression in io.quarkus.maven.it.continuoustesting.TestModeContinuousTestingMavenTestUtils was not fulfilled within 3 minutes. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in io.quarkus.maven.it.continuoustesting.TestModeContinuousTestingMavenTestUtils was not fulfilled within 3 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:26)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:975)
	at io.quarkus.maven.it.continuoustesting.TestModeContinuousTestingMavenTestUtils.waitForNextCompletion(TestModeContinuousTestingMavenTestUtils.java:49)
	at io.quarkus.maven.it.LaunchMojoTestBase.testThatTheTestsAreReRunMultiModule(LaunchMojoTestBase.java:56)

@cescoffier
Copy link
Member

Does this mean we can follow this pattern and completely remove UnsafeAccessedFieldBuildItems in favor of RecomputeFieldValue?

I would be in favor of this. The UnsafeAccessedFieldBuildItems leads to loading the class at build time (and so run the initialization of the class) which can lead to issues if this class must be runtime only. I wonder if we can workaround this using "re-initialize at runtime".

@zakkak
Copy link
Contributor

zakkak commented Apr 3, 2024

The UnsafeAccessedFieldBuildItems leads to loading the class at build time (and so run the initialization of the class) which can lead to issues if this class must be runtime only. I wonder if we can workaround this using "re-initialize at runtime".

FYI I have a fix for this behavior in #39831, but it's still WIP since it triggers other issues.

Copy link
Contributor

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

LGTM

@geoand geoand merged commit e1cc9a2 into quarkusio:main Apr 3, 2024
48 of 49 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.10 - main milestone Apr 3, 2024
@jponge
Copy link
Member Author

jponge commented Apr 3, 2024

One down! 🤣

@geoand
Copy link
Contributor

geoand commented Apr 3, 2024

🎉

@luneo7
Copy link
Contributor

luneo7 commented Apr 3, 2024

should this be backported to 3.8.4 as well? since vertx and netty were also upgraded there

@geoand
Copy link
Contributor

geoand commented Apr 3, 2024

Makes sense

@jponge
Copy link
Member Author

jponge commented Apr 3, 2024

Yes please!

@gsmet gsmet modified the milestones: 3.10 - main, 3.9.3 Apr 9, 2024
@gsmet
Copy link
Member

gsmet commented Apr 9, 2024

@jponge I don't think this can be backported as is to 3.9 and 3.8.

At least the commit with JCTools is causing issues as I think you are only using this with latest Mutiny?

Error: Substitution target for io.quarkus.mutiny.runtime.Target_org_jctools_util_UnsafeRefArrayAccess is not loaded. Use field `onlyWith` in the `TargetClass` annotation to make substitution only active when needed.
com.oracle.svm.core.util.UserError$UserException: Substitution target for io.quarkus.mutiny.runtime.Target_org_jctools_util_UnsafeRefArrayAccess is not loaded. Use field `onlyWith` in the `TargetClass` annotation to make substitution only active when needed.
	at org.graalvm.nativeimage.builder/com.oracle.svm.core.util.UserError.abort(UserError.java:73)
	at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.substitute.AnnotationSubstitutionProcessor.findTargetClass(AnnotationSubstitutionProcessor.java:1074)
	at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.substitute.AnnotationSubstitutionProcessor.handleClass(AnnotationSubstitutionProcessor.java:373)
	at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.substitute.AnnotationSubstitutionProcessor.init(AnnotationSubstitutionProcessor.java:351)
	at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.NativeImageGenerator.createAnnotationSubstitutionProcessor(NativeImageGenerator.java:1029)
	at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.NativeImageGenerator.setupNativeImage(NativeImageGenerator.java:907)
	at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.NativeImageGenerator.doRun(NativeImageGenerator.java:590)
	at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.NativeImageGenerator.run(NativeImageGenerator.java:550)
	at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.NativeImageGeneratorRunner.buildImage(NativeImageGeneratorRunner.java:539)
	at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.NativeImageGeneratorRunner.build(NativeImageGeneratorRunner.java:721)
	at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.NativeImageGeneratorRunner.start(NativeImageGeneratorRunner.java:143)
	at org.graalvm.nativeimage.builder/com.oracle.svm.hosted.NativeImageGeneratorRunner.main(NativeImageGeneratorRunner.java:98)

@jponge
Copy link
Member Author

jponge commented Apr 9, 2024

(discussed offline - yes 👍 )

@gsmet
Copy link
Member

gsmet commented Apr 9, 2024

For 3.9 and 3.8, only the Netty changes should be backported.

See this commit: d2673c8

@gsmet gsmet modified the milestones: 3.9.3, 3.8.4 Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Netty update caused failure when building image with GraalVM for JDK 17
6 participants