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

[Mikael Gerdin]

Jesper,

On 2015-06-11 19:04, Jesper Wilhelmsson wrote:
> Mikael Gerdin skrev den 11/6/15 13:36:
>> Kim,
>>
>> On 2015-06-10 22:12, Kim Barrett wrote:
>>> On Jun 3, 2015, at 9:01 AM, Mikael Gerdin <mikael.gerdin at oracle.com>
>>> wrote:
>>>>
>>>> On 2015-06-03 02:06, Kim Barrett wrote:
>>>>> On Jun 2, 2015, at 1:56 PM, Jesper Wilhelmsson
>>>>> <jesper.wilhelmsson at oracle.com> wrote:
>>>>>> I also added the is_young_gen()/is_old_gen() in GenCollectedHeap as
>>>>>> suggested. There is as you mentioned no way to determine which
>>>>>> generation
>>>>>> it is inside the generation. I haven't found any good way to
>>>>>> implement
>>>>>> that, and I'm also not sure I want to since I think that the
>>>>>> generation
>>>>>> shouldn't need to know this... I may be wrong though.
>>>>>
>>>>> That’s pretty much what I was thinking too.  Asking a generation
>>>>> for its
>>>>> Generation::Type smells a lot like asking it for its level.
>>>>
>>>> I don't think that asking a Generation for its type is necessarily bad.
>>>>  From my view, level is a bad concept because it suggests that
>>>> Generations
>>>> can be freely reordered in the hierarchy when in fact they cannot.
>>>> DefNew
>>>> (and its sublcasses) must always be young and CardGeneration (and its
>>>> sublcasses) must always be old.
>>>>
>>>> My thought was that the constructors of those classes would pass a
>>>> Generation::Type to the Generation constructor and set their type
>>>> through that.
>>>>
>>>> By doing the generation hierarchy cleanup we are moving towards a
>>>> world where
>>>> we most of the time know if we are dealing with an old or a young
>>>> Generation
>>>> object, but at some places we still need to figure out that and
>>>> asking the
>>>> heap for that is slightly strange to me.
>>>
>>> Oops, just noticed this lingering comment.
>>>
>>> Mikael makes a good argument here.  The key point is that the concrete
>>> classes know which generation they are supposed to be.
>>>
>>> However, I'd prefer not exposing the tag via a generation type
>>> accessor, but would still prefer is_{young,old}[_gen] predicates, just
>>> moved to the generation class.  Whether these are implemented via
>>> looking at information recorded in the Generation and provided by the
>>> derived class at construction time, or are virtual functions
>>> implemented in the derived classes, I don't think I care.  I haven't
>>> audited the (current) places where these would be looked at, but it
>>> seems to me that performance-critical code will likely know what kind
>>> of generation it is dealing with.
>>
>> Ok, I think we are in agreement then. I don't really have a strong
>> opinion on
>> where the is_{young,old} predicates should go.
>>
>> I think this change is ready to go then.
>
> Oh.. I thought about your comments and found myself moving the enum to
> GenCollectedHeap. This is the class that knows about the young and old
> generations.

Oh, ok :)

>
> In some kind of object oriented way, I don't think that a generation
> should (have to) know it's type, or even that there are different types
> of generations.

I'm not sure I entirely agree with that statement, it seems to me that 
this is similar to the "old" way of having a bunch of generations 
floating around with only a "level" and no idea about how everything 
ties together. This was nice in theory but in fact all the generations 
are only ever able to function as an oldest or youngest generation and 
everything would break down if that was changed. I think it's just 
dishonest to not admit that a ConcurrentMarkSweepGeneration is an "old" 
generation and that a DefNewGeneration is a "young" generation.
Nevertheless I think your change is a step in the right direction and I 
won't argue for changing this.

>
> The new version is here:
> http://cr.openjdk.java.net/~jwilhelm/8077842/webrev.06/
> http://cr.openjdk.java.net/~jwilhelm/8077842/webrev.06.incremental/

In genCollectedHeap.[ch]pp when you refer to the members of the 
GenerationType enum you don't need to scope them with GenCollectedHeap::

Otherwise the change looks good.

/Mikael

>
> /Jesper
>
>>
>> Jesper, hit the switch :)
>>
>> /Mikael
>>
>>>