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

[Stefan Johansson]

I followed the discussion and agree.

Change looks good,
Stefan

On 2014-05-19 23:17, Jesper Wilhelmsson wrote:
> Hi,
>
> After some discussions we have concluded that the merged flag used to 
> trigger young GCs before a full GC should keep the name 
> ScavengeBeforeFullGC. There were a couple of arguments for this, the 
> most important from my perspective that ScavengeBeforeFullGC is a 
> publicly documented flag.
>
> 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/
>
> Thanks,
> /Jesper
>
>
> 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
>>>>>>
>>>>
>>