RFR: 8183542: Factor out serial GC specific code from GenCollectedHeap into its own subclass

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8183542: Factor out serial GC specific code from GenCollectedHeap into its own subclass

Roman Kennke-6
This gives the serial GC its own (Gen)CollectedHeap subclass. There is
not much serial specific code in GCH, but it seems odd to report it as
'serial' while its subclasses override it as CMS or Parallel.

Tested successfully with hotspot_gc.

http://cr.openjdk.java.net/~rkennke/8183542/webrev.00/ 
<http://cr.openjdk.java.net/%7Erkennke/8183542/webrev.00/>

Opinions? Ok to go in?

Thanks, Roman

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8183542: Factor out serial GC specific code from GenCollectedHeap into its own subclass

Kim Barrett
> On Oct 13, 2017, at 10:25 AM, Roman Kennke <[hidden email]> wrote:
>
> This gives the serial GC its own (Gen)CollectedHeap subclass. There is not much serial specific code in GCH, but it seems odd to report it as 'serial' while its subclasses override it as CMS or Parallel.
>
> Tested successfully with hotspot_gc.
>
> http://cr.openjdk.java.net/~rkennke/8183542/webrev.00/ <http://cr.openjdk.java.net/%7Erkennke/8183542/webrev.00/>
>
> Opinions? Ok to go in?
>
> Thanks, Roman

I have a deeply ingrained aversion to classes which are used as both
base classes and as concrete (directly instantiated) classes.  So I
approve of this change.  Just a few minor and style comments.

------------------------------------------------------------------------------
src/hotspot/share/gc/shared/collectedHeap.hpp
  82 // CollectedHeap
  83 //   SerialHeap
  84 //   G1CollectedHeap
  85 //   ParallelScavengeHeap
  86 //   CMSHeap

Isn't this really

// CollectedHeap
//   GenCollectedHeap
//     SerialHeap
//     CMSHeap
//   G1CollectedHeap
//   ParallelScavengeHeap

Note that the RFR message indicates Parallel derives its heap from
GCH, but it doesn't.  Not that it invalidates the reasons for making
this change.

