-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[GR-49852] [GR-49613] Update JVMCI to 22+22-jvmci-b01. #7748
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
Conversation
b314c0f
to
a7403ed
Compare
if (JavaVersionUtil.JAVA_SPEC < 22) { | ||
mirrorEventMapping = createMirrorEventsMapping(); | ||
} else { | ||
mirrorEventMapping = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For JDK 22, I think you will still need to call createMirrorEventsMapping()
. But createMirrorEventsMapping()
will need ReflectionUtil.lookupClass(false, "jdk.jfr.internal.MirrorEvents");
instead of ReflectionUtil.lookupClass(false, "jdk.jfr.internal.instrument.JDKEvents");
depending on what JavaVersionUtil.JAVA_SPEC
returns.
Additionally, I think an @Alias
will be required for jdk.jfr.internal.instrument.JDKEvents#mirrorEventClasses
similar to what already exists in Target_jdk_jfr_internal_instrument_JDKEvents
for JDK 21.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for the suggestions @roberttoyonaga! Interestingly, the mirrored events are not covered by our testing.
Since the JDK update is rather critical, we plan to merge this PR as is and deal with the remaining JFR changes in a follow-up PR. We should also add some testing so that we cover these cases in our gate jobs.
If you could prepare a PR, we would very much appreciate it as it would speed up things. I can let you know as soon as the labjdk builds are publicly available, so you can start on top of this PR without waiting for it to merge.
Otherwise, if you don't have time, I can prepare a PR as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The labsJDK 22+22 artifacts are available: https://github.com/graalvm/labs-openjdk/releases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you could prepare a PR, we would very much appreciate it as it would speed up things.
Hi @zapster! Yes, I'll prepare a PR for these JFR updates
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @zapster, on second thought, we may not actually need to do much further to accommodate the changes related to MirrorEvents.java
. The new behaviour is that jdk.jfr.internal.MirrorEvents
maintains a cache of all the mirror event classes, instead of that cache needing to be built in JDKEvents
. This means the extra step of using SecuritySupport#registerMirror
is unnecessary in JDK22. So now we also don't have to do this extra step at native image build-time before manually retransforming the JFR event classes in JfrEventSubstitution
anymore.
I made a PR that adds a test to verify mirror events are handled correctly and also adds a substitution to reset MirrorEvent#mirrorEventClasses
. #7771
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks a lot!
…t.java failed with "InterruptedException: sleep interrupted""
62922fe
to
07f8f90
Compare
Update to 22+22-jvmci-b01
ForkJoinPool#invokeAll
method and thus is making JDK 22 source incompatible with previous JDKs. This is not only a problem for native-image but for all Java code that subclassForkJoinPool
.