8306929: Avoid CleanClassLoaderDataMetaspaces safepoints when previous versions are shared

[olivergillespie]

Hi all,

This pull request contains a backport of commit 408cec51 from the openjdk/jdk repository.

The commit being backported was authored by Stefan Johansson on 4 May 2023 and was reviewed by Coleen Phillimore and Serguei Spitsyn.

It's clean except for the surrounding context changing between NULL and nullptr which was trivial to resolve. It comes with its own test, but I also confirmed that it fixes the reproducer shared on the issue.

Edit: This now also backports 8307448/openjdk/jdk@29233e0 to fix the flaky test. 8307448 applied cleanly.

Testing - looks good, just 3 unrelated tests failing on my machine.

 JTREG_KEYWORDS="\!headful & \!external-dep & \!printer" make test TEST=all

   TEST                                              TOTAL  PASS  FAIL ERROR
   jtreg:test/hotspot/jtreg:all                       6131  6131     0     0
>> jtreg:test/jdk:all                                 9140  9137     3     0 <<
   jtreg:test/langtools:all                           4251  4251     0     0
   jtreg:test/jaxp:all                                 467   467     0     0
   jtreg:test/lib-test:all                              30    30     0     0
   
build/AbsPathsInImage.java                                                                                         Failed. Execution failed: `main' threw exception: java.lang.Exception: Test failed
java/nio/channels/FileChannel/directio/DirectIOTest.java                                                           Failed. Execution failed: `main' threw exception: java.lang.RuntimeException: DirectIO is not working properly with read. File still exists in cache!
jdk/internal/platform/docker/TestDockerMemoryMetrics.java                                                          Failed. Execution failed: `main' threw exception: java.lang.RuntimeException: Expected to get exit value of [0]

Thanks!

---

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
• JDK-8307448 needs maintainer approval
• JDK-8306929 needs maintainer approval

Issues

• JDK-8306929: Avoid CleanClassLoaderDataMetaspaces safepoints when previous versions are shared (Enhancement - P3 - Approved)
• JDK-8307448: Test RedefineSharedClassJFR fail due to wrong assumption (Bug - P4 - Approved)

Reviewers

• Aleksey Shipilev (@shipilev - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2776

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

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

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

8306929: Avoid CleanClassLoaderDataMetaspaces safepoints when previous versions are shared
8307448: Test RedefineSharedClassJFR fail due to wrong assumption

Reviewed-by: shade

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

• 37726c5: 8336301: test/jdk/java/nio/channels/AsyncCloseAndInterrupt.java leaves around a FIFO file upon test completion
• 66fa756: 8317112: Add screenshot for Frame/DefaultSizeTest.java

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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@shipilev) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[openjdk]

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

[mlbridge]

Webrevs

• 01: Full - Incremental (9409b2f3)
• 00: Full (350f7696)

[shipilev]

This looks fine. I don't see any follow-up bugs for this change, but please run all tests to see if there is any trouble.

[openjdk]

⚠️ @olivergillespie This change is now ready for you to apply for maintainer approval. This can be done directly in each associated issue or by using the /approval command.

[olivergillespie]

Hmm, the new test is failing on Windows.

[1.056s][trace][redefine,class,iklass,add] adding previous version ref for java.lang.Error, EMCP_cnt=0
[1.056s][trace][redefine,class,iklass,add] scratch class not added; no methods are running
[1.271s][info ][jfr,startup              ] Started recording 1. No limit specified, using maxsize=250MB as default.
[1.271s][info ][jfr,startup              ] 
[1.271s][info ][jfr,startup              ] Use jcmd 2540 JFR.dump name=1 filename=FILEPATH to copy recording data to file.
[1.371s][trace][redefine,class,iklass,purge] Class unloading: should_clean_previous_versions = false

is happening for -Xshare:off instead of the expected

scratch class added; one of its methods is on_stack.
Class unloading: should_clean_previous_versions = true

Will try to investigate. @kstefanj if you happen to know why this might happen please let me know.

Edit: this was already fixed in 8307448, will backport that at the same time.

[olivergillespie]

/issue add JDK-8307448

[openjdk]

@olivergillespie
Adding additional issue to issue list: 8307448: Test RedefineSharedClassJFR fail due to wrong assumption.

[olivergillespie]

/approval 8306929 request Performance enhancement - avoids repeated unnecessary safepoints in reasonably common cases like when using JFR, seen in real applications. Originally landed in JDK21 so has had lots of bake time. Clean except for nullptr/NULL distinction, trivial to resolve. Simple change but in common path so some risk. Confirmed fix against included test and also reproducer from issue.

[olivergillespie]

/approval 8307448 request Test fix to enable backport of 8306929. Applies cleanly, low risk as simple test code only.

[openjdk]

@olivergillespie
8306929: The approval request has been created successfully.

[openjdk]

@olivergillespie
8307448: The approval request has been created successfully.

[olivergillespie]

/integrate

[openjdk]

@olivergillespie
Your change (at version 9409b2f) is now ready to be sponsored by a Committer.

[shipilev]

/sponsor

[openjdk]

Going to push as commit 1f14f86.
Since your change was applied there have been 2 commits pushed to the master branch:

• 37726c5: 8336301: test/jdk/java/nio/channels/AsyncCloseAndInterrupt.java leaves around a FIFO file upon test completion
• 66fa756: 8317112: Add screenshot for Frame/DefaultSizeTest.java

Your commit was automatically rebased without conflicts.

[openjdk]

@shipilev @olivergillespie Pushed as commit 1f14f86.

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