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

[Stefan Johansson]

On Thu, 29 Apr 2021 18:41:24 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:
>> - _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.
>> - _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:
> 
>   Remove temp comment.

I think this is starting to look pretty good. No major comments at this point, but I will have to take one more look in detail at the calculations in the policy.

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

> 171:   // to the locking protocol.
> 172:   // Tries to allocate at least min_word_size words, and at most desired_word_size.
> 173:   // Returns the actual size of the block in actual_word_size.

Please format these lines to fit the rest of the comment:
Suggestion:

  // to the locking protocol. The min and desired word size allow
  // specifying a minimum and maximum size of the allocation. The
  // actual size of allocation is returned in actual_word_size.

`use_retained_region_if_available` is no longer used remove from comment.

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

> 54:   uint node_index = current_node_index();
> 55: 
> 56: 

Remove at least one of those two blank lines.

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

> 1420:   }
> 1421: 
> 1422:   if (_g1h->young_regions_count() == 0 && _collection_set->candidates() != NULL && _collection_set->candidates()->is_empty()) {

Add a comment that we have no regions to collect. Adding a `has_candidates()` to `G1CollectionSet` seems to make sense now when we check for candidates a couple of times and will make this condition read a bit better as well:
Suggestion:

  if (_g1h->young_regions_count() == 0 && !_collection_set->has_candidates()) {
    // Don't attempt a proactive GC when there are no regions to collect.

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

> 1466:   // Old regions
> 1467:   G1CollectionSetCandidates *candidates = _collection_set->candidates();
> 1468:   if (candidates == NULL || candidates->is_empty()) {

Could use `!_collection_set->has_candidates()` if added.

src/hotspot/share/gc/g1/g1VMOperations.cpp line 125:

> 123:   G1CollectedHeap* g1h = G1CollectedHeap::heap();
> 124: 
> 125:   if (_word_size > 0 && _gc_cause != GCCause::_g1_proactive_collection) {

What do you think about adding a helper to ´VM_G1CollectForAllocation`:

bool try_allocate_before_gc() {
  if (_gc_cause == GCCause::_g1_proactive_collection) {
    // Never allocate before proactive GCs.
    return false;
  }
  return true;
}

The condition would then read a bit better:
Suggestion:

  if (try_allocate_before_gc() && _word_size > 0) {

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

Changes requested by sjohanss (Reviewer).

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