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

Jesper Wilhelmsson jesper.wilhelmsson at oracle.com
Mon May 19 21:17:35 UTC 2014


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


More information about the hotspot-gc-dev mailing list