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

Thu Dec 10 13:37:45 UTC 2020

On Sun, 6 Dec 2020 17:39:54 GMT, Charlie Gracie <cgracie at openjdk.org> wrote:

> Bursts of short lived Humongous object allocations can cause GCs to be initiated with 0 free regions. When these GCs happen they take significantly longer to complete. No objects are evacuated so there is a large amount of time spent in reversing self forwarded pointers and the only memory recovered is from the short lived humongous objects. My proposal is to add a check to the slow allocation path which will force a GC to happen if the number of free regions drops below the amount that would be required to complete the GC if it happened at that moment. The threshold will be based on the survival rates from Eden and survivor spaces along with the space required for Tenure space evacuations.
> 
> The goal is to resolve the issue with bursts of short lived humongous objects without impacting other workloads negatively. I would appreciate reviews and any feedback that you might have. Thanks.
> 
> Here are the links to the threads on the mailing list where I initially discussion the issue and my idea to resolve it:
> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-November/032189.html
> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-December/032677.html

This is only a first pass over the changes; I need to think a bit more about the estimation, and whether there are any issues with this new heuristic, i.e. that we could end up doing lots of gcs that effectively achieve nothing at some point and whether this is preferable to running into an evacuation failure.

test/hotspot/jtreg/gc/g1/TestGCLogMessages.java line 316:

> 314:         private static byte[] garbage;
> 315:         private static byte[] largeObject;
> 316:         private static Object[] holder = new Object[800]; // Must be larger than G1EvacuationFailureALotCount

Just curious about these changes: it is not immediately obvious to me why they are necessary as the mechanism to force evacuation failure (G1EvacuationFailureALotCount et al) should be independent of these changes.

And the 17MB (for the humonguous object)+ 16MB of garbage should be enough for at least one gc; but maybe these changes trigger an early gc?

src/hotspot/share/gc/g1/g1VMOperations.hpp line 71:

> 69: class VM_G1CollectForAllocation : public VM_CollectForAllocation {
> 70:   bool _gc_succeeded;
> 71:   bool _force_gc;

Not completely happy about using an extra flag for these forced GCs here; also this makes them indistinguishable with other GCs in the logs as far as I can see.
What do you think about adding  GCCause(s) instead to automatically make them stand out in the logs?
Or at least make sure that they stand out in the logs for debugging issues.

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

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

Why not start the GC with a requested word_size == 0 here and remove the need for the flag (still there is need to record somehow the purpose of this gc)? In some way it makes sense as space is not really exhausted.

src/hotspot/share/gc/g1/g1Policy.hpp line 102:

> 100: 
> 101:   size_t _predicted_survival_bytes_from_survivor;
> 102:   size_t _predicted_survival_bytes_from_old;

As a non-English native speaker I think "survival_bytes" is strange as "survival" isn't an adjective. Maybe "surviving_bytes" sounds better?
The code in `calculate_required_regions_for_next_collect` uses "survivor" btw. I would still prefer "surviving" in some way as it differs from the "survivor" in "survivor regions", but let's keep nomenclature at least consistent.

Also please add a comment what these are used for.

src/hotspot/share/gc/g1/g1Policy.hpp line 368:

> 366:                                                  uint& num_optional_regions);
> 367: 
> 368:   bool can_mutator_consume_free_regions(uint region_count);

Comments missing.

src/hotspot/share/gc/g1/g1Policy.hpp line 369:

> 367: 
> 368:   bool can_mutator_consume_free_regions(uint region_count);
> 369:   void calculate_required_regions_for_next_collect();

"... for_next_collection" sounds better? This method name seems self-explaining, but it does not calculate the required regions but updates the two new members which contain values stored *in bytes*.

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

