RFR: 8257774: G1: Trigger collect when free region count drops below threshold to prevent evacuation failures [v6]

[Stefan Johansson]

On Mon, 17 May 2021 20:38:59 GMT, Aditya Mandaleeka <adityam at openjdk.org> wrote:

>> _This PR picks up from [this previous PR](https://github.com/openjdk/jdk/pull/1650) by @charliegracie._
>> 
>> I won't repeat the full description from the prior PR, but the general idea is to add the notion of a "proactive GC" to G1 which gets triggered in the slow allocation path if the number of free regions drops below the amount that would be required to complete a GC if it happened at that moment. The threshold is based on the survival rates from eden and survivor spaces along with the space required for tenured space evacuations.
>> 
>> There are a couple of outstanding issues/questions known:
>> - [Update: This has been resolved] _Interaction with GCLocker_: In the case where we determine that a proactive GC is required and GC locker is active, we don't allow the young gen to expand (instead threads will stall). @tschatzl raised this and suggested that it should be discussed as part of the review.
>> - [Update: This has been resolved] _Disable proactive GC heuristic during initialization_: I ran into an issue in testing where code in the universe initialization codepath was tripping the proactive GC heuristic, leading to a GC being triggered before the VM has finished initialization. I need to find a good way to prevent this from happening. There may already be a mechanism for this, but I couldn't find one so I added a temporary placeholder (`zzinit_complete`) just to unblock testing.
>
> Aditya Mandaleeka has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Minor updates/feedback.

Took another look at the code and here are some additional comments. Mostly small things. Regarding adding a flag for this, it looks like it can be done in a way that we still get full testing (given that the feature is on by default). If this is the case I can live with adding a flag to enable people to turn this feature off.

One thing that was brought to my attention was that ZGC also does proactive GCs, but the meaning of proactive differs from what this feature mean by it. It is a bit unfortunate to have something mean different things in different GCs and after some brainstorming the alternative "preemptive" or "preventive" came up. To me these are not really better or worse, but it might be nice to reuse the term "proactive". What do you guys think? We do preempt the mutator phase to do a GC.

src/hotspot/share/gc/g1/g1AllocRegion.hpp line 181:

> 179:   inline HeapWord* attempt_allocation_using_new_region(size_t min_word_size,
> 180:                                                      size_t desired_word_size,
> 181:                                                      size_t* actual_word_size);

Indentation off after naming change.

src/hotspot/share/gc/g1/g1AllocRegion.inline.hpp line 111:

> 109: inline HeapWord* G1AllocRegion::attempt_allocation_using_new_region(size_t min_word_size,
> 110:                                                                   size_t desired_word_size,
> 111:                                                                   size_t* actual_word_size) {

Same here, please fix indentation.

src/hotspot/share/gc/g1/g1Allocator.inline.hpp line 69:

> 67:   HeapWord* result = mutator_alloc_region(node_index)->attempt_allocation_using_new_region(word_size, word_size, &temp);
> 68:   assert(result != NULL || mutator_alloc_region(node_index)->get() == NULL,
> 69:          "Must not have a mutator alloc region if there is no memory, but is " PTR_FORMAT, p2i(mutator_alloc_region(node_index)->get()));

Minor nit, but I would prefer moving the `p2i()` to a new line to improve readability a bit.

src/hotspot/share/gc/g1/g1Policy.cpp line 1417:

> 1415: 
> 1416: bool G1Policy::proactive_collection_required(uint alloc_region_count) {
> 1417:   if (!Universe::is_fully_initialized()) {

It should be pretty straight forward to add a check here if we want to have a flag to control the feature, right? From what I can tell that would be the only thing needed or am I missing something? 

If this is the case I'm not as opposed, if the feature is on by default. Adding a way to opt-out is fine, especially if it makes "the field" happy =) But then the next question is what we should call the flag.

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

Changes requested by sjohanss (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3143