RFR: 8297186: G1 triggers unnecessary full GCs when heap utilization is low [v3]

[Kim Barrett]

On Thu, 1 Dec 2022 11:25:55 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> src/hotspot/share/gc/g1/g1Policy.cpp line 316:
>> 
>>> 314:     log_trace(gc, ergo, heap)("Young target length: Desired young length smaller than allocated after GC. Allowing one extra eden region.");
>>> 315:     desired_young_length = allocated_young_length + 1;
>>> 316:   }
>> 
>> I don't understand this.  The log message doesn't match the test being performed (log says "smaller" but
>> the test is for `<=`).  And the action taken seems kind of mysterious.  I think it's to force us into the `else`
>> clause of the following `if` statement.  It might be more obvious if instead of this the `if` condition was
>> changed to `(!after_gc && allocated_young_length >= desired_young_length)`, though then there is some
>> additional stuff to do in the `else` branch too.  A comment instead would help.
>
> The change forces the remaining code in `calculate_young_target_length` to add an extra region to the number of desired regions if the number of currently allocated regions is equal or exceeds them.
> This override should only be used if the caller requested it, i.e. after a young gc to avoid an immediate full gc right away because the allocation that caused the gc could not be satisfied with zero eden regions.
> 
> E.g.
> desired regions = 1 (because of very small heap size)
> allocation regions = 1 (because of survivor regions from previous gc)
> 
> Now this leaves zero regions for eden for the next allocation, causing the described full gcs.
> This code pretends to the actual target calculation (if after a gc) that we actually want one eden region.
> 
> As for the location of this `if`, I moved it to `calculate_young_desired_length` with a comment, since this code actually modifies the desired length. It's imho still a bit awkward there because it is kind of a tacked on condition, but I do not know a better place for this code (I've been fairly undecided about that in the original code alreayd).

Thanks for the restructuring.  I think I understand what this is doing now.

I think the new structure is better, though I think the comment on the place
where the desired_young_length adjustment occurs could be improved. I think
what's happening there is that when computing the target after a GC, since we
want at least one eden region, and since allocated_young_length at that point
is the number of survivor regions, the desired length must exceed the
allocated (survivor) length.

But with that in mind, I think the prior name for the new argument
("after_gc") is better than the new name ("need_one_eden_region").  It's not
so much that the caller wants at least one eden region, it's that the policy
is that there should be at least one eden region after a GC.

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

PR: https://git.openjdk.org/jdk/pull/11209