RFR: 8042298 - Remove the names gen0 and gen1 from the GC code

[John Coomes]

Jesper Wilhelmsson (jesper.wilhelmsson at oracle.com) wrote:
> ...
> So changes since last webrev is that all former usages of CollectGen0First is 
> now using ScavengeBeforeFullGC, and the removal of ScavengeBeforeFullGC is 
> reverted. There is also a check in arguments.cpp to set the correct default 
> value if not running ParallelGC. CollectGen0First and ScavengeBeforeFullGC had 
> different default values.
> 
> An updated webrev is available here:
>   http://cr.openjdk.java.net/~jwilhelm/8042298/webrev.3/

This looks good to me.

-John

> Stefan Johansson skrev 16/5/14 13:45:
> > Thanks Jesper,
> >
> > On 2014-05-16 02:10, Jesper Wilhelmsson wrote:
> >> Hi,
> >>
> >> A new webrev is available here:
> >> http://cr.openjdk.java.net/~jwilhelm/8042298/webrev.2/
> >>
> >> In addition to the last webrev the flags TraceGen0Time, TraceGen1Time and
> >> CollectGen0First have been renamed to get rid of the Gen0/Gen1.
> >>
> >> Also, since CollectGen0First and ScavengeBeforeFullGC was doing the same thing
> >> for different collectors, ScavengeBeforeFullGC has been merged with
> >> CollectGen0First.
> >>
> > Looks good!
> >
> > Stefan
> >> A CCC request has been filed for the flag changes.
> >>
> >> Thanks,
> >> /Jesper
> >>
> >>
> >> Stefan Johansson skrev 15/5/14 12:09:
> >>> On 2014-05-14 23:21, Jesper Wilhelmsson wrote:
> >>>> Hi Stefan,
> >>>>
> >>>> Thanks for looking at this! See comments inline.
> >>>>
> >>>> Stefan Johansson skrev 14/5/14 15:11:
> >>>>> Hi Jesper,
> >>>>>
> >>>>> Thanks for doing this work. Here are some comments:
> >>>>>
> >>>>> src/share/vm/gc_implementation/g1/g1CollectorPolicy.cpp:
> >>>>> * TraceGen0Time and TraceGen1Time should be changed to TraceYoungTime and
> >>>>> TraceOldTime for consistency, and I guess that will need a CCC request.
> >>>>>
> >>>>> src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.hpp
> >>>>>
> >>>>>
> >>>>>
> >>>>> * CollectGen0First -> CollectYoungFirst, same as above. Also used in
> >>>>> tenuredGeneration.hpp.
> >>>>>
> >>>>> src/share/vm/gc_implementation/g1/g1CollectorPolicy.hpp
> >>>>> * Comment starting on line 41 mentions TraceGen0Time and gen1.
> >>>>
> >>>> I agree that the names gen0 and gen1 should be completely removed, which
> >>>> includes changing these flags too. Initially I wanted this change to be a
> >>>> simple name change so that it would be easy to review it. Changing the flags
> >>>> is borderline to be more than a name change, but I'm fine with including that
> >>>> change.
> >>>>
> >>>> I'll file a CCC to change the flags.
> >>>>
> >>> Great, thanks.
> >>>>> As an additional comment, I think that GenCollectedHeap::get_gen(int)
> >>>>> should be
> >>>>> broken up into two functions get_young() and get_old(). I get that this was
> >>>>> left
> >>>>> to a later patch to avoid having code/logic changes in this patch but I think
> >>>>> the change is so little that it makes sense to do it in this patch. I would
> >>>>> like
> >>>>> to avoid having code like, even if it is for a brief period of time:
> >>>>> Generation* young = gch->get_gen(0);
> >>>>
> >>>> This change is part of the larger removal of the generation array which is the
> >>>> last part of this series of collector policy cleanups. Initially I was OK with
> >>>> moving get_young() and get_old() to this change even though that would mean
> >>>> that the change would grow considerably in size and wouldn't be a simple name
> >>>> change anymore. After all the only reason I wanted to keep it small was to
> >>>> make it easier for the reviewers :-)
> >>>>
> >>> That's good, thanks =)
> >>>> However, when I started to look at it in more detail I realized that removing
> >>>> get_gen(int) requires non-trivial changes in the serviceability agent which is
> >>>> the only reason why the last change in this series isn't done yet.
> >>>>
> >>> I understand that you might want to do all the changes in the SA at once, and
> >>> since this patch doesn't require you to change the SA I'm fine with leaving it
> >>> as is.
> >>>> It would be possible to introduce get_young() and get_old() and replace all
> >>>> usages of get_gen(int) within the VM, but leave get_gen(int) for the SA to
> >>>> use. I don't really like this approach, but if you prefer it over just
> >>>> changing the names it's a possibility.
> >>>>
> >>> I'm fine with any of the approaches, none of them are perfect but both are good
> >>> so I'll let you decide how you want to handle it.
> >>>
> >>> Thanks,
> >>> Stefan
> >>>> Thanks,
> >>>> /Jesper
> >>>>
> >>>>>
> >>>>> Thanks,
> >>>>> Stefan
> >>>>>
> >>>>> On 2014-05-02 03:39, Jesper Wilhelmsson wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>> This is a step towards removing the _generations array. The names gen0 and
> >>>>>> gen1 are replaced with young and old to use a more standardized vocabulary.
> >>>>>>
> >>>>>> Webrev: http://cr.openjdk.java.net/~jwilhelm/8042298/webrev/
> >>>>>>
> >>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8042298
> >>>>>>
> >>>>>> Thanks,
> >>>>>> /Jesper
> >>>>>
> >>>
> >