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

[Albert Mingkun Yang]

On Mon, 12 Jun 2023 13:32:52 GMT, Ivan Walulya <iwalulya at openjdk.org> wrote:

>> Please review this change which fixes the thread starvation problem during allocation for G1.
>> 
>> The starvation problem is not limited to GCLocker, however, currently, it manifests as an OOME only when GCLocker is active. In other cases, the starvation only affects the "starved" thread as it may loop indefinitely. 
>> 
>> Starvation with an active GCLocker happens as below:
>> 
>> 1. Thread A tries to allocate memory as normal, and tries to start a GC; the GCLocker is active and so the thread gets stalled waiting for the GC.
>> 2. GCLocker induced GC executes and frees some memory.
>> 3. Thread A does not get any of that memory, but other threads also waiting for memory.
>> 4. Goto 1 until the gclocker retry count has been reached.
>> 
>> In this change, we take the general approach to solving starvation problems with announcement tables (request queues). On slow allocation, a thread that wishes to complete an Allocation GC and then attempt an allocation, announces its allocation request before proceeding to participate in a race to execute a GC safepoint. Whichever thread succeeds in executing the Allocation GC safepoint will be tasked with completing all allocation requests that were announced before the safepoint. This guarantees that all announced allocation requests are either satisfied during the safepoint, or failed in case there is not enough memory to complete all requests. This effectively deals with the starvation issue and reduces the number of allocation GCs triggered.
>> 
>> Note: The change also adopts ZList from ZGC and makes it available under utilities as DoublyLinkedList with slight modifications. 
>> 
>> Testing: Tier 1-7
>
> Ivan Walulya has updated the pull request incrementally with one additional commit since the last revision:
> 
>   clean up

src/hotspot/share/gc/g1/g1CollectedHeap.hpp line 200:

> 198:       _g1h(g1h)
> 199:     {
> 200:       if (_g1h != nullptr) {

This check is always true, isn't it?

src/hotspot/share/utilities/doublyLinkedList.hpp line 63:

> 61: // or any position within the list.
> 62: //
> 63: // \tparam T The type of elements stored in the linked list. It must be default-constructible and derived from DoublyLinkedListNode.

"default-constructible" is outdated.

src/hotspot/share/utilities/doublyLinkedList.hpp line 99:

> 97:   void insert_last(T* elem);
> 98:   void insert_before(T* before, T* elem);
> 99:   void insert_after(T* after, T* elem);

Doesn't seem they need to be `T*`; maybe change the arg type to sth like in `insert`? On a higher level, everything going into the collection can take the `Node` type, while everything coming out of the collection can take the elem type, using template.

src/hotspot/share/utilities/doublyLinkedList.inline.hpp line 228:

> 226: template <typename T>
> 227: inline typename DoublyLinkedList<T>::Iterator DoublyLinkedList<T>::end() {
> 228:  return Iterator(this, cast_to_outer(&_head));

`_head` has the base type; it's illegal to cast it to elem type.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/14077#discussion_r1227777988
PR Review Comment: https://git.openjdk.org/jdk/pull/14077#discussion_r1226703924
PR Review Comment: https://git.openjdk.org/jdk/pull/14077#discussion_r1226703048
PR Review Comment: https://git.openjdk.org/jdk/pull/14077#discussion_r1226699005