8342444: Shenandoah: Uncommit regions from a separate, STS aware thread #22019

earthling-amzn · 2024-11-11T17:31:58Z

Currently, Shenandoah uncommits regions from its control thread. The control thread is responsible for starting GC cycles in a timely fashion. Uncommitting memory from this thread may introduce unwanted delays in the control thread's response to GC pressure.

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-8342444: Shenandoah: Uncommit regions from a separate, STS aware thread (Enhancement - P4)

Reviewers

Aleksey Shipilev (@shipilev - Reviewer)
Kelvin Nilsen (@kdnilsen - Author) Review applies to 997360ac
Y. Srinivas Ramakrishna (@ysramakrishna - Reviewer) Review applies to 997360ac

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22019

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-11-11T17:32:39Z

👋 Welcome back wkemper! 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-11-11T17:33:38Z

@earthling-amzn 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:

8342444: Shenandoah: Uncommit regions from a separate, STS aware thread

Reviewed-by: shade, kdnilsen, ysr

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

5cc150c: 8342979: Start of release updates for JDK 25
85fedbf: 8344607: Link Time Optimization - basic support for clang
5a0899f: 8345302: Building microbenchmarks require larger Java heap
1ece4f9: 8345514: Should use internal class name when calling ClassLoader.getResourceAsByteArray
ef8da28: 8345591: [aarch64] macroAssembler_aarch64.cpp compile fails ceil_log2 not declared
7513b13: 8328944: NMT reports "unknown" memory
691e692: 8345565: Remove remaining SecurityManager motivated APIs from sun.reflect.util
97b8a09: 8345339: JFR: Missing javadoc for RecordingStream::onMetadata
456c71d: 8343699: [aarch64] Bug in MacroAssembler::klass_decode_mode()
308357c: 8345578: New test in JDK-8343622 fails with a promoted build
... and 27 more: https://git.openjdk.org/jdk/compare/1a73c76d83d34d10519c9d10fb0e51d098907ab0...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-11-11T17:34:21Z

@earthling-amzn The following labels will be automatically applied to this pull request:

hotspot-gc
shenandoah

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

kdnilsen · 2024-11-11T18:19:33Z

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp

