RFR (S) 8246274: G1 old gen allocation tracking is not in a separate class

[Thomas Schatzl]

Hi,

   lgtm. Thanks.

Thomas

On 04.06.20 18:15, Luo, Ziyi wrote:
> Hi Thomas,
> 
> A third revision is here:
> http://cr.openjdk.java.net/~phh/8246274/webrev.02
> 
> On 6/4/20, 3:51 AM, Thomas Schatzl wrote:
> 
> [..]
> 
>>>> - I would prefer if there were separate reset_after_[young_]gc() and a
>>>> reset_after_full_gc() methods. Initially I asked myselves why for full
>>>> gc that first parameter passed to reset_after_gc() is zero, and only
>>>> when looking into the code found that it is ignored anyway. I think the
>>>> API of that class isn't that huge yet.
>>>
>>> Make sense. I refactored it into two methods in rev.01:
>>> * reset_after_full_gc()        for full GC
>>> * reset_after_incremental_gc() for young and mixed GC
>>>
> 
>> I would prefer to use "reset_after_young_gc" for the latter - almost all
>> existing code uses "young" already to indicate incremental gc (except
>> the MBeans support code, but this is _very_ old code).
>>
>> The "correct" terms of the two types of young gc would be "young only"
>> and "mixed" btw. Even "young only" has for a long time now also
>> reclaimed some kind of old gen (humongous) regions.
>> I do not expect this to go away in the future but the line blurring even
>> more with potentially adding defragmentation work to young only to
>> facilitate humongous allocation.
> 
> Thanks for the explanation. Fixed in rev.02.
> 
>> And as an ultra-minor nit, please remove the "private" visibility
>> specifier in the g1OldGenAllocationTracker class as its default.
> 
> Done
> 
>> As for performance testing I will do that in conjunction with the
>> changes to the actual IHOP calculations.
> 
> Yep. Besides, I'll add a new gtest for g1OldGenAllocationTracker in the next
> change.
> 
>> Do you need a sponsor for this change or has Paul Hohensee offered to do
>> that?
> 
> Thanks, Paul will sponsor me.
> 
> Best,
> Ziyi
>