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

[Luo, Ziyi]

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,
>
> On 02.06.20 01:12, Luo, Ziyi wrote:
>> Hi,
>>
>> Could you please review this change which refactors
>> G1Policy::_bytes_allocated_in_old_since_last_gc into a dedicated new
>> tracking class G1OldGenAllocationTracker?
>>
>> Bug ID:
>> https://bugs.openjdk.java.net/browse/JDK-8246274
>> Webrev:
>> http://cr.openjdk.java.net/~phh/8246274/webrev.00/
>>
>> Testing: Local run hotspot:tier1.
>>
>> This is the first step toward improving the G1 old gen allocation tracking. As
>> described in JDK-8245511, we will further add humongous allocation tracking
>> and refactor G1IHOPControl::update_allocation_info(). This is a clean
>> refactoring of the original G1Policy::_bytes_allocated_in_old_since_last_gc
>> field and G1Policy::add_bytes_allocated_in_old_since_last_gc() method.
>>
>> Thanks,
>> Ziyi
>>

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

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

Thanks,
Ziyi