search

oracle / graal

GraalVM compiles Java applications into native executables that start instantly, scale fast, and use fewer compute resources 🚀
https://www.graalvm.org
Other
20.27k stars 1.63k forks source link

Fix JFR substitutions for JDK Compatibility #9202

Closed roberttoyonaga closed 3 months ago

roberttoyonaga commented 3 months ago

Related Issue: https://github.com/oracle/graal/issues/9199

A recent JDK change has caused some JFR substitutions to go out-of-date. Native Image binaries with JFR currently will not build with the latest LabsJDK. This PR fixes those substitutions.

roberttoyonaga commented 3 months ago

@zapster Can you please have a look at this one when you have time?

roberttoyonaga commented 3 months ago

Looks like the GHA gates are failing due to:

Internal exception: com.oracle.svm.core.util.UserError$UserException: Substitution target for com.oracle.svm.core.jfr.Target_jdk_jfr_internal_HiddenWait is not loaded. Use field `onlyWith` in the `TargetClass` annotation to make substitution only active when needed.

Maybe the GHAs are not using the latest LabsJDK? Installing labsjdk-ce-latest-24+2-jvmci-b01 to jdk-dl/labsjdk-ce-latest-24+2-jvmci-b01... It seems to work fine when I use labsjdk-ce-24+3.

zapster commented 3 months ago

Thanks @roberttoyonaga for your contribution! We have a similar adoption of this change in our JDK update PR #9181. In general, it always makes sense to look for such JDK update PRs when it comes to recent JDK change adoptions and potentially start discussions there.

In our adoption, we also ported the javaTimeSystemUTC method, because it has a higher resolution. (See also my inline comment.) From the only existing usage in jdk.jfr.internal.MetadataRepository#awaitEpochMilliShift, having only millisecond resolution seems fine, which makes me think we should rather use your approach. However, I'm worried that JVM.nanosNow() might be used by other methods in the future, which require sub-millisecond accuracy. Such new usages would be hard to detect. What do you think?

cc @christianhaeubl

roberttoyonaga commented 3 months ago

Closing this PR because the implementation is already done by another.