RFR: JDK-8076267 - Remove n_gens()

[Kim Barrett]

On Mar 31, 2015, at 2:30 PM, Jesper Wilhelmsson <jesper.wilhelmsson at oracle.com> wrote:
> 
> Hi Kim,
> 
> Thanks for reviewing!
> 
> See comments below. A new webrev is available here:
> 
> http://cr.openjdk.java.net/~jwilhelm/8076267/webrev.01/

Looks good.

Some replies inline below.

> Kim Barrett skrev den 31/3/15 19:48:
>> On Mar 31, 2015, at 9:14 AM, Jesper Wilhelmsson <jesper.wilhelmsson at oracle.com> wrote:
>>> 
>>> Hi,
>>> 
>>> Please review this next cleanup in the backwaters of the generation array removal. This change removes the n_gens() method and the max_gens variable from GenCollectedHeap.
>>> 
>>> Next in line in the cleanup queue is a change to remove the levels concept. Mostly all places where we today pass a "level" in the GC code can be removed, or be made more explicit by introducing two constants, YOUNG and OLD. Removing n_gens() and the levels touches upon the same code in many places and we could skip one, in some places somewhat ugly, intermediate version by doing both changes at the same time. The levels removal is a much larger and trickier change though, so to make it easier to review I choose to split these into two changes. Please keep this in mind when reviewing the current change.
>>> 
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8076267
>>> Webrev: http://cr.openjdk.java.net/~jwilhelm/8076267/webrev.00/
>> 
>> I think YOUNG and OLD might be a little overly generic.  And I’m not sure there’s a need to make them especially short either.
> 
> I agree. I didn't have the level-patch fresh in mind when writing. What I actually have in the patch right now is an enum in the Generation class:
> 
>  enum Type {
>    Young,
>    Old
>  };
> 
> When using it the class name will be present as well as the constant giving the names context and meaning, Generation::Young, and Generation::Old.

Yes, that’s good.

> This also means that in this patch I change the type of any function that still needs to take a level as an argument from int to Generation::Type. So there should not be any risk of missing any stray 0's or 1's in the code, they will all be replaced with this new type. (Some of them will actually be removed since we don't need to pass around the level any more.)
> 
> I'd prefer not to add this new type to this change however since this is one of the changes that makes the level cleanup large and tricky.

OK.

>> ------------------------------------------------------------------------------
>> src/share/vm/memory/genCollectedHeap.cpp
>>  433     bool complete = full && (max_level == 1);
>> 
>> I'd really like to have a name for that literal "1".  Or at least a
>> comment or something, if this code will be changing further with the
>> cleanup of the level concept.
>> 
>> Similarly here:
>>  505     complete = complete || (max_level_collected == 1);
>> 
>> Actually, there's a whole bunch of literal "1"s.  I'm worried that in
>> the future level cleanup it will be very easy to miss some of these.
>> So I think it would be better to introduce the level constants now,
>> even if the rest of the level cleanup isn't happening until later.
> 
> As noted above I'd prefer not to add the new type in this change. I have added comments like /* young */ and /* old */ along with the 1's and 0's I touched in this change and a few more. But there are plenty of these magic level constants in the code (and all of them will disappear with the next change :)).

OK.