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

[Kim Barrett]

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.