RFR: 8198528: Move GenerationSpecs from GenCollectorPolicy to GenCollectedHeap

[Stefan Karlsson]

On 2018-02-22 06:08, Kim Barrett wrote:
>> On Feb 21, 2018, at 4:27 PM, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:
>>
>> Hi all,
>>
>> Please review this patch to move _young_gen_spec and _old_gen_spec out from GenCollectorPolicy into GenCollectedHeap.
>>
>>   http://cr.openjdk.java.net/~stefank/8198528/webrev.01/
>>   https://bugs.openjdk.java.net/browse/JDK-8198528
>>
>> This is a step towards the removal of CollectorPolicy:
>>
>>   https://bugs.openjdk.java.net/browse/JDK-8198505
>>
>> Thanks,
>> StefanK
> 
> Generally looks good.  Just a few minor comments.

Thanks for the review!

I'll fix the two last nits. However, I don't intend to change the style 
unless I get push-back from other maintainers of the GC code. And if we 
decide later to get rid of this style, then it's an easy cleanup to do. 
I've given my take on it below, if anyone is interested in that 
discussion. :)

> 
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/cms/cmsHeap.cpp
>    66 CMSHeap::CMSHeap(GenCollectorPolicy *policy) :
> 
> The new formatting of the initializer list is oddly indented.  I
> suppose it's to make the initializer list visually distinct from the
> body code.  I prefer accomplishing that by putting the open-brace of
> the body on its own line between the initializer list and the body
> code, rather than unusual indentation of the initializer list.
> 
> Similarly for the SerialHeap constructor.

This is a debate about styles, and I know it's not going to get resolved 
in this RFR. But to give my take on this.

I think it's not uncommon to use two indentation levels when breaking 
lines, and this style is adopted by a few of us in the GC team.

If only one indentation level is used there are different ways code will 
end up looking like. For example:

1)
MyClass::MyClass() :
   _member1(),
   _member2() {
   _member1.do();
   _member2.do();
}

2)
MyClass::MyClass() :
   _member1(),
   _member2()
{
   _member1.do();
   _member2.do();
}

3)
MyClass::MyClass() :
   _member1(),
   _member2() {
   // Commment to visually separate the two lines
   _member1.do();
   _member2.do();
}

If we use one indentation level, then its a toss whether people use 1, 
2, or 3. And I find it annoying everytime I see (1).

With two indentation levels, this isn't a problem anymore:

MyClass::MyClass() :
     _member1(),
     _member2() {
   _member1.do();
   _member2.do();
}

MyClass::MyClass() :
     _member1(),
     _member2() {
{
   _member1.do();
   _member2.do();
}


MyClass::MyClass() :
     _member1(),
     _member2() {
   // Comment to visually separate the two lines
   _member1.do();
   _member2.do();
}

It's always easy to see where the initializer list ends.

We have the same situation with if statements.

The following is easier to read (IMHO) ...:
if (cond1 &&
     cond2 &&
     cond3) {
   cond_met();
}

// ... than this:
if (cond1 &&
   cond2 &&
   cond3) {
   cond_met() {
}

or with while loops:
while(cond1 &&
     cond2 &&
     cond3) {
   cond_met();
}

vs:
while(cond1 &&
   cond2 &&
   cond3) {
   cond_met();
}

though most loops would end up like:
while(cond1 &&
       cond2 &&
       cond3) {
   cond_met();
}

(I assume condX is long enough that line breaks are warranted)

> 
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/shared/genCollectedHeap.cpp
> 123 void GenCollectedHeap::initialize_generations(Generation::Name young,
> 
> Shouldn't this function just be removed now?

Yes. Thanks for spotting that.

> 
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/shared/genCollectedHeap.cpp
>   187 GenerationSpec* GenCollectedHeap::young_gen_spec() const {
>   188   assert(_young_gen_spec != NULL, "_young_gen_spec should have been initialized");
>   189   return _young_gen_spec;
>   190 }
>   191
>   192 GenerationSpec* GenCollectedHeap::old_gen_spec() const {
>   193   assert(_old_gen_spec != NULL, "_old_gen_spec should have been initialized");
>   194   return _old_gen_spec;
>   195 }
> 
> The assertions are no longer useful, and indeed somewhat confusing,
> since those members are now initialized in the constructor's
> initializer list.

Yes. I'll remove those.

Thanks,
StefanK

> 
> ------------------------------------------------------------------------------
>