RFR: JDK-8061802 - REDO - Remove the generations array

[Kim Barrett]

On Feb 10, 2015, at 3:38 PM, Jesper Wilhelmsson <jesper.wilhelmsson at oracle.com> wrote:
> 
> Hi Kim,
> 
> Thanks for taking another close look at this change!
> 
> Some of these issues are the result of this being a part of a bigger change, but I have addressed your concerns to make sure this intermediate version is of decent quality even without the following cleanups. (Even though I hope we will get the rest in asap. Those will be a lot easier to review).
> 
> For example anything related to levels and _n_gens will be removed in later patches when the entire level concept is removed.

That’s what I thought, but I would prefer having this changeset be able to stand on its own merits.  I think the further changes will be easier to understand with the additional (usually temporary) asserts and such, especially for anyone who didn’t review this changeset.

> A new webrev is available here:
> http://cr.openjdk.java.net/~jwilhelm/8061802/webrev.02/

This webrev seems to be broken - genCollectedHeap.cpp frame display is messed up.  Old version is showing as empty, although new version still highlights differences.  I didn’t look at it carefully though, since I’m not sure I’d be looking at anything like the intended code.

A few inline replies.

>> ------------------------------------------------------------------------------
>> src/share/vm/memory/genCollectedHeap.cpp
>> collect_generation and do_collection
>> 
>> I tried to give the changes here a thorough look, but the diffs are
>> very messy.  I hope I didn't miss anything.
>> 
>> I think the review of this would have been noticably easier if the
>> extraction of collect_generation into a separate function had happened
>> before and independently of the generation array changes.  Oh well.
> 
> That may be true, even though I believe that the only major difference if this would have been done separately would be that the call sites would use get_gen(0/1) instead of young/old_gen. This is clearly the most difficult part of removing the generation array and also this is where we saw a few bugs in the last version. According to our tests it should be fine now. :)

I think having the splitting isolated might have made comparison checking easier, at least for me, because I’d know there weren’t supposed to be any functional differences.  I wonder if the diff might have been simpler with collect_generation after do_collection.  But this is all water under the bridge.

>> ------------------------------------------------------------------------------
>> src/share/vm/memory/genCollectedHeap.cpp
>>  574 void GenCollectedHeap::
>>  575 gen_process_roots(int level,
>> 
>> I think the proposed changes maintain the previous behavior, as
>> desired.  However ...
>> 
>> This is probably something for a future (but soon, I hope) cleanup.
>> 
>> […]
>> I'm ok with the proposed change as is, as part of that change set.
>> But please make sure there's a follow-on cleanup RFE.
> 
> I absolutely agree and I assume there are other simplifications and cleanups that will be apparent once the rest of the cleanups goes in as well. I copied your description above into an RFE to handle this issue.
> 
> https://bugs.openjdk.java.net/browse/JDK-8072809

Thank you.