> 1450: bool G1Policy::can_mutator_consume_free_regions(uint alloc_region_count) {
> 1451:   uint eden_count = _g1h->eden_regions_count();
> 1452:   if (eden_count < 1) {

I'd prefer "eden_count == 0" here since it is an uint anyway.

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

> 1457:   // adjust the total survival bytes by the target amount of wasted space in PLABs.
> 1458:   // should old bytes be adjusted and turned into a region count on its own?
> 1459:   size_t const adjusted_survival_bytes_bytes = (size_t)(total_predicted_survival_bytes * (100 + TargetPLABWastePct) / 100.0);

Answering the question: yes, both young gen and old gen survivors must be treated separately and rounded up to regions seperately.

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

> 1455:   size_t const predicted_survival_bytes_from_eden = _eden_surv_rate_group->accum_surv_rate_pred(eden_count) * HeapRegion::GrainBytes;
> 1456:   size_t const total_predicted_survival_bytes = predicted_survival_bytes_from_eden + _predicted_survival_bytes_from_survivor + _predicted_survival_bytes_from_old;
> 1457:   // adjust the total survival bytes by the target amount of wasted space in PLABs.

This adjustment for wasted space (the `(100 + PLABWastePercent) / 100)` code) in PLABs is now done twice. Please extract a (maybe static) helper function.

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

> 1477: 
> 1478: void G1Policy::calculate_required_regions_for_next_collect() {
> 1479:   // calculate the survival bytes from survivor in the next GC

Comments that are sentences should start upper case and end with a full stop.

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

> 1488:   _predicted_survival_bytes_from_survivor = survivor_bytes;
> 1489: 
> 1490:   // calculate the survival bytes from old in the next GC

Same problem as above with the comment style. :) Please add a sentence that we use the minimum old gen collection set as conservative estimate for the number of regions to take for this calculation.

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

> 1494:     uint predicted_old_region_count = calc_min_old_cset_length();
> 1495:     uint num_remaining = candidates->num_remaining();
> 1496:     uint iterate_count = num_remaining < predicted_old_region_count ? num_remaining : predicted_old_region_count;

I kind of prefer using the `MIN2()` expression here instead of the "if" as it is what you want anyway (it's the same of course, but the use of MIN2 directly indicates that we are taking the minimum).

Not sure if adding the two locals `predicted_old_region_count` and `num_remaining` should be kept then. They do not add too much (and should be `const`).

I'm also not sure if the second part of the condition of the outer if (i.e. `&& !candidates->is_empty()`) is useful. `candidates->num_remaining()` should be zero in this case.

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

> 1492:   G1CollectionSetCandidates *candidates = _collection_set->candidates();
> 1493:   if ((candidates != NULL) && !candidates->is_empty()) {
> 1494:     uint predicted_old_region_count = calc_min_old_cset_length();

Or add the comment that we intentionally use `calc_min_old_cset_length` as an estimate for the number of old regions for this calculations here.

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

> 763:                                 bool*          succeeded,
> 764:                                 GCCause::Cause gc_cause,
> 765:                                 bool           force_gc);

If that parameter is kept, please add documentation.

src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 423:

> 421:   for (uint try_count = 1, gclocker_retry_count = 0; /* we'll return */; try_count += 1) {
> 422:     bool should_try_gc;
> 423:     bool force_gc = false;

`force_gc` and `should_try_gc` seems to overlap a bit here. At least the naming isn't perfect because we may not do a gc even if `force_gc` is true which I'd kind of expect.

I do not have a good new name right now how to fix this.

src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 428:

> 426:     {
> 427:       MutexLocker x(Heap_lock);
> 428:       if (policy()->can_mutator_consume_free_regions(1)) {

I would prefer if `force_gc` (or whatever name it will have) would be set here unconditionally as the `else` is pretty far away here.

I.e.
force_gc = policy()->can_mutator_consume_free_regions(1);

  if (force_gc) { // needing to use the name "force_gc" here shows that the name is wrong...
    ... try allocation
    ... check if we should expand young gen beyond regular size due to GCLocker
  }
The other issue I have with using `can_mutator_consume_free_regions()` here is that there is already a very similar `G1Policy::should_allocate_mutator_region`; and anyway, the `attempt_allocation_locked` call may actually succeed without requiring a new region (actually, it is not uncommon that another thread got a free region while trying to take the `Heap_lock`.

I think a better place for `can_mutator_consume_free_regions()` is in `G1Policy::should_allocate_mutator_region()` for this case.

`attempt_allocation_locked` however does not return a reason for why allocation failed (at the moment). Maybe it is better to let it return a tuple with result and reason (or a second "out" parameter)? (I haven't tried how this would look like, it seems worth trying and better than the current way of handling this).

This one could be used in the following code.

src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 459:

> 457:       bool succeeded;
> 458:       result = do_collection_pause(word_size, gc_count_before, &succeeded,
> 459:                                    GCCause::_g1_inc_collection_pause, force_gc);

I really think it is better to add a new GCCause for this instead of the additional parameter. We are not doing a GC because we are out of space because of the allocation of `word_size`, but a "pre-emptive" GC due to the allocation of `word_size`.
This warrants a new GC cause imho.

src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 885:

> 883:       bool succeeded;
> 884:       result = do_collection_pause(word_size, gc_count_before, &succeeded,
> 885:                                    GCCause::_g1_humongous_allocation, force_gc);

I believe the reason for this GC if `force_gc` is true is *not* that we do not have enough space for the humongous allocation, but we are doing a "pre-emptive" GC because of the allocation of "word_size" similar to before.

src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 860:

> 858: 
> 859:       size_t size_in_regions = humongous_obj_size_in_regions(word_size);
> 860:       if (policy()->can_mutator_consume_free_regions((uint)size_in_regions)) {

Again, I would prefer that the result of this is stored in a local not called `force_gc` :) and then used instead of appending such a small `else` after the "long'ish" true case.

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

Changes requested by tschatzl (Reviewer).

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