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

[Jesper Wilhelmsson]

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/

Thanks,
/Jesper


Kim Barrett skrev den 12/5/15 00:32:
> On May 7, 2015, at 10:48 AM, Jesper Wilhelmsson <jesper.wilhelmsson at oracle.com> wrote:
>> A new webrev is available here:
>> http://cr.openjdk.java.net/~jwilhelm/8077842/webrev.01/
>>
>> And the increment:
>> http://cr.openjdk.java.net/~jwilhelm/8077842/webrev.01.incremental/
>
> Comments inline.
>
>>> ------------------------------------------------------------------------------
>>> agent/src/share/classes/sun/jvm/hotspot/utilities/PointerLocation.java
>>>    87     return ((gen != null) && (gen == ((GenCollectedHeap)heap).getGen(0)));
>>> and
>>>    91     return ((gen != null) && (gen == ((GenCollectedHeap)heap).getGen(1)));
>>>
>>> Strictly speaking, it seems like the GenCollectedHeap cast ought to be
>>> protected by a try/catch for a bad cast.  Else why is there a
>>> CollectedHeap base class at all?
>>
>> Besides GenCollectedHeap there are two more classes that inherits
>> CollectedHeap, G1CollectedHeap and ParallelScavengeHeap. Neither of
>> these two uses the Generation class, so in order for gen to have a
>> non-null value, heap must be a GenCollectedHeap. This is in no way
>> obvious and the cast is, even though correct now, not extremely future
>> proof, so I changed the implementation here. Let me know what you
>> think.
>
> I think that, given the present structure, I would prefer to leave
> things as they were in the original webrev.  I also think there is
> some refactoring in this area that could be done later.
>
>>> On the other hand, maybe I'm missing something, but I don't see any
>>> code to assign a value to the "gen" field.
>>
>> There is code in PointerFinder.java that will set gen if it is a
>> GenCollectedHeap.
>
> Ick!  Why is that setup of an object performed outside the object,
> rather than as part if its constructor?  That's a rhetorical question;
> this is part of the needed refactoring I mentioned above, and not
> something I think needs to be part of this under review change.
>
>>> ------------------------------------------------------------------------------
>>> src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp
>>>   688     // I didn't want to change the logging when removing the level concept,
>>>   689     // but I guess this logging could say "old" or something instead of "1".
>>>   690     assert(this == gch->old_gen(),
>>>   691            "The CMS generation should be the old generation");
>>>   692     uint level = 1;
>>>
>>> I'm assuming this comment and the retention of level is still
>>> intended, for the reason give.  Is there a followup cleanup RFE for
>>> this?
>>>
>>> Similarly in genCollectedHeap.cpp, line 332 and following.
>>> Similarly in generation.cpp, line 115 and following.
>>
>> My initial thought was that the unified logging work would fix
>> this. I'm not sure if it matters too much if we make several changes
>> or one big one with regard to external parsing tools that reads the
>> output. The next patch in this series can clean up this logging if you
>> think it would be better to do it sooner that the unified logging
>> work.
>
> With that explanation, this is fine as is.
>
> Looks good otherwise.  Just the one issue related to PointerLocation.
>