8373403: [TESTBUG] TestG1ClassUnloadingHWM.java could fail with large G1HeapRegionSize and small InitialHeapSize

[caoman]

Hi all,

Could anyone review this test-only improvement to make TestG1ClassUnloadingHWM.java more robust in various configurations?
See https://bugs.openjdk.org/browse/JDK-8373403 for more details.

-Man

---

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-8373403: [TESTBUG] TestG1ClassUnloadingHWM.java could fail with large G1HeapRegionSize and small InitialHeapSize (Bug - P4)

Reviewers

• Thomas Schatzl (@tschatzl - Reviewer)
• Ivan Walulya (@walulyai - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28738

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

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

8373403: [TESTBUG] TestG1ClassUnloadingHWM.java could fail with large G1HeapRegionSize and small InitialHeapSize

Reviewed-by: tschatzl, iwalulya

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

• 650de99: 8367158: C2: create better fill and copy benchmarks, taking alignment into account
• 325cdb7: 8373517: Revert the macos Tahoe specific change done in JDK-8359830
• c46bed7: 8371502: serviceability/jvmti/vthread/ThreadListStackTracesTest/ThreadListStackTracesTest.java failing
• ... and 20 more: https://git.openjdk.org/jdk/compare/b60ac710bebf195972436da324983e61b51484ef...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]

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

• hotspot-gc

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

• 02: Full - Incremental (70e88efa)
• 01: Full - Incremental (6c6fbfe9)
• 00: Full (05605e16)

[walulyai]

The issue appears to be that we compute an IHOP target threshold that is smaller than the G1HeapRegionSize. So after the "Pause Full (Metadata GC Threshold)", a concurrent cycle is requested.

Would it be better to fix that as below?

--- a/src/hotspot/share/gc/g1/g1IHOPControl.cpp
+++ b/src/hotspot/share/gc/g1/g1IHOPControl.cpp
@@ -115,7 +115,7 @@ size_t G1IHOPControl::get_conc_mark_start_threshold() {
   guarantee(_target_occupancy > 0, "Target occupancy must be initialized");
 
   if (!_is_adaptive || !have_enough_data_for_prediction()) {
-    return (size_t)(_initial_ihop_percent * _target_occupancy / 100.0);
+    return MAX2((size_t)(_initial_ihop_percent * _target_occupancy / 100.0), G1HeapRegionSize);
   }

[tschatzl]

I think this is a test bug: it should check whether the Metadata GCs is a full gc and not a young gc starting a concurrent mark. Right now the test just checks whether there is any full gc and not a concurrent start gc at all. The regexp should add a Metadata GC Threshold part there.

@walulyai' s comment hints at another issue: if there is only one region, there is no gain in collecting that region (unless, very unlikely, that single region is completely empty). Either the concurrent mark will select it as an evacuation candidate and copy its contents to another (single) region, or do nothing.

Either way nothing is gained. However this is a different issue imo.

[caoman]

I think this is a test bug: it should check whether the Metadata GCs is a full gc and not a young gc starting a concurrent mark. Right now the test just checks whether there is any full gc and not a concurrent start gc at all. The regexp should add a Metadata GC Threshold part there.

Updated test to check Metadata GC Threshold explicitly, and removed the check for concurrent mark in testWithoutG1ClassUnloading().

Also filed https://bugs.openjdk.org/browse/JDK-8373461 for avoiding the unnecessary concurrent mark.

[tschatzl]

Updated test to check Metadata GC Threshold explicitly, and removed the check for concurrent mark in testWithoutG1ClassUnloading().

One can check whether the concurrent start GC has the "Metadata GC Threshold" cause as well in the regexes. Young GC both have the cause (e.g. that "Metadata GC Threshold") as well as a type ("Concurrent Start").

Also filed https://bugs.openjdk.org/browse/JDK-8373461 for avoiding the unnecessary concurrent mark.

Thank you.

[caoman]

One can check whether the concurrent start GC has the "Metadata GC Threshold" cause as well in the regexes. Young GC both have the cause (e.g. that "Metadata GC Threshold") as well as a type ("Concurrent Start").

Thanks, done. Now we can keep the check in testWithoutG1ClassUnloading() for non-existence of Pause Young (Concurrent Start) (Metadata GC Threshold).

[walulyai]

Looks good!