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

[Stefan Johansson]

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