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

[Jesper Wilhelmsson]

Kim Barrett skrev den 10/2/15 23:56:
> 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.

Yes. Apparently awk (on Mac?) has a hard coded limitation of 50 patterns, so 
webrev fails to process large files... I have installed gawk now and built a new 
webrev to replace the old one. (same link)
/Jesper

>
> 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.
>
>