RFR: 8075635 - Remove GenerationSpec array

[Mikael Gerdin]

Hi Jesper,

On 2015-03-24 17:27, Jesper Wilhelmsson wrote:
> Hi Kim,
>
> Thanks for reviewing!
>
> A new webrev is available here:
> http://cr.openjdk.java.net/~jwilhelm/8075635/webrev.01/hotspot/

Why are young_gen_spec() and old_gen_spec() virtual in GenCollectorPolicy?

As a possible follow up Generation::spec() only has a few callers, all 
of which are interested in the initial_size of the Generation, if that 
value could be cached in the Generation then the not particularly pretty 
Generation::spec() can go away. Or perhaps your end game is the end of 
GenerationSpec alltogether?

/Mikael

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