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

[Thomas Schatzl]

On 10.08.20 11:59, Thomas Schatzl wrote:
> 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.

Wrong wording, sorry. I understand that mutator -> gc -> mutator -> gc 
-> mutator -> gc is some cyclic behavior, but I would prefer a more 
specific name than that. As mentioned, the (GC) cycle is defined in 
documentation as the image at 
https://docs.oracle.com/javase/9/gctuning/img/jsgct_dt_001_grbgcltncyl.png 
(this is for jdk9).

> 
> 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.

Another item: the new log messages should probably use "gc, ihop" tags, 
not "gc, stats" to group them together with IHOP statistics, or 
alternatively something new. Currently the new log messages are grouped 
with marking statistics which does not fit, and "stats" is quite unspecific.

Thanks,
   Thomas