RFR: 8316632: Shenandoah: Raise OOME when gc threshold is exceeded [v3]

[Aleksey Shipilev]

On Wed, 27 Sep 2023 17:20:06 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> 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).
>
> William Kemper has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains eight additional commits since the last revision:
> 
>  - Merge remote-tracking branch 'openjdk/master' into shenandoah-oome-redux
>  - Extend exemption for EATests that rely on timely OOME to Shenandoah
>  - Improve comment, increase default for no progress threshold
>  - Allocator should not reset bad progress count
>  - Allocator should not reset bad progress count
>  - Fix 32-bit build error
>  - Do not use atomics in header
>  - Signal gc threshold exceeded when appropriate

Apologies, but I have more comments.

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 877:

> 875:     }
> 876: 
> 877:     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...

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 902:

> 900:       ResourceMark rm;
> 901:       log_debug(gc, alloc)("Thread: %s, Result: " PTR_FORMAT ", Shared: %s, Size: " SIZE_FORMAT ", Original: " SIZE_FORMAT ", Latest: " SIZE_FORMAT,
> 902:                            Thread::current()->name(), p2i(result), BOOL_TO_STR(!req.is_lab_alloc()), req.size(), original_count, get_gc_no_progress_count());

There is `type_string()` that can be used instead of `is_lab_alloc()` here.

test/hotspot/jtreg/TEST.groups line 612:

> 610:   gtest/NMTGtests.java
> 611: 
> 612: hotspot_oome = \

Adding new test groups require additional attention. I think this should be removed.

Note that during debugging/testing, you can run the same by:


$ make test TEST="runtime/reflect/ReflectOutOfMemoryError.java gc/InfiniteList.java runtime/ClassInitErrors/TestOutOfMemoryDuringInit.java"

-------------

PR Review: https://git.openjdk.org/jdk/pull/15852#pullrequestreview-1651130171
PR Review Comment: https://git.openjdk.org/jdk/pull/15852#discussion_r1341588940
PR Review Comment: https://git.openjdk.org/jdk/pull/15852#discussion_r1341590508
PR Review Comment: https://git.openjdk.org/jdk/pull/15852#discussion_r1341577573