8316632: Shenandoah: Raise OOME when gc threshold is exceeded #15852

earthling-amzn · 2023-09-20T22:41:52Z

Shenandoah will run back-to-back full GCs and almost grind mutator progress to a halt before eventually exhausting memory. This change will have Shenandoah raise a gc threshold exceeded exception if the collector fails to make progress after ShenandoahNoProgressThreshold full GC cycles (default is 3).

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-8316632: Shenandoah: Raise OOME when gc threshold is exceeded (Enhancement - P4)

Reviewers

Kelvin Nilsen (@kdnilsen - no project role) ⚠️ Review applies to f68a942b
Y. Srinivas Ramakrishna (@ysramakrishna - Reviewer) ⚠️ Review applies to f68a942b
Aleksey Shipilev (@shipilev - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15852

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-09-20T22:43:29Z

👋 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 · 2023-09-20T22:45:50Z

@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.

mlbridge · 2023-09-20T22:49:49Z

Webrevs

kdnilsen

Looks good to me.

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

shipilev · 2023-09-21T08:28:29Z

Please also run tier1 tier2 tier3 with Shenandoah? In hotspot:tier1, I am seeing the failure here:

TEST: gc/InfiniteList.java

STDERR:
Exception in thread "main" java.lang.OutOfMemoryError: GC overhead limit exceeded
        at java.base/java.util.jar.Manifest$FastInputStream.<init>(Manifest.java:421)
        at java.base/java.util.jar.Manifest$FastInputStream.<init>(Manifest.java:416)
        at java.base/java.util.jar.Manifest.read(Manifest.java:287)
        at java.base/java.util.jar.Manifest.<init>(Manifest.java:101)
        at java.base/java.util.jar.Manifest.<init>(Manifest.java:88)
        at java.base/java.util.jar.JarFile.getManifestFromReference(JarFile.java:431)
        at java.base/java.util.jar.JarFile.getManifest(JarFile.java:405)
        at java.base/jdk.internal.loader.URLClassPath$JarLoader$2.getManifest(URLClassPath.java:853)
        at java.base/jdk.internal.loader.BuiltinClassLoader.defineClass(BuiltinClassLoader.java:848)
        at java.base/jdk.internal.loader.BuiltinClassLoader.findClassOnClassPathOrNull(BuiltinClassLoader.java:760)
        at java.base/jdk.internal.loader.BuiltinClassLoader.loadClassOrNull(BuiltinClassLoader.java:681)
        at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:639)
        at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188)
        at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:525)
        at com.sun.javatest.regtest.agent.MainWrapper.main(MainWrapper.java:94)

earthling-amzn · 2023-09-21T16:10:30Z

I will run additional tests with Shenandoah enabled.

ysramakrishna

I have a few questions; thanks for clearing up my confusion! :-)

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

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

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

ysramakrishna

LGTM, modulo a couple of clarifying documentation comments, as discussed.

openjdk · 2023-09-23T01:48:54Z

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

8316632: Shenandoah: Raise OOME when gc threshold is exceeded

Reviewed-by: kdnilsen, ysr, 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 3 new commits pushed to the master branch:

9123961: 8318096: Introduce AsymmetricKey interface with a getParams method
4a142c3: 8274122: java/io/File/createTempFile/SpecialTempFile.java fails in Windows 11
77fe0fd: 8272215: Add InetAddress methods for parsing IP address literals

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 (@ysramakrishna, @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).

shipilev

Apologies, but I have more comments.

test/hotspot/jtreg/TEST.groups

shipilev · 2023-09-29T16:40:22Z

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

@@ -873,19 +874,33 @@ HeapWord* ShenandoahHeap::allocate_memory(ShenandoahAllocRequest& req) {
      result = allocate_memory_under_lock(req, in_new_region);
    }

