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

[Thomas Schatzl]

Hi,

On 04.06.20 01:15, Luo, Ziyi wrote:
> Hi Thomas, thanks for your review. A second revision is published:
> 
> http://cr.openjdk.java.net/~phh/8246274/webrev.01
> (all hotspot:tier1 passed)
> 
> On 6/3/20, 2:06 AM, Thomas Schatzl wrote:
> 
>> Hi,

[..]
>>     - I suggest to keep the existing public interface in G1Policy, i.e.
>> the add_bytes_allocated_in_old_since_last_gc. Making the old gen tracker
>> object public does not seem to be advantageous.
>> I.e. imo it is good to group everything related to old gen allocation
>> tracking into that helper class, but we do not need to expose that fact.
> 
>> Maybe there is something in a follow up change that requires this?
> 
> Yes, the follow up change will introduce two more interfaces in
> G1OldGenAllocationTracker to track the regular and on-collection-pause
> humongous allocations respectively in G1CollectedHeap.

Okay, then let's wait for that.

> 
>>     - the _old_gen_alloc_tracker instance can be aggregated within the
>> G1Policy class directly, i.e. there is no need to make it a pointer and
>> manage via new and delete afaics.
> 
>> Maybe there is something in a follow up change that requires this?
> 
> You are right, fixed in rev.01
> 
>> - 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.

And as an ultra-minor nit, please remove the "private" visibility 
specifier in the g1OldGenAllocationTracker class as its default.

As for performance testing I will do that in conjunction with the 
changes to the actual IHOP calculations.

Do you need a sponsor for this change or has Paul Hohensee offered to do 
that?

Thanks,
   Thomas