------------------------------------------------------------------------------
src/hotspot/share/gc/shared/genCollectedHeap.hpp
 164   CollectorPolicy* collector_policy() const { return gen_policy(); }
 167   AdaptiveSizePolicy* size_policy() {

Why were the virtual qualifiers removed from these definitions?
Hotspot style seems to tend toward being explicit about virtual in
derived classes too (though it's far from universal). (Maybe someday
we'll be able to use C++11 override specifiers.) These are declared in
CollectedHeap.

Hm, this seems to have been done for a whole lot more functions.  And
yet, print_gc_threads_on and gc_threads_do weren't changed that way,
even though immediately bracketed by declarations that were changed.

I'm going to say no, don't do that.

------------------------------------------------------------------------------
src/hotspot/share/gc/serial/serialHeap.hpp

As mentioned above, common style is (I think) to be explicit about
virtual in override declarations.

------------------------------------------------------------------------------

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8183542: Factor out serial GC specific code from GenCollectedHeap into its own subclass

Roman Kennke-6
Hi Kim,

thanks for reviewing!

>> On Oct 13, 2017, at 10:25 AM, Roman Kennke <[hidden email]> wrote:
>>
>> This gives the serial GC its own (Gen)CollectedHeap subclass. There is not much serial specific code in GCH, but it seems odd to report it as 'serial' while its subclasses override it as CMS or Parallel.
>>
>> Tested successfully with hotspot_gc.
>>
>> http://cr.openjdk.java.net/~rkennke/8183542/webrev.00/ <http://cr.openjdk.java.net/%7Erkennke/8183542/webrev.00/>
>>
>> Opinions? Ok to go in?
>>
>> Thanks, Roman
> I have a deeply ingrained aversion to classes which are used as both
> base classes and as concrete (directly instantiated) classes.  So I
> approve of this change.  Just a few minor and style comments.
>
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/shared/collectedHeap.hpp
>    82 // CollectedHeap
>    83 //   SerialHeap
>    84 //   G1CollectedHeap
>    85 //   ParallelScavengeHeap
>    86 //   CMSHeap
>
> Isn't this really
>
> // CollectedHeap
> //   GenCollectedHeap
> //     SerialHeap
> //     CMSHeap
> //   G1CollectedHeap
> //   ParallelScavengeHeap
Ok, I fixed that.

> ------------------------------------------------------------------------------
> src/hotspot/share/gc/shared/genCollectedHeap.hpp
>   164   CollectorPolicy* collector_policy() const { return gen_policy(); }
>   167   AdaptiveSizePolicy* size_policy() {
>
> Why were the virtual qualifiers removed from these definitions?
> Hotspot style seems to tend toward being explicit about virtual in
> derived classes too (though it's far from universal). (Maybe someday
> we'll be able to use C++11 override specifiers.) These are declared in
> CollectedHeap.
>
> Hm, this seems to have been done for a whole lot more functions.  And
> yet, print_gc_threads_on and gc_threads_do weren't changed that way,
> even though immediately bracketed by declarations that were changed.
>
> I'm going to say no, don't do that.
>
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/serial/serialHeap.hpp
>
> As mentioned above, common style is (I think) to be explicit about
> virtual in override declarations.
>
> ------------------------------------------------------------------------------
>
Ok, I added back all the virtual qualifiers.

Differential webrev:
http://cr.openjdk.java.net/~rkennke/8183542/webrev.01.diff/ 
<http://cr.openjdk.java.net/%7Erkennke/8183542/webrev.01.diff/>

Full webrev:
http://cr.openjdk.java.net/~rkennke/8183542/webrev.01/ 
<http://cr.openjdk.java.net/%7Erkennke/8183542/webrev.01/>

Better now?

Thanks, Roman

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8183542: Factor out serial GC specific code from GenCollectedHeap into its own subclass

Kim Barrett
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8183542: Factor out serial GC specific code from GenCollectedHeap into its own subclass

Roman Kennke-6
Am 18.10.2017 um 20:41 schrieb Kim Barrett:

>> On Oct 18, 2017, at 8:08 AM, Roman Kennke <[hidden email]> wrote:
>> Differential webrev:
>> http://cr.openjdk.java.net/~rkennke/8183542/webrev.01.diff/ <http://cr.openjdk.java.net/%7Erkennke/8183542/webrev.01.diff/>
>>
>> Full webrev:
>> http://cr.openjdk.java.net/~rkennke/8183542/webrev.01/ <http://cr.openjdk.java.net/%7Erkennke/8183542/webrev.01/>
>>
>> Better now?
>>
>> Thanks, Roman
> Looks good.
>
Hi Kim,

thanks for the review.

I just fixed a bug caused by my similar CMSHeap extraction, and I think
I need to do the same thing for SerialHeap too:

https://bugs.openjdk.java.net/browse/JDK-8189373

This is the fix for the CMSHeap issue:

http://cr.openjdk.java.net/~rkennke/8189373/webrev.00/ 
<http://cr.openjdk.java.net/%7Erkennke/8189373/webrev.00/>

I'll do the same for SerialHeap once the above has been approved and
pushed, otherwise it'll be a mess. ;-)

Roman


Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8183542: Factor out serial GC specific code from GenCollectedHeap into its own subclass

Kim Barrett
> On Oct 18, 2017, at 4:04 PM, Roman Kennke <[hidden email]> wrote:
>
> Am 18.10.2017 um 20:41 schrieb Kim Barrett:
>>> On Oct 18, 2017, at 8:08 AM, Roman Kennke <[hidden email]> wrote:
>>> Differential webrev:
>>> http://cr.openjdk.java.net/~rkennke/8183542/webrev.01.diff/ <http://cr.openjdk.java.net/%7Erkennke/8183542/webrev.01.diff/>
>>>
>>> Full webrev:
>>> http://cr.openjdk.java.net/~rkennke/8183542/webrev.01/ <http://cr.openjdk.java.net/%7Erkennke/8183542/webrev.01/>
>>>
>>> Better now?
>>>
>>> Thanks, Roman
>> Looks good.
>>
> Hi Kim,
>
> thanks for the review.
>
> I just fixed a bug caused by my similar CMSHeap extraction, and I think I need to do the same thing for SerialHeap too:
>
> https://bugs.openjdk.java.net/browse/JDK-8189373
>
> This is the fix for the CMSHeap issue:
>
> http://cr.openjdk.java.net/~rkennke/8189373/webrev.00/ <http://cr.openjdk.java.net/%7Erkennke/8189373/webrev.00/>
>
> I'll do the same for SerialHeap once the above has been approved and pushed, otherwise it'll be a mess. ;-)
>
> Roman

The SA strikes again!  Yes, it looks like the same thing should be done for SerialHeap.
I’m going to leave the review of 8189373 to others who have more clue about the SA.

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8183542: Factor out serial GC specific code from GenCollectedHeap into its own subclass

Roman Kennke-6
Am 18.10.2017 um 22:41 schrieb Kim Barrett:

>> On Oct 18, 2017, at 4:04 PM, Roman Kennke <[hidden email]> wrote:
>>
>> Am 18.10.2017 um 20:41 schrieb Kim Barrett:
>>>> On Oct 18, 2017, at 8:08 AM, Roman Kennke <[hidden email]> wrote:
>>>> Differential webrev:
>>>> http://cr.openjdk.java.net/~rkennke/8183542/webrev.01.diff/ <http://cr.openjdk.java.net/%7Erkennke/8183542/webrev.01.diff/>
>>>>
>>>> Full webrev:
>>>> http://cr.openjdk.java.net/~rkennke/8183542/webrev.01/ <http://cr.openjdk.java.net/%7Erkennke/8183542/webrev.01/>
>>>>
>>>> Better now?
>>>>
>>>> Thanks, Roman
>>> Looks good.
>>>
>> Hi Kim,
>>
>> thanks for the review.
>>
>> I just fixed a bug caused by my similar CMSHeap extraction, and I think I need to do the same thing for SerialHeap too:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8189373
>>
>> This is the fix for the CMSHeap issue:
>>
>> http://cr.openjdk.java.net/~rkennke/8189373/webrev.00/ <http://cr.openjdk.java.net/%7Erkennke/8189373/webrev.00/>
>>
>> I'll do the same for SerialHeap once the above has been approved and pushed, otherwise it'll be a mess. ;-)
>>
>> Roman
> The SA strikes again!  Yes, it looks like the same thing should be done for SerialHeap.
> I’m going to leave the review of 8189373 to others who have more clue about the SA.
>
Okidoki, so here comes the SerialGC with SA boilerplate:

Differential:
http://cr.openjdk.java.net/~rkennke/8183542/webrev.02.diff/ 
<http://cr.openjdk.java.net/%7Erkennke/8183542/webrev.02.diff/>

Full:
http://cr.openjdk.java.net/~rkennke/8183542/webrev.02/ 
<http://cr.openjdk.java.net/%7Erkennke/8183542/webrev.02/>

This builds on top of the patch for
https://bugs.openjdk.java.net/browse/JDK-8189373 which should land in
the repo shortly, and implements the same thing for SerialHeap. It also
passes the test that failed in the mentioned bug report (with
-XX:+UseSerialGC).

Can I get reviews (for the changed/added stuff) again?

Thanks, Roman

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8183542: Factor out serial GC specific code from GenCollectedHeap into its own subclass

Jini George
Hi Roman,

The SA portion looks good to me.

Thank you,
Jini (not a Reviewer).

On 10/19/2017 10:09 PM, Roman Kennke wrote:

> Am 18.10.2017 um 22:41 schrieb Kim Barrett:
>>> On Oct 18, 2017, at 4:04 PM, Roman Kennke <[hidden email]> wrote:
>>>
>>> Am 18.10.2017 um 20:41 schrieb Kim Barrett:
>>>>> On Oct 18, 2017, at 8:08 AM, Roman Kennke <[hidden email]> wrote:
>>>>> Differential webrev:
>>>>> http://cr.openjdk.java.net/~rkennke/8183542/webrev.01.diff/ 
>>>>> <http://cr.openjdk.java.net/%7Erkennke/8183542/webrev.01.diff/>
>>>>>
>>>>> Full webrev:
>>>>> http://cr.openjdk.java.net/~rkennke/8183542/webrev.01/ 
>>>>> <http://cr.openjdk.java.net/%7Erkennke/8183542/webrev.01/>
>>>>>
>>>>> Better now?
>>>>>
>>>>> Thanks, Roman
>>>> Looks good.
>>>>
>>> Hi Kim,
>>>
>>> thanks for the review.
>>>
>>> I just fixed a bug caused by my similar CMSHeap extraction, and I
>>> think I need to do the same thing for SerialHeap too:
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8189373
>>>
>>> This is the fix for the CMSHeap issue:
>>>
>>> http://cr.openjdk.java.net/~rkennke/8189373/webrev.00/ 
>>> <http://cr.openjdk.java.net/%7Erkennke/8189373/webrev.00/>
>>>
>>> I'll do the same for SerialHeap once the above has been approved and
>>> pushed, otherwise it'll be a mess. ;-)
>>>
>>> Roman
>> The SA strikes again!  Yes, it looks like the same thing should be
>> done for SerialHeap.
>> I’m going to leave the review of 8189373 to others who have more clue
>> about the SA.
>>
> Okidoki, so here comes the SerialGC with SA boilerplate:
>
> Differential:
> http://cr.openjdk.java.net/~rkennke/8183542/webrev.02.diff/ 
> <http://cr.openjdk.java.net/%7Erkennke/8183542/webrev.02.diff/>
>
> Full:
> http://cr.openjdk.java.net/~rkennke/8183542/webrev.02/ 
> <http://cr.openjdk.java.net/%7Erkennke/8183542/webrev.02/>
>
> This builds on top of the patch for
> https://bugs.openjdk.java.net/browse/JDK-8189373 which should land in
> the repo shortly, and implements the same thing for SerialHeap. It also
> passes the test that failed in the mentioned bug report (with
> -XX:+UseSerialGC).
>
> Can I get reviews (for the changed/added stuff) again?
>
> Thanks, Roman
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8183542: Factor out serial GC specific code from GenCollectedHeap into its own subclass

Kim Barrett
In reply to this post by Roman Kennke-6
> On Oct 19, 2017, at 12:39 PM, Roman Kennke <[hidden email]> wrote:
>
> Am 18.10.2017 um 22:41 schrieb Kim Barrett:
>>> On Oct 18, 2017, at 4:04 PM, Roman Kennke <[hidden email]> wrote:
>>>
>>> Am 18.10.2017 um 20:41 schrieb Kim Barrett:
>>>>> On Oct 18, 2017, at 8:08 AM, Roman Kennke <[hidden email]> wrote:
>>>>> Differential webrev:
>>>>> http://cr.openjdk.java.net/~rkennke/8183542/webrev.01.diff/ <http://cr.openjdk.java.net/%7Erkennke/8183542/webrev.01.diff/>
>>>>>
>>>>> Full webrev:
>>>>> http://cr.openjdk.java.net/~rkennke/8183542/webrev.01/ <http://cr.openjdk.java.net/%7Erkennke/8183542/webrev.01/>
>>>>>
>>>>> Better now?
>>>>>
>>>>> Thanks, Roman
>>>> Looks good.
>>>>
>>> Hi Kim,
>>>
>>> thanks for the review.
>>>
>>> I just fixed a bug caused by my similar CMSHeap extraction, and I think I need to do the same thing for SerialHeap too:
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8189373
>>>
>>> This is the fix for the CMSHeap issue:
>>>
>>> http://cr.openjdk.java.net/~rkennke/8189373/webrev.00/ <http://cr.openjdk.java.net/%7Erkennke/8189373/webrev.00/>
>>>
>>> I'll do the same for SerialHeap once the above has been approved and pushed, otherwise it'll be a mess. ;-)
>>>
>>> Roman
>> The SA strikes again!  Yes, it looks like the same thing should be done for SerialHeap.
>> I’m going to leave the review of 8189373 to others who have more clue about the SA.
>>
> Okidoki, so here comes the SerialGC with SA boilerplate:
>
> Differential:
> http://cr.openjdk.java.net/~rkennke/8183542/webrev.02.diff/ <http://cr.openjdk.java.net/%7Erkennke/8183542/webrev.02.diff/>
>
> Full:
> http://cr.openjdk.java.net/~rkennke/8183542/webrev.02/ <http://cr.openjdk.java.net/%7Erkennke/8183542/webrev.02/>
>
> This builds on top of the patch for https://bugs.openjdk.java.net/browse/JDK-8189373 which should land in the repo shortly, and implements the same thing for SerialHeap. It also passes the test that failed in the mentioned bug report (with -XX:+UseSerialGC).
>
> Can I get reviews (for the changed/added stuff) again?
>
> Thanks, Roman

Looks good to me, though I don’t know much about the SA.
It at least looks consistent with the changes for JDK-8189373.

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8183542: Factor out serial GC specific code from GenCollectedHeap into its own subclass

Roman Kennke-6
Hi Kim, hi Jini,

thank you both for your reviews!

I think I need a sponsor now. The final webrev (same as before plus
Reviewed-by line):
http://cr.openjdk.java.net/~rkennke/8183542/webrev.03/ 
<http://cr.openjdk.java.net/%7Erkennke/8183542/webrev.03/>

Thanks, Roman

>> On Oct 19, 2017, at 12:39 PM, Roman Kennke <[hidden email]> wrote:
>>
>> Am 18.10.2017 um 22:41 schrieb Kim Barrett:
>>>> On Oct 18, 2017, at 4:04 PM, Roman Kennke <[hidden email]> wrote:
>>>>
>>>> Am 18.10.2017 um 20:41 schrieb Kim Barrett:
>>>>>> On Oct 18, 2017, at 8:08 AM, Roman Kennke <[hidden email]> wrote:
>>>>>> Differential webrev:
>>>>>> http://cr.openjdk.java.net/~rkennke/8183542/webrev.01.diff/ <http://cr.openjdk.java.net/%7Erkennke/8183542/webrev.01.diff/>
>>>>>>
>>>>>> Full webrev:
>>>>>> http://cr.openjdk.java.net/~rkennke/8183542/webrev.01/ <http://cr.openjdk.java.net/%7Erkennke/8183542/webrev.01/>
>>>>>>
>>>>>> Better now?
>>>>>>
>>>>>> Thanks, Roman
>>>>> Looks good.
>>>>>
>>>> Hi Kim,
>>>>
>>>> thanks for the review.
>>>>
>>>> I just fixed a bug caused by my similar CMSHeap extraction, and I think I need to do the same thing for SerialHeap too:
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8189373
>>>>
>>>> This is the fix for the CMSHeap issue:
>>>>
>>>> http://cr.openjdk.java.net/~rkennke/8189373/webrev.00/ <http://cr.openjdk.java.net/%7Erkennke/8189373/webrev.00/>
>>>>
>>>> I'll do the same for SerialHeap once the above has been approved and pushed, otherwise it'll be a mess. ;-)
>>>>
>>>> Roman
>>> The SA strikes again!  Yes, it looks like the same thing should be done for SerialHeap.
>>> I’m going to leave the review of 8189373 to others who have more clue about the SA.
>>>
>> Okidoki, so here comes the SerialGC with SA boilerplate:
>>
>> Differential:
>> http://cr.openjdk.java.net/~rkennke/8183542/webrev.02.diff/ <http://cr.openjdk.java.net/%7Erkennke/8183542/webrev.02.diff/>
>>
>> Full:
>> http://cr.openjdk.java.net/~rkennke/8183542/webrev.02/ <http://cr.openjdk.java.net/%7Erkennke/8183542/webrev.02/>
>>
>> This builds on top of the patch for https://bugs.openjdk.java.net/browse/JDK-8189373 which should land in the repo shortly, and implements the same thing for SerialHeap. It also passes the test that failed in the mentioned bug report (with -XX:+UseSerialGC).
>>
>> Can I get reviews (for the changed/added stuff) again?
>>
>> Thanks, Roman
> Looks good to me, though I don’t know much about the SA.
> It at least looks consistent with the changes for JDK-8189373.
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8183542: Factor out serial GC specific code from GenCollectedHeap into its own subclass

