RFR: 8075635 - Remove GenerationSpec array

[Jesper Wilhelmsson]

Hi Mikael,

I guess the simple answer is that young/old_gen_spec() was virtual because their 
predecessor "generations()" was. There is no reason for them to be virtual. I 
have removed that now.

Latest webrev: http://cr.openjdk.java.net/~jwilhelm/8075635/webrev.02

(Only change was to remove virtual from these two methods.)

I have added your comments about Generation::spec() to my list of future work.

Thanks,
/Jesper


Mikael Gerdin skrev den 25/3/15 14:46:
> 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