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

[Thomas Schatzl]

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?

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

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

Thanks,
   Thomas