+    if (result == nullptr && !req.is_lab_alloc() && get_gc_no_progress_count() > ShenandoahNoProgressThreshold) {


Can this be moved to the block that already does allocation-after-gc retries? That block already exits with nullptr (implies delivering OOME), and it already calls handle_alloc_failure (thus triggering GC). We "only" need it to specialize for is_lab_alloc and ShenandoahNoProgressThreshold?

This PR changes that block anyway...

Yes, I've made this change as you suggest, but I'm not sure it improves readability. Note that when the ShenandoahNoProgressThreshold is exceeded, we do not retry the allocation or wait for another gc cycle to complete.

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

shipilev · 2023-10-16T17:28:40Z

test/jdk/com/sun/jdi/EATests.java

@@ -271,7 +271,7 @@ public static class TargetVMOptions {
        public final boolean EliminateAllocations;
        public final boolean DeoptimizeObjectsALot;
        public final boolean DoEscapeAnalysis;
-        public final boolean ZGCIsSelected;
+        public final boolean ConcurrentGCIsSelected;


Let's do explicit ZGCIsSelected and ShenandoahIsSelected. Unfortunately, "Concurrent GC" is fairly ambiguous.

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

shipilev

All right, this looks fine.

earthling-amzn · 2023-10-26T23:15:13Z

/integrate

openjdk · 2023-10-26T23:15:40Z

@earthling-amzn
Your change (at version 95f3379) is now ready to be sponsored by a Committer.

shipilev · 2023-10-27T08:17:36Z

/sponsor

openjdk · 2023-10-27T08:18:42Z

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

abad040: 8313781: Add regression tests for large page logging and user-facing error messages
9123961: 8318096: Introduce AsymmetricKey interface with a getParams method
4a142c3: 8274122: java/io/File/createTempFile/SpecialTempFile.java fails in Windows 11
77fe0fd: 8272215: Add InetAddress methods for parsing IP address literals

Your commit was automatically rebased without conflicts.

openjdk · 2023-10-27T08:18:51Z

@shipilev @earthling-amzn Pushed as commit 5b5fd36.

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

earthling-amzn added 3 commits September 18, 2023 17:02

Signal gc threshold exceeded when appropriate

3216f82

Do not use atomics in header

2cac109

Fix 32-bit build error

f68a942

openjdk bot added the rfr Pull request is ready for review label Sep 20, 2023

openjdk bot added hotspot-gc hotspot-gc-dev@openjdk.org shenandoah shenandoah-dev@openjdk.org labels Sep 20, 2023

kdnilsen approved these changes Sep 20, 2023

View reviewed changes

shipilev reviewed Sep 21, 2023

View reviewed changes

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

ysramakrishna reviewed Sep 21, 2023

View reviewed changes

earthling-amzn added 2 commits September 21, 2023 17:42

Allocator should not reset bad progress count

26c5c49

Allocator should not reset bad progress count

54b4c4b

ysramakrishna approved these changes Sep 23, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 23, 2023

earthling-amzn added 3 commits September 25, 2023 13:59

Improve comment, increase default for no progress threshold

f864be6

Extend exemption for EATests that rely on timely OOME to Shenandoah

5e956ad

Merge remote-tracking branch 'openjdk/master' into shenandoah-oome-redux

cd4989e

shipilev reviewed Sep 29, 2023

View reviewed changes

earthling-amzn added 2 commits October 2, 2023 14:16

Revert change to TEST.groups

81038a0

Merge check for no-progress into retry allocation block

1971467

earthling-amzn mentioned this pull request Oct 10, 2023

8314935: Shenandoah: Unable to throw OOME on back-to-back Full GCs openjdk/jdk17u-dev#1724

Closed

4 tasks

shipilev reviewed Oct 16, 2023

View reviewed changes

earthling-amzn added 3 commits October 16, 2023 13:56

Merge branch 'openjdk-master' into shenandoah-oome-redux

8db452a

Restore original retry logic, pull gc overhead check back over retry

2735b48

Make explicit field for Shenandoah

868af37

earthling-amzn added 2 commits October 17, 2023 11:27

Update formatting

2a3f9f7

Remove unnecessary nesting and other differences

4154370

shipilev approved these changes Oct 17, 2023

View reviewed changes

Merge remote-tracking branch 'openjdk/master' into shenandoah-oome-redux

95f3379

openjdk bot added the sponsor Pull request is ready to be sponsored label Oct 26, 2023

openjdk bot added the integrated Pull request has been integrated label Oct 27, 2023

openjdk bot closed this Oct 27, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Oct 27, 2023

earthling-amzn mentioned this pull request Dec 5, 2023

8321806: Shenandoah: each mutator must see FullGC or GC overhead limit is exceeded before throwing OOM #16985

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8316632: Shenandoah: Raise OOME when gc threshold is exceeded #15852

8316632: Shenandoah: Raise OOME when gc threshold is exceeded #15852

earthling-amzn commented Sep 20, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 20, 2023

openjdk bot commented Sep 20, 2023

mlbridge bot commented Sep 20, 2023 •

edited

Loading

kdnilsen left a comment

shipilev commented Sep 21, 2023

earthling-amzn commented Sep 21, 2023

ysramakrishna left a comment

ysramakrishna left a comment

openjdk bot commented Sep 23, 2023 •

edited

Loading

shipilev left a comment

shipilev Sep 29, 2023

earthling-amzn Oct 2, 2023

shipilev Oct 16, 2023

shipilev left a comment

earthling-amzn commented Oct 26, 2023

openjdk bot commented Oct 26, 2023

shipilev commented Oct 27, 2023

openjdk bot commented Oct 27, 2023

openjdk bot commented Oct 27, 2023

8316632: Shenandoah: Raise OOME when gc threshold is exceeded #15852

8316632: Shenandoah: Raise OOME when gc threshold is exceeded #15852

Conversation

earthling-amzn commented Sep 20, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Sep 20, 2023

openjdk bot commented Sep 20, 2023

mlbridge bot commented Sep 20, 2023 • edited Loading

Webrevs

kdnilsen left a comment

Choose a reason for hiding this comment

shipilev commented Sep 21, 2023

earthling-amzn commented Sep 21, 2023

ysramakrishna left a comment

Choose a reason for hiding this comment

ysramakrishna left a comment

Choose a reason for hiding this comment

openjdk bot commented Sep 23, 2023 • edited Loading

shipilev left a comment

Choose a reason for hiding this comment

shipilev Sep 29, 2023

Choose a reason for hiding this comment

earthling-amzn Oct 2, 2023

Choose a reason for hiding this comment

shipilev Oct 16, 2023

Choose a reason for hiding this comment

shipilev left a comment

Choose a reason for hiding this comment

earthling-amzn commented Oct 26, 2023

openjdk bot commented Oct 26, 2023

shipilev commented Oct 27, 2023

openjdk bot commented Oct 27, 2023

openjdk bot commented Oct 27, 2023

earthling-amzn commented Sep 20, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 20, 2023 •

edited

Loading

openjdk bot commented Sep 23, 2023 •

edited

Loading