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

[Thomas Schatzl]

Hi,

On 11.08.20 17:32, Luo, Ziyi wrote:
> 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 :)

So the code passes a reference to an uninitialized variable to the 
_ihop_control constructor, which is exactly the problem :). It is 
cleaner if _old_gen_alloc_tracker were located somewhere before 
_ihop_control in the initializer list. This means you need to shuffle 
the class members a bit too.

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

Looks better now imho.

> 
> Added G1AdaptiveIHOPControl as a friend class of G1OldGenAllocationTracker.
> 

I would prefer a method in G1OldGenAllocationTracker that returned the 
recent survived old gen bytes (saturated at the bottom to 0) instead of 
the friend declaration.

I.e. instead of

double G1AdaptiveIHOPControl::last_mutator_period_old_allocation_rate() 
const {
   // The upper limit of the freed region count is the number of regions 
allocated
   // since the last gc. When more humongous regions survived the 
current gc than
   // survived the previous one, deduct the increment.
   assert(_last_allocation_time_s > 0, "This should not be called when 
the last GC is full");
   size_t freed_humongous_bytes = 
_old_gen_alloc_tracker->_last_period_humongous_bytes;
   size_t humongous_bytes_after_penultimate_gc = 
_old_gen_alloc_tracker->_humongous_bytes_after_penultimate_gc;
   size_t humongous_bytes_after_last_gc = 
_old_gen_alloc_tracker->_humongous_bytes_after_last_gc;
   size_t last_period_old_bytes = 
_old_gen_alloc_tracker->last_period_old_bytes();

   if (freed_humongous_bytes > 0 && humongous_bytes_after_penultimate_gc 
< humongous_bytes_after_last_gc) {
     freed_humongous_bytes -= humongous_bytes_after_last_gc - 
humongous_bytes_after_penultimate_gc;
   }
   assert(last_period_old_bytes >= freed_humongous_bytes, "Allocation 
rate cannot be negative");
   return (last_period_old_bytes - freed_humongous_bytes) / 
_last_allocation_time_s;
  }

something like

double G1AdaptiveIHOPControl::last_mutator_period_old_allocation_rate() 
const {
   return _old_gen_alloc_tracker->last_period_net_survived_old_bytes() / 
_last_allocation_time_s;
}

and move all that calculation into the old gen allocation tracker. THis 
would afaics avoid the friend declaration and be cleaner.

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

Okay, fair point.

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

All good with our standard benchmarks. I forgot to do regression testing 
(tier1-5) yesterday, is running with the new patch. I'll shout if 
there's an issue.

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

That's better, yes.

Some pre-existing issue, please fix while there: there is a missing 
whitespace between the closing parenthesis and the curly brace.

G1IHOPControl* G1Policy::create_ihop_control(const 
G1OldGenAllocationTracker* old_gen_alloc_tracker,
                                              const G1Predictions* 
predictor){
          ^^^

Please try to provide an incremental webrev too so that we reviewers do 
not have to look through all changes all the time. It's easy to do (and 
as soon as we're on github they will be created automatically):

- enable mq

- make a new mq revision of the current patch (e.g. hg qnew 8245511-base 
-u ...)

- do your new changes

- make a new mq revision of the changes (e.g. hg qnew 
8245511-tschatzl-review -u ...)

- create a diff webrev:

rm -rf webrev* ; webrev -r -2 ; mv webrev webrev.2_to_3 (in this case)
rm -rf webrev* ; webrev -r -3 (or to qbase); mv webrev webrev.3

upload both webrev.2_to_3 and webrev.3 .

Also, this is your second patch, isn't it? I can sponsor this one, but 
please apply for authorship after that :)

Thanks,
   Thomas