RFR: JDK-8077842 - Remove the level parameter passed around in GenCollectedHeap

[Mikael Gerdin]

Hi Jesper,

On 2015-06-02 19:56, Jesper Wilhelmsson wrote:
> Hi Mikael,
>
> Thanks for reviewing!
>
> A new version is available here:
> http://cr.openjdk.java.net/~jwilhelm/8077842/webrev.05/
>
> and the increment:
> http://cr.openjdk.java.net/~jwilhelm/8077842/webrev.05.incremental/
>
> A few answers inline.
>
> Mikael Gerdin skrev den 1/6/15 17:23:
>> Hi Jesper,
>>
>>
>> On 2015-05-28 17:57, Jesper Wilhelmsson wrote:
>>> Hi,
>>>
>>> Another non-trivial merge later the webrev looks like this:
>>>
>>> http://cr.openjdk.java.net/~jwilhelm/8077842/webrev.04
>>>
>>> I reran all the tests and it turned out that the new assert in
>>> defNewGeneration.cpp was assuming that the generations were set up,
>>> which was not true at startup. So I have changed the assert to:
>>>
>>> +DefNewGeneration::IsAliveClosure::IsAliveClosure(Generation* young_gen)
>>> : _young_gen(young_gen) {
>>> +  assert(_young_gen->kind() == Generation::ParNew ||
>>> +         _young_gen->kind() == Generation::DefNew, "Expected the young
>>> generation here");
>>>
>>> This is more like the old assert that also looked at the properties of
>>> the generation itself:
>>>
>>> -DefNewGeneration::IsAliveClosure::IsAliveClosure(Generation* g) :
>>> _g(g) {
>>> -  assert(g->level() == 0, "Optimized for youngest gen.");
>>>
>>> Compared to the other webrevs this is the only change modulo some code
>>> that was removed by recent changes and didn't need to be changed any
>>> more.
>>>
>>> Still need a Reviewer to have a look at this.
>>
>> Now that you've introduced the Generation::Type enum, can you use that
>> instead
>> of doing pointer compares on Generation instances?
>> It "doesn't feel right" to do all these pointer compares even though I
>> know they
>> are singletons and so on.
>
> It's slightly problematic since the generation itself doesn't know if it
> is young or old. We could introduce a Type variable and pass a boolean
> to the constructor to tell the generation which one it is, or implement
> a Generation::age() that looks at the kind() and determines which
> generation it is. I don't like either of these, maybe there is a third
> option?

My thought was that since you've introduced a Generation::Type enum a 
generation could have a member variable of that type and an associated 
getter for the value (and predicates is_x if we want to)

Just seeing the enum in Generation more or less made me expect to find 
such a field and accessor in the Generation class.


>
> The way I cleaned it up now was to introduce the
> is_young_gen()/is_old_gen() as suggested by Kim. These still do the
> pointer comparison but it's now only in one place.

This will work fine as long as we don't need to determine if a 
generation is old or young in the setup of GenCollectedHeap.
If _young_gen is null then GCH->is_young_gen(g) will always return false.

>
>>
>> ConcurrentMarkSweepGeneration::printOccupancy acesses the
>> GenCollectedHeap::_young_gen field instead of using the accessor, is
>> that intended?
>
> Nope. Fixed.
>
>>
>> Further cleanups:
>> Generation::update_gc_stats could be moved to CardGeneration (since
>> that's the
>> common superclass for old gens) if GenCollectedHeap::_old_gen had
>> CardGeneration* as its type.
>
> I'll put it on my list. It is the fifteenth cleanup in this series :)
:D

/Mikael

> /Jesper
>
>>
>> /Mikael
>>
>>>
>>> Thanks,
>>> /Jesper
>>>
>>>
>>> Jesper Wilhelmsson skrev den 20/5/15 19:15:
>>>> Kim Barrett skrev den 14/5/15 00:03:
>>>>> On May 12, 2015, at 3:02 PM, Jesper Wilhelmsson
>>>>> <jesper.wilhelmsson at oracle.com> wrote:
>>>>>>
>>>>>> Thanks Kim!
>>>>>>
>>>>>> I reverted the latest change in PointerLocation.java and
>>>>>> CollectedHeap.java,
>>>>>> now we're back to casts :)
>>>>>>
>>>>>> That's the only change in this webrev:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~jwilhelm/8077842/webrev.02/
>>>>>
>>>>> Looks ok.
>>>>>
>>>>> I’m assuming the (Java) GenCollectedHeap.getGen() will eventually be
>>>>> replaced
>>>>> with specific young/old-gen accessors?
>>>>>
>>>>
>>>> I filed JDK-8080765 to handle the cleanups in the SA.
>>>>
>>>> Since the restructure of the GC files went in I had to rebase my patch.
>>>> Mercurial handled it flawlessly and I only had to merge two files
>>>> where I added
>>>> a new include, genMarkSweep.cpp and vmGCOperations.hpp
>>>>
>>>> A new webrev with the new patch is available here:
>>>>
>>>> http://cr.openjdk.java.net/~jwilhelm/8077842/webrev.03
>>>>
>>>> Thanks,
>>>> /Jesper