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

[Jesper Wilhelmsson]

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?

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.

>
> 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 :)
/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