8329223: Parallel: Parallel GC resizes heap even if -Xms = -Xmx #18539

zhengyu123 · 2024-03-28T19:21:01Z

Parallel GC resizes old gen even when setting -Xms == -Xmx.

During GC argument setup, it always set MinOldSize = GenAlignment and does not make any adjustment when -Xms == -Xmx, while does the adjustment for young gen here

Progress

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

Issue

JDK-8329223: Parallel: Parallel GC resizes heap even if -Xms = -Xmx (Bug - P3)

Reviewers

Albert Mingkun Yang (@albertnetymk - Reviewer)
Guoxiong Li (@lgxbslgx - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18539

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-03-28T19:21:25Z

👋 Welcome back zgu! 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 · 2024-03-28T19:22:33Z

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

8329223: Parallel: Parallel GC resizes heap even if -Xms = -Xmx

Reviewed-by: ayang, gli

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

9ce21d1: 8327647: Occasional SIGSEGV in markWord::displaced_mark_helper() for SPECjvm2008 sunflow
130f71c: 8326742: Change compiler tests without additional VM flags from @run driver to @run main
f4caac8: 8329138: Convert JFR FileForceEvent to static mirror event
2cc8ecc: 8331346: Update PreviewFeature of STREAM_GATHERERS to JEP-473
33e8122: 8331410: Remove unused MemAllocator::mem_allocate_inside_tlab
22a1c61: 8330817: jdk/internal/vm/Continuation/OSRTest.java times out on libgraal
ef4ec2d: 8331284: Inline methods in softRefPolicy.cpp
cff841f: 8328934: Assert that ABS input and output are legal
0630bb0: 8331264: Reduce java.lang.constant initialization overhead
60b61e5: 8331298: avoid alignment checks in UBSAN enabled build
... and 141 more: https://git.openjdk.org/jdk/compare/6ee8407758c92d32e18642b0758d2d5c71ad09f5...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 · 2024-03-28T19:22:49Z

@zhengyu123 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 · 2024-03-28T23:31:39Z

Webrevs

src/hotspot/share/gc/parallel/psOldGen.cpp

albertnetymk · 2024-04-02T07:51:34Z

Can MinOldSize be set as MinOldSize = MinHeapSize - MinNewSize; on L279, right after MaxOldSize?

zhengyu123 · 2024-04-07T22:48:51Z

Can MinOldSize be set as MinOldSize = MinHeapSize - MinNewSize; on L279, right after MaxOldSize?

Then the value is likely reset here: https://github.com/openjdk/jdk/blob/master/src/hotspot/share/gc/shared/genArguments.cpp#L290

albertnetymk · 2024-04-08T08:13:40Z

I meant setting MinOldSize based the new + old = heap invariant on L279 and removing all other writes to it.

zhengyu123 · 2024-04-15T14:21:54Z

I meant setting MinOldSize based the new + old = heap invariant on L279 and removing all other writes to it.

Sorry for replying late, I was on PTO with spotty network connection.

Applied the changes you suggested, I am getting following errors:
[0.013s][warning][gc,ergo] Inconsistency between generation sizes and heap size, resizing the generations to fit the heap.
Error occurred during initialization of VM
GC triggered before VM initialization completed. Try increasing NewSize, current value 192K.

albertnetymk · 2024-04-15T17:30:56Z

I see; seems that there can be conflicting constraints from min, init and max triplets. Maybe sth like:

  MinOldSize = MIN3(MaxOldSize,
                    InitialHeapSize - initial_young_size,
                    MinHeapSize - MinNewSize);

zhengyu123 · 2024-04-15T20:35:28Z

Make sense, updated.

albertnetymk · 2024-04-16T07:26:36Z

updated.

The diff is the same as before, just fyi.

albertnetymk

I wonder what others think.

/reviewers 2

openjdk · 2024-04-16T11:47:15Z

@albertnetymk
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

albertnetymk · 2024-04-19T14:28:35Z

src/hotspot/share/gc/shared/genArguments.cpp

@@ -306,7 +308,8 @@ void GenArguments::initialize_size_info() {
      initial_old_size = MaxOldSize;
    }

-    MinOldSize = MIN2(initial_old_size, MinHeapSize - MinNewSize);
+    // Make sure MinOldSize <= OldSize
+    MinOldSize = MIN2(MinOldSize, initial_old_size);


Do you have a test case that requires this? I'd expect MIN3 above to be enough.

Without this, gtest fails on MacOSX (https://github.com/zhengyu123/jdk/actions/runs/8743941793/job/23997262102)

I see; why is MinOldSize being adjusted here, instead of initial_old_size? In the else branch of if (!FLAG_IS_CMDLINE(OldSize)) {, we emit warning for user-set inconsistent OldSize, when it's above the max. For symmetry, I'd expect sth like this for the else part:

if (initial_old_size > MaxOldSize) { log_warning(gc, ergo)(...); initial_old_size = MaxOldSize; } else if (initial_old_size < MinOldSize) { log_waring(gc, ergo)(...); initial_old_size = MinOldSize; }

src/hotspot/share/gc/shared/genArguments.cpp

albertnetymk

It's the failing assert in CheckOldMin that is causing the pthread_mutex_destroy error msg. (I can recreate the error msg on my linux after adding a ASSERT_LT(2, 1) there.)

Then, I think the current fix should be safe, and keeps existing tests happy.

The longer term fix should probably be removing OldSize...

zhengyu123 · 2024-04-26T16:04:37Z

Friendly ping! Could I get a second review.

lgxbslgx

Looks good. Two nits/questions.

lgxbslgx · 2024-04-30T15:11:27Z

src/hotspot/share/gc/shared/genArguments.cpp

+    } else if (initial_old_size < MinOldSize) {
+      log_warning(gc, ergo)("Inconsistency between initial old size and minimum old size");
+      MinOldSize = initial_old_size;


The comment shown below seems strange. Can it be removed?

// The generation minimums and the overall heap minimum should // be within one generation alignment.

The warning message shown below can be more consistent with this newly added one.

log_warning(gc, ergo)("Inconsistency between maximum heap size and maximum " "generation sizes: using maximum heap = " SIZE_FORMAT ", -XX:OldSize flag is being ignored", MaxHeapSize);

If you think they are not related to this bug, it is also OK to fix them in another ticket.

@lgxbslgx Thanks for the review, filed JDK-8331432

zhengyu123 · 2024-04-30T16:19:54Z

Thanks, @albertnetymk and @lgxbslgx

zhengyu123 · 2024-04-30T16:20:00Z

/integrate

openjdk · 2024-04-30T16:20:12Z

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

3d11692: 8331252: C2: MergeStores: handle negative shift values
9ce21d1: 8327647: Occasional SIGSEGV in markWord::displaced_mark_helper() for SPECjvm2008 sunflow
130f71c: 8326742: Change compiler tests without additional VM flags from @run driver to @run main
f4caac8: 8329138: Convert JFR FileForceEvent to static mirror event
2cc8ecc: 8331346: Update PreviewFeature of STREAM_GATHERERS to JEP-473
33e8122: 8331410: Remove unused MemAllocator::mem_allocate_inside_tlab
22a1c61: 8330817: jdk/internal/vm/Continuation/OSRTest.java times out on libgraal
ef4ec2d: 8331284: Inline methods in softRefPolicy.cpp
cff841f: 8328934: Assert that ABS input and output are legal
0630bb0: 8331264: Reduce java.lang.constant initialization overhead
... and 142 more: https://git.openjdk.org/jdk/compare/6ee8407758c92d32e18642b0758d2d5c71ad09f5...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-04-30T16:20:18Z

@zhengyu123 Pushed as commit aca1e83.

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

zhengyu123 · 2024-05-13T15:00:49Z

/backport jdk22u

openjdk · 2024-05-13T15:02:36Z

@zhengyu123 the backport was successfully created on the branch backport-zhengyu123-aca1e836 in my personal fork of openjdk/jdk22u. To create a pull request with this backport targeting openjdk/jdk22u:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

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

The commit being backported was authored by Zhengyu Gu on 30 Apr 2024 and was reviewed by Albert Mingkun Yang and Guoxiong Li.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk22u:

$ git fetch https://github.com/openjdk-bots/jdk22u.git backport-zhengyu123-aca1e836:backport-zhengyu123-aca1e836
$ git checkout backport-zhengyu123-aca1e836
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk22u.git backport-zhengyu123-aca1e836

8329223: Parallel: Parallel GC resizes heap even has -Xms = -Xmx

16e0ce3

openjdk bot added the hotspot-gc hotspot-gc-dev@openjdk.org label Mar 28, 2024

zhengyu123 marked this pull request as ready for review March 28, 2024 23:27

openjdk bot added the rfr Pull request is ready for review label Mar 28, 2024

ecki reviewed Mar 28, 2024

View reviewed changes

src/hotspot/share/gc/parallel/psOldGen.cpp Outdated Show resolved Hide resolved

Cleanup

00c9d8b

Merge branch 'master' into JDK-8329223

219c9be

albertnetymk's suggestion

b6992f2

albertnetymk approved these changes Apr 16, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 16, 2024

openjdk bot removed the ready Pull request is ready to be integrated label Apr 16, 2024

zhengyu.gu added 2 commits April 18, 2024 16:00

Merge branch 'master' into JDK-8329223

35d88dc

Make sure MinOldSize <= OldSize when it is set on the command line

2ce4827

albertnetymk reviewed Apr 19, 2024

View reviewed changes

Albert's comment

e869ee8

openjdk bot mentioned this pull request Apr 20, 2024

8272364: Parallel GC adaptive size policy may shrink the heap below MinHeapSize #18877

Closed

3 tasks

Update copyright

3b9fd3a

albertnetymk reviewed Apr 22, 2024

View reviewed changes

src/hotspot/share/gc/shared/genArguments.cpp Show resolved Hide resolved

zhengyu123 changed the title ~~8329223: Parallel: Parallel GC resizes heap even has -Xms = -Xmx~~ 8329223: Parallel: Parallel GC resizes heap even if -Xms = -Xmx Apr 22, 2024

zhengyu.gu added 2 commits April 23, 2024 10:17

Check with github action

6e784f5

Reverse previous change

0c96fbc

albertnetymk approved these changes Apr 25, 2024

View reviewed changes

lgxbslgx suggested changes Apr 30, 2024

View reviewed changes

lgxbslgx approved these changes Apr 30, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 30, 2024

openjdk bot added the integrated Pull request has been integrated label Apr 30, 2024

openjdk bot closed this Apr 30, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8329223: Parallel: Parallel GC resizes heap even if -Xms = -Xmx #18539

8329223: Parallel: Parallel GC resizes heap even if -Xms = -Xmx #18539

zhengyu123 commented Mar 28, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Mar 28, 2024

openjdk bot commented Mar 28, 2024 •

edited

Loading

openjdk bot commented Mar 28, 2024

mlbridge bot commented Mar 28, 2024 •

edited

Loading

albertnetymk commented Apr 2, 2024

zhengyu123 commented Apr 7, 2024

albertnetymk commented Apr 8, 2024

zhengyu123 commented Apr 15, 2024

albertnetymk commented Apr 15, 2024

zhengyu123 commented Apr 15, 2024

albertnetymk commented Apr 16, 2024

albertnetymk left a comment

openjdk bot commented Apr 16, 2024

albertnetymk Apr 19, 2024

zhengyu123 Apr 19, 2024

albertnetymk Apr 19, 2024

albertnetymk left a comment

zhengyu123 commented Apr 26, 2024

lgxbslgx left a comment

lgxbslgx Apr 30, 2024

lgxbslgx Apr 30, 2024

zhengyu123 Apr 30, 2024

zhengyu123 commented Apr 30, 2024

zhengyu123 commented Apr 30, 2024

openjdk bot commented Apr 30, 2024

openjdk bot commented Apr 30, 2024

zhengyu123 commented May 13, 2024

openjdk bot commented May 13, 2024

8329223: Parallel: Parallel GC resizes heap even if -Xms = -Xmx #18539

8329223: Parallel: Parallel GC resizes heap even if -Xms = -Xmx #18539

Conversation

zhengyu123 commented Mar 28, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Mar 28, 2024

openjdk bot commented Mar 28, 2024 • edited Loading

openjdk bot commented Mar 28, 2024

mlbridge bot commented Mar 28, 2024 • edited Loading

Webrevs

albertnetymk commented Apr 2, 2024

zhengyu123 commented Apr 7, 2024

albertnetymk commented Apr 8, 2024

zhengyu123 commented Apr 15, 2024

albertnetymk commented Apr 15, 2024

zhengyu123 commented Apr 15, 2024

albertnetymk commented Apr 16, 2024

albertnetymk left a comment

Choose a reason for hiding this comment

openjdk bot commented Apr 16, 2024

albertnetymk Apr 19, 2024

Choose a reason for hiding this comment

zhengyu123 Apr 19, 2024

Choose a reason for hiding this comment

albertnetymk Apr 19, 2024

Choose a reason for hiding this comment

albertnetymk left a comment

Choose a reason for hiding this comment

zhengyu123 commented Apr 26, 2024

lgxbslgx left a comment

Choose a reason for hiding this comment

lgxbslgx Apr 30, 2024

Choose a reason for hiding this comment

lgxbslgx Apr 30, 2024

Choose a reason for hiding this comment

zhengyu123 Apr 30, 2024

Choose a reason for hiding this comment

zhengyu123 commented Apr 30, 2024

zhengyu123 commented Apr 30, 2024

openjdk bot commented Apr 30, 2024

openjdk bot commented Apr 30, 2024

zhengyu123 commented May 13, 2024

openjdk bot commented May 13, 2024

zhengyu123 commented Mar 28, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Mar 28, 2024 •

edited

Loading

mlbridge bot commented Mar 28, 2024 •

edited

Loading