Kim Barrett
> On Oct 25, 2017, at 4:07 AM, Roman Kennke <[hidden email]> wrote:
>
> Hi Kim, hi Jini,
>
> thank you both for your reviews!
>
> I think I need a sponsor now. The final webrev (same as before plus Reviewed-by line):
> http://cr.openjdk.java.net/~rkennke/8183542/webrev.03/ <http://cr.openjdk.java.net/%7Erkennke/8183542/webrev.03/>

Sorry, I overlooked the sponsorship request.  I’ll take care of it, but not today.

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8183542: Factor out serial GC specific code from GenCollectedHeap into its own subclass

Roman Kennke-6
Am 02.11.2017 um 04:55 schrieb Kim Barrett:

>> On Oct 25, 2017, at 4:07 AM, Roman Kennke <[hidden email]> wrote:
>>
>> Hi Kim, hi Jini,
>>
>> thank you both for your reviews!
>>
>> I think I need a sponsor now. The final webrev (same as before plus Reviewed-by line):
>> http://cr.openjdk.java.net/~rkennke/8183542/webrev.03/ <http://cr.openjdk.java.net/%7Erkennke/8183542/webrev.03/>
> Sorry, I overlooked the sponsorship request.  I’ll take care of it, but not today.
>
Thanks, Kim!

Roman

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8183542: Factor out serial GC specific code from GenCollectedHeap into its own subclass

Roman Kennke-6
In reply to this post by Kim Barrett
Am 02.11.2017 um 04:55 schrieb Kim Barrett:

>> On Oct 25, 2017, at 4:07 AM, Roman Kennke <[hidden email]> wrote:
>>
>> Hi Kim, hi Jini,
>>
>> thank you both for your reviews!
>>
>> I think I need a sponsor now. The final webrev (same as before plus Reviewed-by line):
>> http://cr.openjdk.java.net/~rkennke/8183542/webrev.03/ <http://cr.openjdk.java.net/%7Erkennke/8183542/webrev.03/>
> Sorry, I overlooked the sponsorship request.  I’ll take care of it, but not today.
>
Ping? :-)