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

[Luo, Ziyi]

My last email missed one comment, sorry. Appended inline:

Hi Thomas,

Thank you for your review. I've published a new webrev:
http://cr.openjdk.java.net/~bmathiske/8245511/webrev.02/

In reply to your comments:

> On 8/10/20, 5:17 AM, Thomas Schatzl wrote:

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

_old_gen_alloc_tracker is initialized at L76, not shown in this webrev :)

> - g1Policy.cpp:807:
>
> please break the line after the first parameter so that the line isn't
> that long.

Done.

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

Got you. s/cycle/period/g in G1OldGenAllocationTracker. In G1IHOPControl, I
used full name for the method to calculate the old gen allocation rate:
`G1AdaptiveIHOPControl::last_mutator_period_old_allocation_rate`.

> - 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)?

Combining these two comments:
I do not have a strong opinion where to keep the duration value. You are right
on that the allocation rate is currently only used by the IHOP so it makes
sense to relocate the calculation in G1IHOPControl:

I kept `_last_allocation_time_s` in G1IHOPControl and removed the duration
field (and refactored other methods such as reset_after(_full|_young)_gc) in
G1OldGenAllocationTracker. Besides, `mutator_time_s` is kept.

Added G1AdaptiveIHOPControl as a friend class of G1OldGenAllocationTracker.

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

I kept `_last_allocation_time_s` in G1IHOPControl as-is and removed the
new duration field in G1OldGenAllocationTracker. The value of
`_last_allocation_time_s` is passed from `app_time_ms` in g1Policy.cpp:649.
Looks to me it's non-trivial to transform the type of `app_time_ms` to
Tickspan.

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

Done

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

The test is moved to test_g1IHOPControl as the allocation rate calculation is
relocated to G1AdaptiveIHOPControl. This guard is required now.

> I will do some perf checking.

Please let me know if you have any findings. Thank you!

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

The log emitted by G1OldGenAllocationTracker only contains the allocated-byte
stats in the last mutator period. Placing it under "gc, ihop" does not seem
accurate either. How about "gc, alloc, stats"?

Best,
Ziyi