8351142: Add JFR monitor deflation and statistics events

[shipilev]

We already have JFR JavaMonitorInflate event, which tells when the monitor is inflated. We are missing JavaMonitorDeflate event, which would tell us when the monitor is deflated. This makes it hard to see the monitor lifecycle, and/or estimate the population of currently inflated monitors. I believe we should add JavaMonitorDeflate event. It would also be useful to have the statistics for the number of currently used/deflating monitors. Deflation event alone would require post-processing to investigate this, so it would be good to have the statistics event as well.

This would also replace two of the RT counters that are going away in JDK-8348829.

Monitor deflation is done asynchronously in MonitorDeflationThread, so the additional overhead of recording the deflation events would likely be performance neutral. We still only enable the statistics event by default to be on a safer side.

Additional testing:

• Linux x86_64 server fastdebug, jdk_jfr

---

Progress

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

Issue

• JDK-8351142: Add JFR monitor deflation and statistics events (Enhancement - P4)

Reviewers

• David Holmes (@dholmes-ora - Reviewer) Review applies to 8102aed8
• Leonid Mesnik (@lmesnik - Reviewer) Review applies to 8102aed8
• Erik Gahlin (@egahlin - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23900

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

@shipilev 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:

8351142: Add JFR monitor deflation and statistics events

Reviewed-by: egahlin, dholmes, lmesnik

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 6 new commits pushed to the master branch:

• a21fa46: 8351309: test/hotspot/jtreg/runtime/posixSig/TestPosixSig.java fails on static-jdk
• 3b189e0: 8351345: [IR Framework] Improve reported disabled IR verification messages
• 95b66d5: 8351700: Remove code conditional on BarrierSetNMethod being null
• 84f87dd: 8351665: Remove unused UseNUMA in os_aix.cpp
• 4be502e: 8350642: Interpreter: Upgrade CountBytecodes to 64 bit on 64 bit platforms
• 1fe4526: 8350194: Last 2 parameters of ReturnNode::ReturnNode are swapped in the declaration

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this 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.

[openjdk]

@shipilev The following label will be automatically applied to this pull request:

• hotspot

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

[mlbridge]

Webrevs

• 04: Full - Incremental (0caf4b3a)
• 03: Full (edd9beaf)
• 02: Full (02293a80)
• 01: Full - Incremental (8102aed8)
• 00: Full (0cbb9f53)

[dholmes-ora]

Seems like a good idea. I wonder why the deflation event was not added earlier?

[lmesnik]

Test changes looks good.

[egahlin]

Looks good.

[dholmes-ora]

I'm not completely clear on how the events operate, but the general runtime changes look okay to me.

[shipilev]

Fixed the merge conflicts, and touched up event descriptions a bit. @egahlin, see if those still make sense to you?

[egahlin]

Looks good overall, but I'm not sure we should add maxCount.

I'm hesitant because the peak value can easily be calculated, which we already do for other events (CPULoad, NetworkUtilization, NativeMemoryUsage etc) in "jfr view".

[shipilev]

I'm hesitant because the peak value can easily be calculated, which we already do for other events (CPULoad, NetworkUtilization, NativeMemoryUsage etc) in "jfr view".

True, I thought about that, and still ended up adding max, because it is tracked internally by locking subsystem, and thus does not run into sampling bias. I.e. JFR sampling thread may not see the spike in monitor counts if bulk inflation/deflation happens between two samples.

Looking around other stats in the metadata.xml, maybe a better name for it is peakCount?

[shipilev]

Looking around other stats in the metadata.xml, maybe a better name for it is peakCount?

Yeah, I think it should be peakCount. Changed in new version.

[egahlin]

Since this event is only emitted once per chunk, it might be necessary to have a peak value to avoid sampling bias, but I think we should only add such metrics where there is a strong justification to do so, and where a calculated value would have failed to solve the underlying problem. I don't want to end up in a situation where we add peak, average, minimum, etc. for every event value. It adds noise and may confuse users when there are two maximum values in the GUI, one during the recording and one from when the JVM started.

I agree, "peakCount" is a better name than "maxCount".

[shipilev]

I think we should only add such metrics where there is a strong justification to do so, and where a calculated value would have failed to solve the underlying problem

Right, makes sense. Let's recap.

The underlying reason for providing peak statistics is to track the population of object monitors without computing it from individual inflate/deflate events. Since it is periodic, it run into sampling bias. The sampling bias works not only for peaks, but also for dips, so I would guess a fuller solution would be indeed to add min/max counters, if we wanted to avoid the bias. But, I think this goes too far.

We want to replace one of the ObjectMonitor counters that counts the instantaneous monitor population. It does not really report peak/max. I can see that OM code might want to stop tracking max, but JFR event would force its hand, if we start reporting it.

So, thinking that adding a new field into JFR event is easier than yanking the unnecessary/bad one, I think we should be conservative and just report the instantaneous monitor population. If we find it is insufficient, then we can talk about extending the event. Sounds good?

Dropped peakCount from this PR in new version. Take a look again, please?

[shipilev]

Thank you for reviews, appreciated! I'll integrate shortly.

[shipilev]

/integrate

[openjdk]

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

• db531bf: 8351881: Tidy complains about missing "alt" attribute
• 02c850f: 8350982: -server|-client causes fatal exception on static JDK
• 2371696: 8303770: Remove Baltimore root certificate expiring in May 2025
• f16a742: 8351626: Update remaining icons to SVG format
• a21fa46: 8351309: test/hotspot/jtreg/runtime/posixSig/TestPosixSig.java fails on static-jdk
• 3b189e0: 8351345: [IR Framework] Improve reported disabled IR verification messages
• 95b66d5: 8351700: Remove code conditional on BarrierSetNMethod being null
• 84f87dd: 8351665: Remove unused UseNUMA in os_aix.cpp
• 4be502e: 8350642: Interpreter: Upgrade CountBytecodes to 64 bit on 64 bit platforms
• 1fe4526: 8350194: Last 2 parameters of ReturnNode::ReturnNode are swapped in the declaration

Your commit was automatically rebased without conflicts.

[openjdk]

@shipilev Pushed as commit 895f64a.

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

[dholmes-ora]

@shipilev we missed the fact the obj may be null when deflating. A bug is being filed.

[shipilev]

@shipilev we missed the fact the obj may be null when deflating. A bug is being filed.

On it: #24121