RFR: 8075635 - Remove GenerationSpec array

[Jesper Wilhelmsson]

Hi Kim,

Thanks for reviewing!

A new webrev is available here:
http://cr.openjdk.java.net/~jwilhelm/8075635/webrev.01/hotspot/

Latest increment:
http://cr.openjdk.java.net/~jwilhelm/8075635/webrev.01/hotspot_incremental/

Comments inline.

Kim Barrett skrev den 23/3/15 22:59:
> On Mar 23, 2015, at 1:10 PM, Jesper Wilhelmsson <jesper.wilhelmsson at oracle.com> wrote:
>>
>> Hi,
>>
>> Please review another follow up after the generation array removal. In this change the array of generation specifications is reduced to two variables in the same way as was done for the generations.
>>
>> The reference to the genSpecs that was previously duplicated in GenCollectedHeap is now only available through the collector policy class.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8075635
>> Webrev: http://cr.openjdk.java.net/~jwilhelm/8075635/webrev.00
>
> ------------------------------------------------------------------------------
> src/share/vm/gc_implementation/concurrentMarkSweep/cmsCollectorPolicy.cpp
> src/share/vm/memory/collectorPolicy.hpp
> src/share/vm/memory/generationSpec.hpp
>
> Needs copyright update.

Fixed.

> ------------------------------------------------------------------------------
> agent/src/share/classes/sun/jvm/hotspot/memory/GenCollectedHeap.java
>   131     if (level == 0) {
>   132       return (GenerationSpec)
>   133               VMObjectFactory.newObject(GenerationSpec.class,
>   134                       youngGenSpecField.getAddress());
>   135     } else {
>   136       return (GenerationSpec)
>   137               VMObjectFactory.newObject(GenerationSpec.class,
>   138                       oldGenSpecField.getAddress());
>   139     }
>
> This is effectively assuming that nGens() == 2, but the earlier
> assertions in this function don't reflect that assumption.  I realize
> there will probably be further cleanup in this area later, but I'd
> like things to be in a consistent state at each step in these
> incremental changes.

I changed the existing assert to look for 0 or 1 rather than using nGens(). This 
is the same approach as we used in the last change that changed the assert in a 
method a few lines above this one.

>
> Similar issue here:
> src/share/vm/memory/generation.cpp
>    67   return level() == 0 ? gch->gen_policy()->young_gen_spec() : gch->gen_policy()->old_gen_spec();

Same change here.

>
> ------------------------------------------------------------------------------
> src/share/vm/memory/genCollectedHeap.cpp
>   151   size_t total_reserved = young_spec->max_size() + old_spec->max_size();
>   152   if (total_reserved < young_spec->max_size() ||
>   153       total_reserved < old_spec->max_size()) {
>
> Not that it's all that important, but either of those comparisons
> alone should be sufficient to test for overflow.  It might also be
> nice to leave a comment about this being a test for overflow.

Yes, you are right. You are most likely the first one to give this code enough 
thought to realize this. Good catch! I cleaned up this code a bit in the new 
version.

> ------------------------------------------------------------------------------
> src/share/vm/memory/generationSpec.hpp
>    42   GenerationSpec(Generation::Name name, size_t init_size, size_t max_size, size_t alignment) {
>    43     _name = name;
>    44     _init_size = align_size_up(init_size, alignment);
>    45     _max_size = align_size_up(max_size, alignment);
>    46   }
>
> I like that you've eliminated the separate post-construction init_size
> and max_size alignment step and are instead doing that in the
> constructor.
>
> Is there anything that ensures init_size <= max_size?  Should there be
> an assertion to that effect here?

This is verified in the collector policy code where initial and max sizes are 
set up. I wouldn't object to having an assert here if you feel strongly about 
it. On the other hand we have a quite extensive set off asserts in 
GenCollectorPolicy::assert_flags() and GenCollectorPolicy::assert_size_info() to 
make sure this stuff is correct already.

>
> Also consider initializing members in ctor-initializer rather than
> assignments in the constructor body.

Fixed.

Thanks!
/Jesper