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

[Hohensee, Paul]

Thanks, Thomas. Looks good to me as well. I've submitted a submit repo run just to be sure, and will push when it successfully completes.

Paul

On 6/4/20, 10:09 AM, "hotspot-gc-dev on behalf of Thomas Schatzl" <hotspot-gc-dev-bounces at openjdk.java.net on behalf of thomas.schatzl at oracle.com> wrote:

    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
    >