RFR: JDK-8076267 - Remove n_gens()

[Jesper Wilhelmsson]

Hi Kim,

Thanks for reviewing!

See comments below. A new webrev is available here:

http://cr.openjdk.java.net/~jwilhelm/8076267/webrev.01/

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.

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.

>
> ------------------------------------------------------------------------------
> src/share/vm/memory/cardTableRS.cpp
>    47   int max_gens = 2;
> ...
>    53   for (int i = 0; i < max_gens + 1; i++) {
>
> These should be unsigned int or size_t types, rather than (signed)
> int.

Fixed.

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

Thanks,
/Jesper