-      if (heap->is_bitmap_slice_committed(region)) {
-        ctx->clear_bitmap(region);
+      {
+        ShenandoahHeapLocker locker(heap->lock());


Was it a bug that previous version of this code did not acquire the heap lock?

Is the lock required for the entirety of time that we are clearing the bitmap? Or is it just required to get a trustworthy check on is_bitmap_slice_committed()?

After reading more of this PR, I believe we need the heap lock to get a reliable signal of bitmap_slice_committed(). But I believe we do not need the heap lock for ctx->clear_bitmap(region) so would prefer to move that outside the lock, unless I am misunderstanding.

Hmm, I'm not sure we can do that. Prior to this change, the control thread performed both clearing the bitmap and uncommitting the region's bitmap, so they could never happen concurrently. With this change, a separate thread could perform the uncommit. Consider:

Control thread takes heap lock, observes that bitmap slice for region A is committed

Control thread releases heap lock, begins clearing bitmap (writing zeros to bitmap slice)

Uncommit thread takes heap lock, believes it must uncommit region A

Uncommit thread uncommits bitmap slice for region A

Segfault in Control Thread

I do believe if we had a per region lock, it would be useful here. Holding a lock over the entire heap for this feels like overkill. Or, we could schedule the uncommit so that it does not occur during a GC cycle.

So, wait a sec. This code is in ShenandoahResetBitmapTask, so it can run in parallel. Putting a lock here inhibits parallelism. I understand the failure mode, but I think we should really be optimizing for the case when ShenandoahUncommit is not enabled (e.g. -Xmx == -Xms).

Sounds like there is a hassle in allowing concurrent uncommit to overlap with the GC cycle. In addition to this particular problem, we might be stealing cycles from the GC threads and take additional TTSP lag to park the uncommitter for the in-cycle GC pauses. I have no clear solution for this yet, but I think we need to explore if we can suspend the uncommit before going into GC cycle...

We could have the control and uncommit threads coordinate their efforts. In the worst case, it could mean delaying concurrent reset while the control thread waits for the uncommit thread to yield.

We could also try a more targeted lock only for the region's bitmap slice, but it doesn't seem right that one thread would be trying to clear a bitmap, while the other is trying to uncommit it. A lock could preserve technical correctness, but contention here would just mean that one thread would have wasted its time (either clearing a bitmap that is then uncommitted, or attempting to clear a bitmap that was first uncommitted (in this case, we would need the control thread to detect this and skip the region)).

Can we open a ticket to consider future improved concurrency by moving clear_bitmap(region) outside the global heap lock?

earthling-amzn · 2024-11-12T17:28:19Z

I modified the testing pipelines to set -Xms4g -Xmx10g -XX:+ShenandoahUncommit. All performance and stress tests completed successfully on x86 and aarch64. Marking this as ready for review.

mlbridge · 2024-11-12T17:31:59Z

Webrevs

shipilev

Cursory review:

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp

shipilev · 2024-11-12T17:51:39Z

src/hotspot/share/gc/shenandoah/shenandoahUncommitThread.cpp

+  if (count > 0) {
+    _heap->notify_heap_changed();
+    double elapsed = os::elapsedTime() - start;
+    log_info(gc)("Uncommitted " SIZE_FORMAT " regions, in %.3fs", count, elapsed);


If we can, can we match the current log format? E.g. print Concurrent uncommit, with appropriately formatted timestamp? I think we also want log_info(gc,start) at the beginning of the method. I think ShenandoahConcurrentPhase helper did all that, can we still use it?

We can restore the log messages, but I don't think ShenandoahConcurrentPhase and friends will like being used outside of a cycle. I'll look into it.

Yeah, at least restore the log format and add gc+start log as well.

This one is still not addressed, unfortunately ^

Yes, I spent some time trying to resurrect ShenandoahConcurrentPhase for uncommit here, but it really doesn't want to be used outside of a gc cycle. Also, previously it was logging heap usage, which isn't quite what we want here (this may actually increase during this phase, which makes it seem as though nothing is being uncommitted).

I've restored the original logging format, but instead of logging heap usage it is now logging heap committed before and after. Here is an excerpt from specjbb2015 with -Xms5g -Xmx10g:

[2024-11-20T20:02:25.056+0000][97.396s][22293][info][gc,start ] Concurrent uncommit [2024-11-20T20:02:25.072+0000][97.412s][22293][info][gc ] Concurrent uncommit 5424M->5120M(5120M) 15.988ms [2024-11-20T20:05:17.916+0000][270.255s][22293][info][gc,start ] Concurrent uncommit [2024-11-20T20:05:18.169+0000][270.508s][22293][info][gc ] Concurrent uncommit 10240M->5120M(5120M) 253.048ms [2024-11-20T20:06:45.329+0000][357.668s][22293][info][gc,start ] Concurrent uncommit [2024-11-20T20:06:45.596+0000][357.935s][22293][info][gc ] Concurrent uncommit 10240M->5120M(5120M) 267.144ms [2024-11-20T20:06:57.147+0000][369.486s][22293][info][gc,start ] Concurrent uncommit [2024-11-20T20:06:57.148+0000][369.487s][22293][info][gc ] Concurrent uncommit 5456M->5440M(5440M) 1.189ms

If we are emitting a log line that looks like a properly formatted GC log line, but the numbers there mean something else for Concurrent uncommit, we are bound to confuse users and automatic tools. Uncommit should affect capacity, this is how we know how deep we have uncommitted. So, I suggest we emit:

Concurrent uncommit XXXXM->XXXXM (YYYYM) z.zzzms

...where XXXX is the heap used at the end of uncommit (note before and after are the same) and YYYY is capacity. This will not expose users to thinking uncommit grows the heap usage, and would give us instantaneous view on heap usage and capacity.

Hmm, the numbers are preceded by Concurrent uncommit, with that context it's not much of a stretch to think these numbers represent the change in committed memory. The original log message (in which heap usage may increase during uncommit) was not helpful. A message with the same format in which heap usage also appears to not change at all during an uncommit is also perplexing. Are we trying too hard to preserve the original, not useful message? Maybe we just want a new message that plainly says:

Concurrently uncommitted XXXXM in z.zzzms

or

Concurrent uncommit: time z.zzzms, committed before XXXXM, committed after YYYYM, capacity ZZZZM

Yes, I don't want to emit something that looks like a heap usage GC log line, if it is not. Unfortunately, X->Y (Z) T.TTTTms is a common format for X and Y as heap use. I agree posting X == Y would be only marginally better. So, maybe this goes as middle ground:

Concurrent uncommit XXXXM (YYYYM) z.zzzms

...where XXXX is the amount uncommitted, YYYY is the final heap capacity?

Okay, this looks good.

shipilev · 2024-11-12T18:17:29Z

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp

-      if (heap->is_bitmap_slice_committed(region)) {
-        ctx->clear_bitmap(region);
+      {
+        ShenandoahHeapLocker locker(heap->lock());


So, wait a sec. This code is in ShenandoahResetBitmapTask, so it can run in parallel. Putting a lock here inhibits parallelism. I understand the failure mode, but I think we should really be optimizing for the case when ShenandoahUncommit is not enabled (e.g. -Xmx == -Xms).

Sounds like there is a hassle in allowing concurrent uncommit to overlap with the GC cycle. In addition to this particular problem, we might be stealing cycles from the GC threads and take additional TTSP lag to park the uncommitter for the in-cycle GC pauses. I have no clear solution for this yet, but I think we need to explore if we can suspend the uncommit before going into GC cycle...

src/hotspot/share/gc/shenandoah/shenandoahUncommitThread.cpp

src/hotspot/share/gc/shenandoah/shenandoahUncommitThread.hpp

kdnilsen

I'm ok with this, but best to wait for @shipilev approval before integrating.

kdnilsen · 2024-11-16T17:49:12Z

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp

-      if (heap->is_bitmap_slice_committed(region)) {
-        ctx->clear_bitmap(region);
+      {
+        ShenandoahHeapLocker locker(heap->lock());


Can we open a ticket to consider future improved concurrency by moving clear_bitmap(region) outside the global heap lock?

kdnilsen

Have read through the latest version of the code. Thanks.

ysramakrishna

Looks good to me. A few documentation comment requests.

Also please share performance data in this PR or in the ticket, especially from the perf/benchmark that may have precipitated this change.

src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp

src/hotspot/share/gc/shenandoah/shenandoahUncommitThread.hpp

src/hotspot/share/gc/shenandoah/shenandoahUncommitThread.cpp

src/hotspot/share/gc/shenandoah/shenandoahUncommitThread.hpp

shipilev

I like it, thanks!

src/hotspot/share/gc/shenandoah/shenandoahControlThread.cpp

src/hotspot/share/gc/shenandoah/shenandoahUncommitThread.cpp

shipilev

I'll approve again, with the following nits:

src/hotspot/share/gc/shenandoah/shenandoahControlThread.cpp

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp

shipilev · 2024-11-19T20:23:57Z

src/hotspot/share/gc/shenandoah/shenandoahUncommitThread.cpp

+  if (count > 0) {
+    _heap->notify_heap_changed();
+    double elapsed = os::elapsedTime() - start;
+    log_info(gc)("Uncommitted " SIZE_FORMAT " regions, in %.3fs", count, elapsed);


Yeah, at least restore the log format and add gc+start log as well.

shipilev

Almost there, modulo restoring the logging.

shipilev · 2024-11-20T09:26:19Z

src/hotspot/share/gc/shenandoah/shenandoahUncommitThread.cpp

+  if (count > 0) {
+    _heap->notify_heap_changed();
+    double elapsed = os::elapsedTime() - start;
+    log_info(gc)("Uncommitted " SIZE_FORMAT " regions, in %.3fs", count, elapsed);


This one is still not addressed, unfortunately ^

src/hotspot/share/gc/shenandoah/shenandoahControlThread.cpp

shipilev

I think log message is still confusing a bit...

src/hotspot/share/gc/shenandoah/shenandoahUncommitThread.cpp

shipilev

Looks good!

earthling-amzn · 2024-11-26T20:10:38Z

@ysramakrishna - I ran several iterations of specjbb2015 with different variations of polling interval. Results show that 1/10th of ShenandoahUncommitDelay is reasonable, and avoids unintentional commit delays when the polling interval is equal or greater than ShenandoahUncommitDelay.

                      Category |  Count |         Total |      GeoMean |      Average |     Trim 0.1 |       StdDev |      Minimum |      Maximum
openjdk:master   critical_jops |      5 |     50862.000 |    10162.095 |    10172.400 |    10172.400 |      513.605 |     9630.000 |    10882.000                                
30ms polling     critical_jops |      5 |     48035.000 |     9582.113 |     9607.000 |     9607.000 |      778.036 |     8808.000 |    10692.000
30s polling      critical_jops |      5 |     56398.000 |    11272.026 |    11279.600 |    11279.600 |      460.355 |    10627.000 |    11842.000
no polling       critical_jops |      5 |     55917.000 |    11176.046 |    11183.400 |    11183.400 |      460.960 |    10899.000 |    11995.000

earthling-amzn · 2024-12-05T17:51:51Z

/integrate

openjdk · 2024-12-05T17:55:38Z

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

dbf48a5: 8344665: Refactor PartialArrayState allocation for reuse
5cc150c: 8342979: Start of release updates for JDK 25
85fedbf: 8344607: Link Time Optimization - basic support for clang
5a0899f: 8345302: Building microbenchmarks require larger Java heap
1ece4f9: 8345514: Should use internal class name when calling ClassLoader.getResourceAsByteArray
ef8da28: 8345591: [aarch64] macroAssembler_aarch64.cpp compile fails ceil_log2 not declared
7513b13: 8328944: NMT reports "unknown" memory
691e692: 8345565: Remove remaining SecurityManager motivated APIs from sun.reflect.util
97b8a09: 8345339: JFR: Missing javadoc for RecordingStream::onMetadata
456c71d: 8343699: [aarch64] Bug in MacroAssembler::klass_decode_mode()
... and 28 more: https://git.openjdk.org/jdk/compare/1a73c76d83d34d10519c9d10fb0e51d098907ab0...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-12-05T17:56:17Z

@earthling-amzn Pushed as commit bedb68a.

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

earthling-amzn added 8 commits October 21, 2024 14:47

WIP: checkpoint

0fb8b23

WIP: checkpoint before sync up

35147bb

Little bit of cleanup

7676be3

Take heap lock when uncommitting bitmaps, uncommit thread joins STS.

2320759

Fix shutdown protocol

89047a7

Merge remote-tracking branch 'jdk/master' into shen-uncommit-thread

8f681e3

Fix ridiculous typo

c6c0e22

Check for safepoint when stopping (stopping thread is java thread)

e668436

earthling-amzn marked this pull request as draft November 11, 2024 17:32

openjdk bot added hotspot-gc hotspot-gc-dev@openjdk.org shenandoah shenandoah-dev@openjdk.org labels Nov 11, 2024

kdnilsen reviewed Nov 11, 2024

View reviewed changes

earthling-amzn marked this pull request as ready for review November 12, 2024 17:27

openjdk bot added the rfr Pull request is ready for review label Nov 12, 2024

shipilev reviewed Nov 12, 2024

View reviewed changes

earthling-amzn added 3 commits November 12, 2024 10:41

Alphabetize includes in shenandoahHeap.cpp

f06fe78

Style and formatting fixes

7301871

Prevent uncommit thread from running during GC

997360a

kdnilsen approved these changes Nov 16, 2024

View reviewed changes

kdnilsen approved these changes Nov 18, 2024

View reviewed changes

ysramakrishna approved these changes Nov 19, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 19, 2024

shipilev approved these changes Nov 19, 2024

View reviewed changes

src/hotspot/share/gc/shenandoah/shenandoahControlThread.cpp Outdated Show resolved Hide resolved

src/hotspot/share/gc/shenandoah/shenandoahUncommitThread.cpp Outdated Show resolved Hide resolved

earthling-amzn added 2 commits November 19, 2024 11:38

Do not notify uncommit thread when uncommit is forbidden

9a13ef4

Improve comments

05db955

shipilev approved these changes Nov 19, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 19, 2024

Use idiomatic name for CADR class

7007415

openjdk bot removed the ready Pull request is ready to be integrated label Nov 19, 2024

Allow commits initially

a00945c

shipilev approved these changes Nov 20, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 20, 2024

Restore logging format, show change in committed heap, rather than usage

2cb7114

openjdk bot removed the ready Pull request is ready to be integrated label Nov 20, 2024

shipilev suggested changes Nov 21, 2024

View reviewed changes

earthling-amzn added 2 commits November 25, 2024 11:30

Merge remote-tracking branch 'jdk/master' into shen-uncommit-thread

0c1e7e7

Log uncommitted delta and capacity

847a259

shipilev suggested changes Nov 26, 2024

View reviewed changes

src/hotspot/share/gc/shenandoah/shenandoahUncommitThread.cpp Outdated Show resolved Hide resolved

src/hotspot/share/gc/shenandoah/shenandoahUncommitThread.cpp Outdated Show resolved Hide resolved

earthling-amzn added 2 commits November 26, 2024 11:05

Decouple polling interval from uncommit time out

467db20

Use count of regions uncommitted to compute uncommit delta

8d3c392

shipilev approved these changes Nov 26, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 26, 2024

Merge jdk/master

e70d874

openjdk bot removed the ready Pull request is ready to be integrated label Dec 3, 2024

shipilev approved these changes Dec 4, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 4, 2024

Merge remote-tracking branch 'jdk/master' into shen-uncommit-thread

c39be0f

openjdk bot added the integrated Pull request has been integrated label Dec 5, 2024

openjdk bot closed this Dec 5, 2024

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

8342444: Shenandoah: Uncommit regions from a separate, STS aware thread #22019

8342444: Shenandoah: Uncommit regions from a separate, STS aware thread #22019

Conversation

earthling-amzn commented Nov 11, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Nov 11, 2024

openjdk bot commented Nov 11, 2024 • edited Loading

openjdk bot commented Nov 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

earthling-amzn commented Nov 12, 2024

mlbridge bot commented Nov 12, 2024 • edited Loading

Webrevs

shipilev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shipilev Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdnilsen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdnilsen left a comment

Choose a reason for hiding this comment

ysramakrishna left a comment

Choose a reason for hiding this comment

shipilev left a comment

Choose a reason for hiding this comment

shipilev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shipilev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shipilev left a comment

Choose a reason for hiding this comment

shipilev left a comment

Choose a reason for hiding this comment

earthling-amzn commented Nov 26, 2024

earthling-amzn commented Dec 5, 2024

openjdk bot commented Dec 5, 2024

openjdk bot commented Dec 5, 2024

earthling-amzn commented Nov 11, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Nov 11, 2024 •

edited

Loading

mlbridge bot commented Nov 12, 2024 •

edited

Loading

shipilev Nov 25, 2024 •

edited

Loading