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

[William Kemper]

On Fri, 29 Sep 2023 16:40:22 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

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

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

TIL - thank you.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/15852#discussion_r1343174921
PR Review Comment: https://git.openjdk.org/jdk/pull/15852#discussion_r1343175156