8289524: Add JFR JIT restart event

[MBaesken]

backport of 8289524

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8289524: Add JFR JIT restart event

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk17u-dev pull/853/head:pull/853
$ git checkout pull/853

Update a local copy of the PR:
$ git checkout pull/853
$ git pull https://git.openjdk.org/jdk17u-dev pull/853/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 853

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk17u-dev/pull/853.diff

[bridgekeeper]

👋 Welcome back mbaesken! 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]

This backport pull request has now been updated with issue from the original commit.

[openjdk]

@MBaesken This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8289524: Add JFR JIT restart event

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[mlbridge]

Webrevs

• 00: Full (bf0b67c7)

[egahlin]

This is an enhancement. The bar for backporting events should be high. This PR also changes metadata of an existing event (adds a new field) which increases the risk for issues. The event is enabled by default, so it will have an impact on existing recordings running in production. It also contains a bug (JDK-8295419, fixed in mainline).

[MBaesken]

I am aware of the name change done in JDK-8295419, would backport this afterwards.
Regarding

This PR also changes metadata of an existing event

Yes sure it does , if it is such a bad concern it would be possible to separate the CodeCacheFull related change from the backport, not sure if this is really an issue in practise.

[egahlin]

Why is this enhancement needed in an update release?

I don't doubt the event has some use, all have or they would not be integrated into mainline. I just don't see what makes this event special. If a user upgrades to get the latest security fixes, and this event (or similar) is included and for some reason causes issues, how can it be explained? Why is it so important?

[MBaesken]

Having a couple of additional interesting (and not too complex !) events in the latest LTS is not a bad thing but this is my personal impression. On the other hand I do not care that much, if it is rejected than probably all additional events would be rejected because the others I looked at that were interesting were more complex.
Our users will avoid non-LTS releases because of the effort and investment costs , so this means Sept 2023 + probably ~ one more year of testing etc. , until they can consume other interesting JFR events that came in after 17 , quite a long time I think.

[GoeLin]

Hi
I would second Matthias' pledge.
The changes to non-jfr code by this change are minimal and trivial.
The risk of breaking something thus is small.
The event is only a small improvement for side-case, if properly
setup a vm should not start and stop the JIT. Nevertheless it
might be useful analyzing systems in abnormal states.
Tools using JFR data should be able to handle added fields.
The cited "bug" nor really is a bug, it is about spelling of commands.
It should be backported, too. But it can not be taken as proof
for this being a complicated, risky change.

[egahlin]

Having a couple of additional interesting (and not too complex !) events in the latest LTS is not a bad thing but this is my personal impression. On the other hand I do not care that much, if it is rejected than probably all additional events would be rejected because the others I looked at that were interesting were more complex.

I think the bar should be high and there should be a good reason, for example to support new hardware or OS versions. Improve security. Things that are essential to be able to use the existing JDK release. I don't see this JFR event fall into that category.

[RealCLanger]

Having a couple of additional interesting (and not too complex !) events in the latest LTS is not a bad thing but this is my personal impression. On the other hand I do not care that much, if it is rejected than probably all additional events would be rejected because the others I looked at that were interesting were more complex.

I think the bar should be high and there should be a good reason, for example to support new hardware or OS versions. Improve security. Things that are essential to be able to use the existing JDK release. I don't see this JFR event fall into that category.

I know there have been discussions about backporting JFR events earlier on and I think none have ever happened. But I think we should constantly reconsider our decisions. Regarding JFR event backports, we have been very conservative overall and I think that is not bad. We need to be careful regarding stability and not to break things. Also, major feature improvements need to come with newer JDK releases. However, we still need and want to support our customers that are running on LTS releases of the OpenJDK. And that's what additional/improved JFR events will help to achieve. So, while we should err on the path of caution as to not break anything, I think we should not reject JFR enhancements per se. At least that's my current thinking...

[tstuefe]

@egahlin do you have any concrete concerns regarding the safety or risk of this patch? Or would it make the produced JFR output incompatible with JMC or other readers in any way?

