8352414: JFR: JavaMonitorDeflateEvent crashes when deflated monitor object is dead

[shipilev]

Little regression crept in with JDK-8351142: on the deflation path, object associated with monitor can be already dead.

A new stress test fails within seconds without a fix. It also covers other monitor events, so we have extra coverage there as well.

Additional testing:

• Linux x86_64 server fastdebug, new stress test now passes
• 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-8352414: JFR: JavaMonitorDeflateEvent crashes when deflated monitor object is dead (Enhancement - P4)

Reviewers

• David Holmes (@dholmes-ora - Reviewer)
• Markus Grönlund (@mgronlun - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24121

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24121.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:

8352414: JFR: JavaMonitorDeflateEvent crashes when deflated monitor object is dead

Reviewed-by: dholmes, mgronlun

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

• b84b292: 8352615: [Test] RISC-V: TestVectorizationMultiInvar.java fails on riscv64 without rvv support
• a54445f: 8350609: Cleanup unknown unwind opcode (0xB) for windows
• c87e1be: 8349582: APX NDD code generation for OpenJDK
• 7d1fe0e: 8339543: [vectorapi] laneHelper and withLaneHelper should be ForceInline
• ddc4318: 8352628: Refine Grapheme test
• d8c2f59: 8352064: AIX: now also able to build static-jdk image with a statically linked launcher
• 5709f79: 8352648: JFR: 'jfr query' should not be available in product builds
• 02a4ce2: 8352147: G1: TestEagerReclaimHumongousRegionsClearMarkBits test takes very long
• de58009: 8351468: C2: array fill optimization assigns wrong type to intrinsic call
• a875733: 8352486: [ubsan] compilationMemoryStatistic.cpp:659:21: runtime error: index 64 out of bounds for type const struct unnamed struct
• ... and 57 more: https://git.openjdk.org/jdk/compare/20f1bca0770b6b4d935b068e7f6a742cef4f5449...master

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

• 01: Full - Incremental (862a0176)
• 00: Full (2b1d1b14)

[dholmes-ora]

Looks fine to me.

Thanks

[shipilev]

@egahlin, @mgronlun -- you folks are good with this version?

[mgronlun]

A event about a deflated monitor without any information which monitor is deflated seems just like noise to me.

What problem are we trying to solve here again? Are we interested in how long a monitor was inflated? Pairing up inflates with deflates?

Implementation wise, we should use monotonic, internally assigned ids as keys for monitor identity, instead of relying on the oops (but that is outside this PR).

[shipilev]

What problem are we trying to solve here again? Are we interested in how long a monitor was inflated? Pairing up inflates with deflates?

Yes, we want to pair inflates with deflates. When deflate happens on a dead object, we don't have a clear signal which object was deflated. But the event counts (and their timestamps) would still be matchable, and inflations - deflations would be roughly equal to monitor count from the stats event. I expect the events with dead objects would be fairly rare anyway.

[mgronlun]

How do you match up the timestamps? Because without the oop, the connection between the two events is lost.

Are we measuring how long time it takes to deflate a monitor? How useful is that information?

Deflations can also happen asynchronously. If so, the thread information cannot be used to pair them up either.

[shipilev]

How do you match up the timestamps? Because without the oop, the connection between the two events is lost.

True, but even the timestamp+event is a useful bit of info. If there are X monitors recorded by stats event at 13:13, then 100+ inflations happened at 13:14, and then 50+ deflations happened at 13:15, then I can plausibly guess the momentary monitor population is X+50, even if deflation events gives me no precise mapping for dead objects was deflated.

Are we measuring how long time it takes to deflate a monitor? How useful is that information?

It is useful to know when deflations happened, as this shows if deflater thread is actually performing well. We have seen "memory leaks" due to deflation policy bugs when monitor deflater was essentially stuck / outpaced by inflations. Pretty sure there are still lingering issues when monitor population spikes in a very short time frame, so it is useful to know inflations/deflations at individual events scale.

[mgronlun]

Ok. We could devise a more stable scheme to retain the pairing in the future.

[shipilev]

Thanks! I gave it another spin through testing, and it still looks green.

/integrate

[openjdk]

Going to push as commit 17dc30c.
Since your change was applied there have been 76 commits pushed to the master branch:

• 6bc4803: 8351277: Remove pipewire from AIX build
• c94bc74: 8352595: Regression of JDK-8314999 in IR matching
• 48fac66: 8347406: [REDO] C1/C2 don't handle allocation failure properly during initialization (RuntimeStub::new_runtime_stub fatal crash)
• 99c8a6e: 8350463: AArch64: Add vector rearrange support for small lane count vectors
• b2da0d3: 8352289: [macos] Review skipped tests in tools/jpackage/macosx/SigningPackage*
• ba658a7: 8349522: AArch64: Add backend implementation for new unsigned and saturating vector operations
• 5625b43: 8350429: runtime/NMT/CheckForProperDetailStackTrace.java should only run for debug JVM
• 2c60fc5: 8352176: Automate setting up environment for mac signing tests
• 6e6a39d: 8347321: [ubsan] CGGlyphImages.m:553:30: runtime error: nan is outside the range of representable values of type 'unsigned long'
• b84b292: 8352615: [Test] RISC-V: TestVectorizationMultiInvar.java fails on riscv64 without rvv support
• ... and 66 more: https://git.openjdk.org/jdk/compare/20f1bca0770b6b4d935b068e7f6a742cef4f5449...master

Your commit was automatically rebased without conflicts.

[openjdk]

@shipilev Pushed as commit 17dc30c.

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