8382332: Large regression (~50%) in SPECjbb2015 when using JFR after JEP 500 by mgronlun · Pull Request #30922 · openjdk/jdk

mgronlun · 2026-04-24T13:24:12Z

Greetings,

please see JDK-8382332 for detailed information about this issue.

Testing: jdk_jfr, tier1-6, SpecJbb2015

Thanks
Markus

I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8382332: Large regression (~50%) in SPECjbb2015 when using JFR after JEP 500 (Bug - P2)

Reviewers

Alan Bateman (@AlanBateman - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/30922/head:pull/30922
$ git checkout pull/30922

Update a local copy of the PR:
$ git checkout pull/30922
$ git pull https://git.openjdk.org/jdk.git pull/30922/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 30922

View PR using the GUI difftool:
$ git pr show -t 30922

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/30922.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2026-04-24T13:25:18Z

👋 Welcome back mgronlun! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2026-04-24T13:26:14Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2026-04-24T13:27:06Z

@mgronlun The following labels will be automatically applied to this pull request:

core-libs
hotspot
security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

openjdk · 2026-04-24T13:27:09Z

The total number of required reviews for this PR has been set to 2 based on the presence of this label: hotspot. This can be overridden with the /reviewers command.

mgronlun · 2026-04-24T13:28:08Z

/label remove security

mgronlun · 2026-04-24T13:28:17Z

/label add hotspot-jfr

openjdk · 2026-04-24T13:28:34Z

@mgronlun
The security label was successfully removed.

openjdk · 2026-04-24T13:29:17Z

@mgronlun
The hotspot-jfr label was successfully added.

mlbridge · 2026-04-24T13:32:17Z

Webrevs

AlanBateman · 2026-04-25T16:18:09Z

While there are many unused bits in the slot field, there is something a bit uncomfortable with encoding a the epoch generation into a final field. Did you consider adding an additional field, or even an injected field that is conditionally compiled in with #if INCLUDE_JFR ?

merykitty · 2026-04-25T19:17:32Z

+            if (currentEpoch != (slot >> JfrEpochGeneration.EPOCH_SHIFT_INT)) {
+                int newRootFieldSlotEpoch =
+                    currentEpoch << JfrEpochGeneration.EPOCH_SHIFT_INT | (slot & JfrEpochGeneration.VALUE_MASK_INT);
+                return JfrEpochGeneration.compareAndExchange(root, SLOT_OFFSET, slot, newRootFieldSlotEpoch);


Drive-by-comment: Can you make slot non-final? I'm afraid there is a chance the JIT will not produce correct code for accesses into this field if it lies about its finality. See JDK-8379875.

I will leave the slot final, and instead inject a dedicated field for the epoch value.

mgronlun · 2026-04-25T19:57:35Z

Adding an injected field for the epoch generation will be safer, of course. Sometimes, we try to preserve the footprint, and the encoding scheme might need to be implemented in existing fields to maintain it. If that is not considered a problem here, I will add an injected field.

egahlin · 2026-04-26T13:10:52Z

I think it would be good to mention in the event description that only one event per field, per chunk, is recorded, and there might be more mutations, possibly with different stack traces.

Perhaps rename the label to make it clear, e.g. "Final Field Mutation Sample", or something similar.

mgronlun · 2026-04-26T19:52:12Z

Attempting to add an injected field did not work because you cannot reflect an injected field, not even using Unsafe. So I cannot read the value of an injected field from Java. Either we go with the encoding in the slot field, or I will add a separate non-final field "jfrEpoch" to java.lang.reflect.Field. Thoughts?

mgronlun · 2026-04-27T08:05:22Z

Updated with an alternative provided by @merykitty to make the slot non-final. As well as @egahlin's suggestions for better event descriptions.

If we don't want to use the "slot" field, we must introduce another non-final int field, explicitly (not injected), dedicated to holding the encoded epoch. It can be done, I have no preferences either way.

AlanBateman · 2026-04-27T08:07:43Z

Attempting to add an injected field did not work because you cannot reflect an injected field, not even using Unsafe. So I cannot read the value of an injected field from Java. Either we go with the encoding in the slot field, or I will add a separate non-final field "jfrEpoch" to java.lang.reflect.Field. Thoughts?

I assume an injected field would be feasible if all the access were via JVM funtions rather than reflect/Unsafe but that would add complexity as it might end up needing instrinics. In that case, an explicit field would be better so the slot field is not overridden.

mgronlun · 2026-04-27T08:08:42Z

Attempting to add an injected field did not work because you cannot reflect an injected field, not even using Unsafe. So I cannot read the value of an injected field from Java. Either we go with the encoding in the slot field, or I will add a separate non-final field "jfrEpoch" to java.lang.reflect.Field. Thoughts?

I assume an injected field would be feasible if all the access were via JVM funtions rather than reflect/Unsafe but that would add complexity as it might end up needing instrinics. In that case, an explicit field would be better so the slot field is not overridden.

Sounds like @AlanBateman suggests a dedicated field for the jfrEpoch - coming up.

AlanBateman · 2026-04-27T08:10:17Z

-@Label("Final Field Mutation")
+@Label("Final Field Mutation Sample")
+@Description("One event per field, per chunk, is recorded." +
+        "There might be more mutations, possibly with different stack traces")


The description is "developer facing" and I wonder if this will be understandable.

Right - will reformulate it.

mgronlun · 2026-04-27T08:23:57Z

Attempting to add an injected field did not work because you cannot reflect an injected field, not even using Unsafe. So I cannot read the value of an injected field from Java. Either we go with the encoding in the slot field, or I will add a separate non-final field "jfrEpoch" to java.lang.reflect.Field. Thoughts?

I assume an injected field would be feasible if all the access were via JVM funtions rather than reflect/Unsafe but that would add complexity as it might end up needing instrinics. In that case, an explicit field would be better so the slot field is not overridden.

Yes, we can do an intrinsic too, if we really want to tuck this away. I can look into that alternative as well, since I already built similar intrinsics to read the jfrEpoch, which are currently in use for virtual threads.

mgronlun · 2026-04-27T08:49:45Z

Attempting to add an injected field did not work because you cannot reflect an injected field, not even using Unsafe. So I cannot read the value of an injected field from Java. Either we go with the encoding in the slot field, or I will add a separate non-final field "jfrEpoch" to java.lang.reflect.Field. Thoughts?

I assume an injected field would be feasible if all the access were via JVM funtions rather than reflect/Unsafe but that would add complexity as it might end up needing instrinics. In that case, an explicit field would be better so the slot field is not overridden.

Yes, we can do an intrinsic too, if we really want to tuck this away. I can look into that alternative as well, since I already built similar intrinsics to read the jfrEpoch, which are currently in use for virtual threads.

Ok, let's try this design, which can be generalized more broadly: we inject a field into those classes that need this support. The JfrEpochGeneration class will get an intrinsic to read and potentially cas-update any instance passed to it.

openjdk · 2026-05-01T13:09:38Z

@mgronlun security has been added to this pull request based on files touched in new commit(s).

mgronlun · 2026-05-01T15:31:01Z

Updated with a solution that uses injected fields and extendable intrinsics.

mgronlun · 2026-05-01T15:43:21Z

/label remove security

openjdk · 2026-05-01T15:44:34Z

@mgronlun
The security label was successfully removed.

AlanBateman

I did a pass over the updated implementation that uses the injected fields and it looks good.

AlanBateman · 2026-05-07T11:31:25Z

+            // Epoch 4.
+            validate(secondRecording, 3);
+            secondRecording.close();
+            validate(firstRecording, 9);


I haven't seen a test that has two recordings in use at the same time. The event isn't explicitly enabled for secondRecording, is the nesting relevant here? Also does the stop + start guaranteed to bump the epoch?

We have many JFR tests that start multiple, nested recordings. Since a recording cannot turn off an event enabled by another recording, and there is only one recording stream in total, the second recording inherits the enabled event (the event setting is a union of 1 v 0), which means the event is also enabled for the subsequent recording.

Start and stop are both implemented as rotations, and rotations use a stop-the-world safepoint to bump the epoch.

I wasn't sure how it worked with concurrent recordings so your message is useful, thanks. I will note that I did read through the Recording API docs but couldn't find anything obvious that documents this.

It's really an implementation detail.

AlanBateman · 2026-05-07T12:56:32Z

        int len = next_length();  // 0 or length in [1..5]
        if (len == 0)  break;
        _position += len;
+        ++actual;


This looks like a pre-existing bug, should it be tracked separately?

Its a pre-existing bug, indeed. I will refactor it.

Refactored into #31077

Split out into; https://bugs.openjdk.org/browse/JDK-8384100

AlanBateman

I don't have any other comments, thanks for taking this on.

mgronlun · 2026-05-07T16:16:18Z

I don't have any other comments, thanks for taking this on.

Many thanks for reviewing this, @AlanBateman. Cheers.

vnkozlov · 2026-05-07T20:54:29Z

@mgronlun how important jdk.jfr.internal.JVM::getClassId?

While looking on external addresses referenced in C2 intrinsics we noticed that JFR's getClassId is not generated:

1214  456    b    4       jdk.jfr.jvm.TestJFRIntrinsic::getClassIdIntrinsic (22 bytes)
                              @ 1   jdk.jfr.internal.JVM::getClassId (0 bytes)   failed to inline: native method

_getClassId is missing in LibraryCallKit::try_to_inline() and C2Compiler::is_intrinsic_supported(). Even so we have
LibraryCallKit::inline_native_classID().

mgronlun added 2 commits April 24, 2026 00:18

downthrottler

2b09a97

test

6b369a4

openjdk Bot added security security-dev@openjdk.org hotspot hotspot-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Apr 24, 2026

openjdk Bot added the rfr Pull request is ready for review label Apr 24, 2026

openjdk Bot removed the security security-dev@openjdk.org label Apr 24, 2026

openjdk Bot added the hotspot-jfr hotspot-jfr-dev@openjdk.org label Apr 24, 2026

macros

44a92da

merykitty reviewed Apr 25, 2026

View reviewed changes

make slot field non-final + sample descriptions

8ce984b

AlanBateman reviewed Apr 27, 2026

View reviewed changes

more general event description

c958f4a

be explicit about final

b69b58d

intrinsics

3a3a944

openjdk Bot added the security security-dev@openjdk.org label May 1, 2026

openjdk Bot removed the security security-dev@openjdk.org label May 1, 2026

AlanBateman reviewed May 7, 2026

View reviewed changes

mgronlun added 2 commits May 7, 2026 17:12

Remove increment of actual in loop

055c8ca

Split out into; https://bugs.openjdk.org/browse/JDK-8384100

Update copyright year in unsigned5.hpp

b43a33c

Split out into; https://bugs.openjdk.org/browse/JDK-8384100

AlanBateman approved these changes May 7, 2026

View reviewed changes

Conversation

mgronlun commented Apr 24, 2026 • edited by openjdk Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper Bot commented Apr 24, 2026

Uh oh!

openjdk Bot commented Apr 24, 2026

Uh oh!

openjdk Bot commented Apr 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk Bot commented Apr 24, 2026

Uh oh!

mgronlun commented Apr 24, 2026

Uh oh!

mgronlun commented Apr 24, 2026

Uh oh!

openjdk Bot commented Apr 24, 2026

Uh oh!

openjdk Bot commented Apr 24, 2026

Uh oh!

mlbridge Bot commented Apr 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

AlanBateman commented Apr 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

merykitty Apr 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgronlun commented Apr 25, 2026

Uh oh!

egahlin commented Apr 26, 2026

Uh oh!

mgronlun commented Apr 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mgronlun commented Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlanBateman commented Apr 27, 2026

Uh oh!

mgronlun commented Apr 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgronlun commented Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mgronlun commented Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk Bot commented May 1, 2026

Uh oh!

mgronlun commented May 1, 2026

Uh oh!

mgronlun commented May 1, 2026

Uh oh!

openjdk Bot commented May 1, 2026

Uh oh!

AlanBateman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgronlun May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mgronlun commented Apr 24, 2026 •

edited by openjdk Bot

Loading

openjdk Bot commented Apr 24, 2026 •

edited

Loading

mlbridge Bot commented Apr 24, 2026 •

edited

Loading

AlanBateman commented Apr 25, 2026 •

edited

Loading

merykitty Apr 25, 2026 •

edited

Loading

mgronlun commented Apr 26, 2026 •

edited

Loading

mgronlun commented Apr 27, 2026 •

edited

Loading

mgronlun commented Apr 27, 2026 •

edited

Loading

mgronlun commented Apr 27, 2026 •

edited

Loading

mgronlun May 7, 2026 •

edited

Loading