If it is just a general concern about downporting practices, I vote for downporting it. As maintainers we have a high interest in keeping 17u stable, but we also need to have tools to analyze problems in the field.

[MBaesken]

I know there have been discussions about backporting JFR events earlier on and I think none have ever happened.

We had a few backports to 11 (8003209 even backported to 8) in the past

https://bugs.openjdk.org/browse/JDK-8213617
(11.0.4)

https://bugs.openjdk.org/browse/JDK-8223438
(11.0.6)

https://bugs.openjdk.org/browse/JDK-8003209
(11.0.1)

The last one looks like a rather complex event, as far as I can see ...
Two of my examples added events, one just a field to an existing event. Not aware of any tools crashing/stopped working because of those additions (but to be fair, I do not monitor all tools that deal with JFR events).
See also
https://github.com/openjdk/jdk11u-dev/commits/master/src/hotspot/share/jfr/metadata/metadata.xml
to get more info about the topic.

Maybe regarding this event, as a compromise we could wait until jdk20 is released (includes JDK-8289524) and then some time; and then do the backport.

[egahlin]

@egahlin do you have any concrete concerns regarding the safety or risk of this patch? Or would it make the produced JFR output incompatible with JMC or other readers in any way?

I don't know the impact on JMC or other tools consuming JFR data.

To know, one would need to try them out, but any modification of the event metadata, or what is enabled in the default configuration, risks being a problem.

When backporting events several releases back, it also becomes confusing for JFR API users since the event will be missing in releases between. In this case, JDK 18 and JDK 19.

If it is just a general concern about downporting practices, I vote for downporting it. As maintainers we have a high interest in keeping 17u stable, but we also need to have tools to analyze problems in the field.

I like to know what problem this event solves?

If there have been say 3-5 JVM bugs where this event would have resolved them significant faster, I think an argument can be made (and explained to a customer why it was added in a security update). It contributes to stability (faster fixes). On the other hand, adding events to get features 10 months faster into a LTS release doesn't sound like the right reason to me.

[TheRealMDoerr]

The EventJitRestart is the counterpart of the EventCodeCacheFull. Those who want to see the latter will probably also want to see the first one. Otherwise, you can't see when the problem is solved (performance issue or other issues related to full code cache). It may be needed to understand other events.
It is legitimate to omit non-LTS releases, but not so nice. JDK 18 is out of support.
I think a JFR user should try it and comment about the usefulness.

[MBaesken]

it is labeled clean so I got the info from Goetz Lindenmaier that it can be integrated.

[MBaesken]

/integrate

[openjdk]

Going to push as commit 10b7a6d.
Since your change was applied there have been 33 commits pushed to the master branch:

• 8ce7bb4: 8029633: Raw inner class constructor ref should not perform diamond inference
• cfedd3f: 8294307: ISO 4217 Amendment 173 Update
• 175bd05: 8285794: AsyncGetCallTrace might acquire a lock via JavaThread::thread_from_jni_environment
• 3486bb2: 8295872: [PPC64] JfrGetCallTrace: Need pc == nullptr check before frame constructor
• 7d17479: 8296480: java/security/cert/pkix/policyChanges/TestPolicy.java is failing
• b5710c4: 8296108: (tz) Update Timezone Data to 2022f
• f29ade7: 8286872: Refactor add/modify notification icon (TrayIcon)
• cd4cc5d: 8255439: System Tray icons get corrupted when Windows scaling changes
• 3d18b23: 8202836: [macosx] test java/awt/Graphics/TextAAHintsTest.java fails
• 386f714: 8274029: Remove jtreg tag manual=yesno for java/awt/print/Dialog/DialogOrient.java
• ... and 23 more: https://git.openjdk.org/jdk17u-dev/compare/e60939850e5328b9c0f2002ac5ed7744375bf18b...master

Your commit was automatically rebased without conflicts.

[openjdk]

@MBaesken Pushed as commit 10b7a6d.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.