RFR: 8308507: G1: GClocker induced GCs can starve threads requiring memory leading to OOME [v4]

[Ivan Walulya]

On Wed, 31 May 2023 22:51:54 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> Ivan Walulya has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Thomas review
>
> src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 419:
> 
>> 417: 
>> 418:   if (should_try_gc) {
>> 419:     bool succeeded = false;
> 
> Why is this var needed?

forced by existing implementation of `G1CollectedHeap::do_collection_pause`, I sure can change that and remove the need for the variable.

> src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 502:
> 
>> 500:     }
>> 501: 
>> 502:     if (attempt_allocation_after_gc(word_size, gc_count_before, should_try_gc, &result, GCCause::_g1_inc_collection_pause)) {
> 
> When this returns `false`, it would loop. Why is it safe/correct to discard `GCLockerRetryAllocationCount` here? (Can't find it in G1 code.)

Because the false return will not be due to GCLocker.

> src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 1049:
> 
>> 1047:   bool *expect_null_alloc_regions = (bool*)alloca(active_numa_nodes * sizeof(bool));
>> 1048:   for (uint i = 0; i < active_numa_nodes; i++) {
>> 1049:     expect_null_alloc_regions[i] = expect_null_mutator_alloc_region;
> 
> AFAICS, this array is just for assert eventually, right? If so, how useful is that assert? Can it be removed?

I wasn't in position to determine that, so I went with keeping it. Removing it would make the code a lot cleaner and also avoid the use of alloca, but again, I need guidance on the removing it part

> src/hotspot/share/gc/g1/g1CollectedHeap.hpp line 224:
> 
>> 222:   Mutex _alloc_request_lock;
>> 223:   DoublyLinkedList<StalledAllocReq> _stalled_allocations;
>> 224:   DoublyLinkedList<StalledAllocReq> _satisfied_allocations;
> 
> Not super obvious to me why two lists are needed since each element has `state` already. Can they be merged into one? Then, one can avoid the list manipulation in the following example.
> 
> 
>     alloc_req->set_state(StalledAllocReq::AllocationState::Success, result);
>     ...
>     // Move the allocation request from stalled to satisfied list.
>     _stalled_allocations.remove(alloc_req);
>     _satisfied_allocations.insert_last(alloc_req);

I thought it would be easier to follow with the two lists. I can merge them, If that is easier.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/14077#discussion_r1212889822
PR Review Comment: https://git.openjdk.org/jdk/pull/14077#discussion_r1212894366
PR Review Comment: https://git.openjdk.org/jdk/pull/14077#discussion_r1212897636
PR Review Comment: https://git.openjdk.org/jdk/pull/14077#discussion_r1212890664