RFR: 8280087: G1: Handle out-of-mark stack situations during reference processing more gracefully [v4]

[Albert Mingkun Yang]

On Thu, 7 Dec 2023 14:52:46 GMT, Ivan Walulya <iwalulya at openjdk.org> wrote:

>> Hi all,
>> 
>> Please review this change to modify how we grow the global marking stack used by Concurrent Marking. This patch allows for growing the marking stack without the need to copy over elements. Consequently, we can grow the marking stack even during the reference processing phase without the need to restart the marking cycle.
>> 
>> This mainly addresses the issue where object marking work created during reference processing may overflow the global marking stack. Currently G1 just bails out with a fatal error  as expanding the marking stack would require a restart which is not valid during the reference processing phase.
>> 
>> We have decided to maintain the restart concurrent marking when the global mark stack overflows during the marking phase of the concurrent cycle as this offers better memory utilisation.
>> 
>> Testing: Tier 1-5.
>
> Ivan Walulya has updated the pull request incrementally with one additional commit since the last revision:
> 
>   add bounds checking, fix bug in array sizing

src/hotspot/share/gc/g1/g1ConcurrentMark.cpp line 216:

> 214:   // We reset without regard for the outcome of the expansion attempt.
> 215:   // Expand is called in preparation for restart.
> 216:   reset();

I believe the caller already invokes `reset`. (It's also surprising that `expand` triggers `reset` implicitly, IMO.)

src/hotspot/share/gc/g1/g1ConcurrentMark.cpp line 238:

> 236:     log_debug(gc)("Cannot expand overflow mark stack beyond the max_capacity" SIZE_FORMAT " chunks.", _max_capacity);
> 237:     return false;
> 238:   }

Can this be an precondition? (This is a private API.)

src/hotspot/share/gc/g1/g1ConcurrentMark.hpp line 144:

> 142:     size_t _capacity;
> 143:     size_t _num_buckets;
> 144:     bool _growable;

I wonder if this can be named `_should_grow`.

src/hotspot/share/gc/g1/g1ConcurrentMark.hpp line 174:

> 172:       }
> 173:       return array_idx - (1ULL << find_highest_bit(array_idx));
> 174:     }

Could you add more doc on this? It's unclear what "array_idx" means for example. Maybe some diagram will help illustrate the data structure.

src/hotspot/share/gc/g1/g1ConcurrentMark.hpp line 190:

> 188:     }
> 189: 
> 190:     void grow_incrementally() {

This is essentially a setter, but it would become more obvious with a more setter-like name.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16979#discussion_r1419297329
PR Review Comment: https://git.openjdk.org/jdk/pull/16979#discussion_r1419294968
PR Review Comment: https://git.openjdk.org/jdk/pull/16979#discussion_r1419298905
PR Review Comment: https://git.openjdk.org/jdk/pull/16979#discussion_r1419303733
PR Review Comment: https://git.openjdk.org/jdk/pull/16979#discussion_r1419300367