RFR: 8245511: G1 adaptive IHOP does not account for reclamation of humongous objects by young GC

[Thomas Schatzl]

Hi,

   apologies for the delay. Other things (jdk15 release, vacations) kept 
me from getting to this change in time:

On 17.07.20 22:58, Luo, Ziyi wrote:
> Hi,
> 
> Please review this patch to adjust the old gen allocation rate for G1 Adaptive
> IHOP to consider the humongous objects freed by young collector.
> 
> Webrev: http://cr.openjdk.java.net/~bmathiske/8245511/webrev.01/
> CR: https://bugs.openjdk.java.net/browse/JDK-8245511
> 
> This revision addresses a comment from Thomas in webrev.00:
>> one other improvement could be to put the old gen allocation tracking into
> its own class...
> 
> This is added in JDK-8245511. This RFR refactors G1*IHOPControl to no longer
> store any allocation data but read from the old gen allocation tracker
> instance.
> 
> To answer the following questions from Thomas:
>> Quick(!) testing on the reproducer with the suggested patch showed around
> 23% of young gcs were evacuation failures
> 
> The evacuation failures are "by design". The test case attached to the JBS bug
> intends to create a low pressure on the survive space and then suddenly
> increase the life cycle of objects in the Eden space to create a to-space
> exhaustion (I checked GC log, all evacuation failures are to-space
> exhaustions). This is the fastest approach I can come up with to get objects
> promoted to the old gen directly to trigger enough concurrent cycles for
> Adaptive IHOP to sample. Once Adaptive IHOP starts to predict, the to-space
> exhaustion is no longer needed. The evacuation failures are also observed in
> test run with Static IHOP. Why then are not observed without this fix? Because
> GC happens every 50 ms. Even during the allocation spike, only one region is
> filled up in Eden :)
> 
>> but in return the change imho needs to provide some measure of the spikyness
> of the allocation between gcs. Otherwise G1 will run into to-space exhaustions
> and potentially full gcs all the time with such loads.
> 
> I agree that the spikiness of the allocation between GCs should be considered
> and I understand your concern here. My fix might bring the old gen allocation
> rate too low that theoretically back-to-back full GC is possible. However, I
> was not able to create a test case to produce it as those short-live humongous
> objects are reclaimed so rapidly (my patch only accounts humongous objects
> that are allocated and reclaimed in the same allocation cycle). Could you give
> me some hint on how to produce the worst-case scenario?
> 
> Tests:
> All hotspot tier-1 passed in fastdebug build.
> 

Looks good, mostly some comments about code location/naming:

- g1Policy.cpp:60:

   _ihop_control(create_ihop_control(&_old_gen_alloc_tracker, &_predictor)),

Please initialize the _old_gen_alloc_tracker before using it. Although 
only the reference is passed, the constructor might do something with it 
(in the future). Actually I remember that at some point MSVC complained 
about such uses, but apparently does not any more, or it got more clever 
about it.

- g1Policy.cpp:807:

please break the line after the first parameter so that the line isn't 
that long.

- G1OldGenAllocationTracker: I dislike the use of "cycle" for the recent 
mutator period. While the previous change introduced it, now that it is 
used in a widespread way I would like this change to rename it. (GC) 
"Cycle" is in G1 typically used for the time between young gen and the 
next young gen after a marking, so this is confusing.

The time from one gc to the next gc also does not have any cyclical 
behavior.

Something like "mutator_period" would imho be better, but it makes some 
of the already long identifiers even longer. Maybe these can be 
shortened a bit?

- in G1Policy::update_ihop_prediction() I would prefer if the 
mutator_time_s parameter were either kept, or the 
"_old_gen_alloc_tracker.last_cycle_duration() > min_valid_time" 
condition moved into G1IHOPControl::update_allocation_info(). The reason 
is that "cycle" is an already overloaded term and looks bad here. Maybe 
this code will also improve if the above renaming of "cycle" mentioned 
above would be done.

- Related to that I was wondering whether "last_cycle_duration" should 
actually be placed into OldGenAllocTracker or not. I am kind of seeing 
G1OldGenAllocationTracker as plain storage for just the byte sizes.

Also all the rates (and that member) are only used in G1IHOPControl 
which indicates that maybe wrt to information hiding it and the uses of 
it may be better located in G1IHOPControl (as it were)?

- new durations should be of type Tickspan, not double. This saves you 
the "_s" (or whathever) suffix too.

- g1OldGenAllocationTracker.hpp:65 and 80: break the last line of the 
comment paragraph again to avoid.

- the G1OldGenAllocationTracker simple doesn't need a "if (!UseG1GC" 
guard as other g1 tests.

Looks good otherwise.

I will do some perf checking.

Thanks,
   Thomas