Re: RFR: 8191564: Refactor GC related servicability code into GC specific subclasses

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

Re: RFR: 8191564: Refactor GC related servicability code into GC specific subclasses

Roman Kennke-6
Hi Erik,

Thanks for your review!

All of the things that you mentioned should be addressed in the
following changes:

Differential:
http://cr.openjdk.java.net/~rkennke/8191564/webrev.05.diff/
Full:
http://cr.openjdk.java.net/~rkennke/8191564/webrev.05/

Also, Erik D (H) was so kind to contribute an additional testcase, which
is also included in the above patch.

Ok now?

Also, I need a review from serviceability-dev too!

Thanks,
Roman


> 1) The code uses the "mgr" short name for "manager" in a bunch of names.
> I would be happy if we could write out the whole thing instead of the
> abbreviation.
> 2) It would be great if a generation would have a pointer to its
> manager, so we do not have to pass around the manager where we already
> pass around the generation (such as GenCollectedHeap::collect_generation).
> The generation could be created first, then the pools, then the
> managers, then do something like generation->set_memory_manager(x).
> 3) In cmsHeap.cpp:54: maxSize should preferably not use camel case.
>
> Otherwise I think it looks good.
>
> Thanks,
> /Erik
>
> On 2017-11-27 10:30, Roman Kennke wrote:
>> Erik implemented a few more refactorings and touch-ups, and here is
>> our final (pending reviews) webrev:
>>
>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.04/
>>
>> Compared to webrev.02, it improves the handling of gc-end-message,
>> avoids dragging the GCMemoryManager through Generation and a few
>> little related refactorings.
>>
>> Ok to push now?
>>
>> Roman
>>
>>> After a few more discussions with Erik I made some more changes.
>>>
>>> MemoryService is now unaware of the number and meaning of GC memory
>>> managers (minor vs major). This should be better for GCs that don't
>>> make that distinction and/or use more different GCs (e.g. minor,
>>> intermediate, full).
>>>
>>> This means that I needed to add some abstractions:
>>> - GCMemoryManager now has gc_end_message() which is used by
>>> GCNotifier::pushNotification().
>>> - gc_begin() and gc_end() methods in MemoryService now accept a
>>> GCMemoryManager* instead of bull full_gc
>>> - Same for TraceMemoryManagerStats
>>> - Generation now knows about the corresponding GCMemoryManager
>>>
>>> Please review the full change:
>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.02/
>>>
>>> Thanks, Roman
>>>
>>>
>>>> I had some off-band discussions with Erik Helin and re-did most of
>>>> the changeset:
>>>> - The GC interface now resides in CollectedHeap, specifically the
>>>> two methods memory_managers() and memory_pools(), and is implemented
>>>> in the various concrete subclasses.
>>>> - Both methods return (by value) a GrowableArray<GCMemoryManager*>
>>>> and GrowableArray<MemoryPool> respectively. Returning a
>>>> stack-allocated GrowableArray seemed least complicated (avoid
>>>> explicit cleanup of short-lived array object), and most
>>>> future-proof, e.g. currently there is an implicit expectation to get
>>>> 2 GCMemoryManagers, even though some GCs don't necessarily have two.
>>>> The API allows for easy extension of the situation.
>>>>
>>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.01/
>>>>
>>>> I think this requires reviews from both the GC and Serviceability team.
>>>>
>>>> Roman
>>>>
>>>>
>>>>> Currently, there's lots of GC specific code sprinkled over
>>>>> src/hotspot/share/services. This change introduces a GC interface
>>>>> for that, and moves all GC specific code to their respective
>>>>> src/hotspot/share/gc directory.
>>>>>
>>>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.00/ 
>>>>> <http://cr.openjdk.java.net/%7Erkennke/8191564/webrev.00/>
>>>>>
>>>>> Testing: hotspot_gc and hotspot_serviceability, none showed
>>>>> regressions
>>>>>
>>>>> Built minimal and server without regressions
>>>>>
>>>>> What do you think?
>>>>>
>>>>> Roman
>>>>>
>>>>>
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191564: Refactor GC related servicability code into GC specific subclasses

Erik Österlund-2
Hi Roman,

This looks better now. Nice job.
I wonder though, is it possible to extract the creation of managers and
pools to a separate function for each collected heap?
Now I see managers are created in e.g. CMS constructor, and the pools
are created in CMSHeap::initialize(). Perhaps initialize could call
initialize_serviceability() that sets up those things, so that they are
nicely separated. Unless of course there is a good reason why the
presence of managers is needed earlier than that in the bootstrapping.

Otherwise I think this looks good!

Thanks,
/Erik

On 2017-11-28 12:22, Roman Kennke wrote:

> Hi Erik,
>
> Thanks for your review!
>
> All of the things that you mentioned should be addressed in the
> following changes:
>
> Differential:
> http://cr.openjdk.java.net/~rkennke/8191564/webrev.05.diff/
> Full:
> http://cr.openjdk.java.net/~rkennke/8191564/webrev.05/
>
> Also, Erik D (H) was so kind to contribute an additional testcase,
> which is also included in the above patch.
>
> Ok now?
>
> Also, I need a review from serviceability-dev too!
>
> Thanks,
> Roman
>
>
>> 1) The code uses the "mgr" short name for "manager" in a bunch of
>> names. I would be happy if we could write out the whole thing instead
>> of the abbreviation.
>> 2) It would be great if a generation would have a pointer to its
>> manager, so we do not have to pass around the manager where we
>> already pass around the generation (such as
>> GenCollectedHeap::collect_generation).
>> The generation could be created first, then the pools, then the
>> managers, then do something like generation->set_memory_manager(x).
>> 3) In cmsHeap.cpp:54: maxSize should preferably not use camel case.
>>
>> Otherwise I think it looks good.
>>
>> Thanks,
>> /Erik
>>
>> On 2017-11-27 10:30, Roman Kennke wrote:
>>> Erik implemented a few more refactorings and touch-ups, and here is
>>> our final (pending reviews) webrev:
>>>
>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.04/
>>>
>>> Compared to webrev.02, it improves the handling of gc-end-message,
>>> avoids dragging the GCMemoryManager through Generation and a few
>>> little related refactorings.
>>>
>>> Ok to push now?
>>>
>>> Roman
>>>
>>>> After a few more discussions with Erik I made some more changes.
>>>>
>>>> MemoryService is now unaware of the number and meaning of GC memory
>>>> managers (minor vs major). This should be better for GCs that don't
>>>> make that distinction and/or use more different GCs (e.g. minor,
>>>> intermediate, full).
>>>>
>>>> This means that I needed to add some abstractions:
>>>> - GCMemoryManager now has gc_end_message() which is used by
>>>> GCNotifier::pushNotification().
>>>> - gc_begin() and gc_end() methods in MemoryService now accept a
>>>> GCMemoryManager* instead of bull full_gc
>>>> - Same for TraceMemoryManagerStats
>>>> - Generation now knows about the corresponding GCMemoryManager
>>>>
>>>> Please review the full change:
>>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.02/
>>>>
>>>> Thanks, Roman
>>>>
>>>>
>>>>> I had some off-band discussions with Erik Helin and re-did most of
>>>>> the changeset:
>>>>> - The GC interface now resides in CollectedHeap, specifically the
>>>>> two methods memory_managers() and memory_pools(), and is
>>>>> implemented in the various concrete subclasses.
>>>>> - Both methods return (by value) a GrowableArray<GCMemoryManager*>
>>>>> and GrowableArray<MemoryPool> respectively. Returning a
>>>>> stack-allocated GrowableArray seemed least complicated (avoid
>>>>> explicit cleanup of short-lived array object), and most
>>>>> future-proof, e.g. currently there is an implicit expectation to
>>>>> get 2 GCMemoryManagers, even though some GCs don't necessarily
>>>>> have two. The API allows for easy extension of the situation.
>>>>>
>>>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.01/
>>>>>
>>>>> I think this requires reviews from both the GC and Serviceability
>>>>> team.
>>>>>
>>>>> Roman
>>>>>
>>>>>
>>>>>> Currently, there's lots of GC specific code sprinkled over
>>>>>> src/hotspot/share/services. This change introduces a GC interface
>>>>>> for that, and moves all GC specific code to their respective
>>>>>> src/hotspot/share/gc directory.
>>>>>>
>>>>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.00/ 
>>>>>> <http://cr.openjdk.java.net/%7Erkennke/8191564/webrev.00/>
>>>>>>
>>>>>> Testing: hotspot_gc and hotspot_serviceability, none showed
>>>>>> regressions
>>>>>>
>>>>>> Built minimal and server without regressions
>>>>>>
>>>>>> What do you think?
>>>>>>
>>>>>> Roman
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191564: Refactor GC related servicability code into GC specific subclasses

Roman Kennke-6
Hi Erik,

thank you for reviewing!

You are right. I think this is a leftover from when we tried to pass the
GCMemoryManager* into the Generation constructor. The way it is done now
(installing the GCMmemoryManager* later through set_memory_manager()) we
can group all serviceability related set-up into
initialize_serviceability():

Differential:
http://cr.openjdk.java.net/~rkennke/8191564/webrev.06.diff/
Full:
http://cr.openjdk.java.net/~rkennke/8191564/webrev.06/

Notice that I hooked this up into CollectedHeap::post_initialize() and
in doing so made that method concrete, and changed all subclass
post_initialize() methods to call the super-class.

Good now?

Thanks, Roman


> Hi Roman,
>
> This looks better now. Nice job.
> I wonder though, is it possible to extract the creation of managers and
> pools to a separate function for each collected heap?
> Now I see managers are created in e.g. CMS constructor, and the pools
> are created in CMSHeap::initialize(). Perhaps initialize could call
> initialize_serviceability() that sets up those things, so that they are
> nicely separated. Unless of course there is a good reason why the
> presence of managers is needed earlier than that in the bootstrapping.
>
> Otherwise I think this looks good!
>
> Thanks,
> /Erik
>
> On 2017-11-28 12:22, Roman Kennke wrote:
>> Hi Erik,
>>
>> Thanks for your review!
>>
>> All of the things that you mentioned should be addressed in the
>> following changes:
>>
>> Differential:
>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.05.diff/
>> Full:
>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.05/
>>
>> Also, Erik D (H) was so kind to contribute an additional testcase,
>> which is also included in the above patch.
>>
>> Ok now?
>>
>> Also, I need a review from serviceability-dev too!
>>
>> Thanks,
>> Roman
>>
>>
>>> 1) The code uses the "mgr" short name for "manager" in a bunch of
>>> names. I would be happy if we could write out the whole thing instead
>>> of the abbreviation.
>>> 2) It would be great if a generation would have a pointer to its
>>> manager, so we do not have to pass around the manager where we
>>> already pass around the generation (such as
>>> GenCollectedHeap::collect_generation).
>>> The generation could be created first, then the pools, then the
>>> managers, then do something like generation->set_memory_manager(x).
>>> 3) In cmsHeap.cpp:54: maxSize should preferably not use camel case.
>>>
>>> Otherwise I think it looks good.
>>>
>>> Thanks,
>>> /Erik
>>>
>>> On 2017-11-27 10:30, Roman Kennke wrote:
>>>> Erik implemented a few more refactorings and touch-ups, and here is
>>>> our final (pending reviews) webrev:
>>>>
>>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.04/
>>>>
>>>> Compared to webrev.02, it improves the handling of gc-end-message,
>>>> avoids dragging the GCMemoryManager through Generation and a few
>>>> little related refactorings.
>>>>
>>>> Ok to push now?
>>>>
>>>> Roman
>>>>
>>>>> After a few more discussions with Erik I made some more changes.
>>>>>
>>>>> MemoryService is now unaware of the number and meaning of GC memory
>>>>> managers (minor vs major). This should be better for GCs that don't
>>>>> make that distinction and/or use more different GCs (e.g. minor,
>>>>> intermediate, full).
>>>>>
>>>>> This means that I needed to add some abstractions:
>>>>> - GCMemoryManager now has gc_end_message() which is used by
>>>>> GCNotifier::pushNotification().
>>>>> - gc_begin() and gc_end() methods in MemoryService now accept a
>>>>> GCMemoryManager* instead of bull full_gc
>>>>> - Same for TraceMemoryManagerStats
>>>>> - Generation now knows about the corresponding GCMemoryManager
>>>>>
>>>>> Please review the full change:
>>>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.02/
>>>>>
>>>>> Thanks, Roman
>>>>>
>>>>>
>>>>>> I had some off-band discussions with Erik Helin and re-did most of
>>>>>> the changeset:
>>>>>> - The GC interface now resides in CollectedHeap, specifically the
>>>>>> two methods memory_managers() and memory_pools(), and is
>>>>>> implemented in the various concrete subclasses.
>>>>>> - Both methods return (by value) a GrowableArray<GCMemoryManager*>
>>>>>> and GrowableArray<MemoryPool> respectively. Returning a
>>>>>> stack-allocated GrowableArray seemed least complicated (avoid
>>>>>> explicit cleanup of short-lived array object), and most
>>>>>> future-proof, e.g. currently there is an implicit expectation to
>>>>>> get 2 GCMemoryManagers, even though some GCs don't necessarily
>>>>>> have two. The API allows for easy extension of the situation.
>>>>>>
>>>>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.01/
>>>>>>
>>>>>> I think this requires reviews from both the GC and Serviceability
>>>>>> team.
>>>>>>
>>>>>> Roman
>>>>>>
>>>>>>
>>>>>>> Currently, there's lots of GC specific code sprinkled over
>>>>>>> src/hotspot/share/services. This change introduces a GC interface
>>>>>>> for that, and moves all GC specific code to their respective
>>>>>>> src/hotspot/share/gc directory.
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.00/ 
>>>>>>> <http://cr.openjdk.java.net/%7Erkennke/8191564/webrev.00/>
>>>>>>>
>>>>>>> Testing: hotspot_gc and hotspot_serviceability, none showed
>>>>>>> regressions
>>>>>>>
>>>>>>> Built minimal and server without regressions
>>>>>>>
>>>>>>> What do you think?
>>>>>>>
>>>>>>> Roman
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191564: Refactor GC related servicability code into GC specific subclasses

Erik Österlund-2
Hi Roman,

Looks good now. Thanks for doing this.

Thanks,
/Erik

On 2017-11-28 22:26, Roman Kennke wrote:

> Hi Erik,
>
> thank you for reviewing!
>
> You are right. I think this is a leftover from when we tried to pass
> the GCMemoryManager* into the Generation constructor. The way it is
> done now (installing the GCMmemoryManager* later through
> set_memory_manager()) we can group all serviceability related set-up
> into initialize_serviceability():
>
> Differential:
> http://cr.openjdk.java.net/~rkennke/8191564/webrev.06.diff/
> Full:
> http://cr.openjdk.java.net/~rkennke/8191564/webrev.06/
>
> Notice that I hooked this up into CollectedHeap::post_initialize() and
> in doing so made that method concrete, and changed all subclass
> post_initialize() methods to call the super-class.
>
> Good now?
>
> Thanks, Roman
>
>
>> Hi Roman,
>>
>> This looks better now. Nice job.
>> I wonder though, is it possible to extract the creation of managers
>> and pools to a separate function for each collected heap?
>> Now I see managers are created in e.g. CMS constructor, and the pools
>> are created in CMSHeap::initialize(). Perhaps initialize could call
>> initialize_serviceability() that sets up those things, so that they
>> are nicely separated. Unless of course there is a good reason why the
>> presence of managers is needed earlier than that in the bootstrapping.
>>
>> Otherwise I think this looks good!
>>
>> Thanks,
>> /Erik
>>
>> On 2017-11-28 12:22, Roman Kennke wrote:
>>> Hi Erik,
>>>
>>> Thanks for your review!
>>>
>>> All of the things that you mentioned should be addressed in the
>>> following changes:
>>>
>>> Differential:
>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.05.diff/
>>> Full:
>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.05/
>>>
>>> Also, Erik D (H) was so kind to contribute an additional testcase,
>>> which is also included in the above patch.
>>>
>>> Ok now?
>>>
>>> Also, I need a review from serviceability-dev too!
>>>
>>> Thanks,
>>> Roman
>>>
>>>
>>>> 1) The code uses the "mgr" short name for "manager" in a bunch of
>>>> names. I would be happy if we could write out the whole thing
>>>> instead of the abbreviation.
>>>> 2) It would be great if a generation would have a pointer to its
>>>> manager, so we do not have to pass around the manager where we
>>>> already pass around the generation (such as
>>>> GenCollectedHeap::collect_generation).
>>>> The generation could be created first, then the pools, then the
>>>> managers, then do something like generation->set_memory_manager(x).
>>>> 3) In cmsHeap.cpp:54: maxSize should preferably not use camel case.
>>>>
>>>> Otherwise I think it looks good.
>>>>
>>>> Thanks,
>>>> /Erik
>>>>
>>>> On 2017-11-27 10:30, Roman Kennke wrote:
>>>>> Erik implemented a few more refactorings and touch-ups, and here
>>>>> is our final (pending reviews) webrev:
>>>>>
>>>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.04/
>>>>>
>>>>> Compared to webrev.02, it improves the handling of gc-end-message,
>>>>> avoids dragging the GCMemoryManager through Generation and a few
>>>>> little related refactorings.
>>>>>
>>>>> Ok to push now?
>>>>>
>>>>> Roman
>>>>>
>>>>>> After a few more discussions with Erik I made some more changes.
>>>>>>
>>>>>> MemoryService is now unaware of the number and meaning of GC
>>>>>> memory managers (minor vs major). This should be better for GCs
>>>>>> that don't make that distinction and/or use more different GCs
>>>>>> (e.g. minor, intermediate, full).
>>>>>>
>>>>>> This means that I needed to add some abstractions:
>>>>>> - GCMemoryManager now has gc_end_message() which is used by
>>>>>> GCNotifier::pushNotification().
>>>>>> - gc_begin() and gc_end() methods in MemoryService now accept a
>>>>>> GCMemoryManager* instead of bull full_gc
>>>>>> - Same for TraceMemoryManagerStats
>>>>>> - Generation now knows about the corresponding GCMemoryManager
>>>>>>
>>>>>> Please review the full change:
>>>>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.02/
>>>>>>
>>>>>> Thanks, Roman
>>>>>>
>>>>>>
>>>>>>> I had some off-band discussions with Erik Helin and re-did most
>>>>>>> of the changeset:
>>>>>>> - The GC interface now resides in CollectedHeap, specifically
>>>>>>> the two methods memory_managers() and memory_pools(), and is
>>>>>>> implemented in the various concrete subclasses.
>>>>>>> - Both methods return (by value) a
>>>>>>> GrowableArray<GCMemoryManager*> and GrowableArray<MemoryPool>
>>>>>>> respectively. Returning a stack-allocated GrowableArray seemed
>>>>>>> least complicated (avoid explicit cleanup of short-lived array
>>>>>>> object), and most future-proof, e.g. currently there is an
>>>>>>> implicit expectation to get 2 GCMemoryManagers, even though some
>>>>>>> GCs don't necessarily have two. The API allows for easy
>>>>>>> extension of the situation.
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.01/
>>>>>>>
>>>>>>> I think this requires reviews from both the GC and
>>>>>>> Serviceability team.
>>>>>>>
>>>>>>> Roman
>>>>>>>
>>>>>>>
>>>>>>>> Currently, there's lots of GC specific code sprinkled over
>>>>>>>> src/hotspot/share/services. This change introduces a GC
>>>>>>>> interface for that, and moves all GC specific code to their
>>>>>>>> respective src/hotspot/share/gc directory.
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.00/ 
>>>>>>>> <http://cr.openjdk.java.net/%7Erkennke/8191564/webrev.00/>
>>>>>>>>
>>>>>>>> Testing: hotspot_gc and hotspot_serviceability, none showed
>>>>>>>> regressions
>>>>>>>>
>>>>>>>> Built minimal and server without regressions
>>>>>>>>
>>>>>>>> What do you think?
>>>>>>>>
>>>>>>>> Roman
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191564: Refactor GC related servicability code into GC specific subclasses

Roman Kennke-6
Hi Erik,

thanks for the review!

I think this requires another reviewer from serviceability-dev. Who can
I ping about this?

Roman


> Hi Roman,
>
> Looks good now. Thanks for doing this.
>
> Thanks,
> /Erik
>
> On 2017-11-28 22:26, Roman Kennke wrote:
>> Hi Erik,
>>
>> thank you for reviewing!
>>
>> You are right. I think this is a leftover from when we tried to pass
>> the GCMemoryManager* into the Generation constructor. The way it is
>> done now (installing the GCMmemoryManager* later through
>> set_memory_manager()) we can group all serviceability related set-up
>> into initialize_serviceability():
>>
>> Differential:
>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.06.diff/
>> Full:
>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.06/
>>
>> Notice that I hooked this up into CollectedHeap::post_initialize() and
>> in doing so made that method concrete, and changed all subclass
>> post_initialize() methods to call the super-class.
>>
>> Good now?
>>
>> Thanks, Roman
>>
>>
>>> Hi Roman,
>>>
>>> This looks better now. Nice job.
>>> I wonder though, is it possible to extract the creation of managers
>>> and pools to a separate function for each collected heap?
>>> Now I see managers are created in e.g. CMS constructor, and the pools
>>> are created in CMSHeap::initialize(). Perhaps initialize could call
>>> initialize_serviceability() that sets up those things, so that they
>>> are nicely separated. Unless of course there is a good reason why the
>>> presence of managers is needed earlier than that in the bootstrapping.
>>>
>>> Otherwise I think this looks good!
>>>
>>> Thanks,
>>> /Erik
>>>
>>> On 2017-11-28 12:22, Roman Kennke wrote:
>>>> Hi Erik,
>>>>
>>>> Thanks for your review!
>>>>
>>>> All of the things that you mentioned should be addressed in the
>>>> following changes:
>>>>
>>>> Differential:
>>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.05.diff/
>>>> Full:
>>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.05/
>>>>
>>>> Also, Erik D (H) was so kind to contribute an additional testcase,
>>>> which is also included in the above patch.
>>>>
>>>> Ok now?
>>>>
>>>> Also, I need a review from serviceability-dev too!
>>>>
>>>> Thanks,
>>>> Roman
>>>>
>>>>
>>>>> 1) The code uses the "mgr" short name for "manager" in a bunch of
>>>>> names. I would be happy if we could write out the whole thing
>>>>> instead of the abbreviation.
>>>>> 2) It would be great if a generation would have a pointer to its
>>>>> manager, so we do not have to pass around the manager where we
>>>>> already pass around the generation (such as
>>>>> GenCollectedHeap::collect_generation).
>>>>> The generation could be created first, then the pools, then the
>>>>> managers, then do something like generation->set_memory_manager(x).
>>>>> 3) In cmsHeap.cpp:54: maxSize should preferably not use camel case.
>>>>>
>>>>> Otherwise I think it looks good.
>>>>>
>>>>> Thanks,
>>>>> /Erik
>>>>>
>>>>> On 2017-11-27 10:30, Roman Kennke wrote:
>>>>>> Erik implemented a few more refactorings and touch-ups, and here
>>>>>> is our final (pending reviews) webrev:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.04/
>>>>>>
>>>>>> Compared to webrev.02, it improves the handling of gc-end-message,
>>>>>> avoids dragging the GCMemoryManager through Generation and a few
>>>>>> little related refactorings.
>>>>>>
>>>>>> Ok to push now?
>>>>>>
>>>>>> Roman
>>>>>>
>>>>>>> After a few more discussions with Erik I made some more changes.
>>>>>>>
>>>>>>> MemoryService is now unaware of the number and meaning of GC
>>>>>>> memory managers (minor vs major). This should be better for GCs
>>>>>>> that don't make that distinction and/or use more different GCs
>>>>>>> (e.g. minor, intermediate, full).
>>>>>>>
>>>>>>> This means that I needed to add some abstractions:
>>>>>>> - GCMemoryManager now has gc_end_message() which is used by
>>>>>>> GCNotifier::pushNotification().
>>>>>>> - gc_begin() and gc_end() methods in MemoryService now accept a
>>>>>>> GCMemoryManager* instead of bull full_gc
>>>>>>> - Same for TraceMemoryManagerStats
>>>>>>> - Generation now knows about the corresponding GCMemoryManager
>>>>>>>
>>>>>>> Please review the full change:
>>>>>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.02/
>>>>>>>
>>>>>>> Thanks, Roman
>>>>>>>
>>>>>>>
>>>>>>>> I had some off-band discussions with Erik Helin and re-did most
>>>>>>>> of the changeset:
>>>>>>>> - The GC interface now resides in CollectedHeap, specifically
>>>>>>>> the two methods memory_managers() and memory_pools(), and is
>>>>>>>> implemented in the various concrete subclasses.
>>>>>>>> - Both methods return (by value) a
>>>>>>>> GrowableArray<GCMemoryManager*> and GrowableArray<MemoryPool>
>>>>>>>> respectively. Returning a stack-allocated GrowableArray seemed
>>>>>>>> least complicated (avoid explicit cleanup of short-lived array
>>>>>>>> object), and most future-proof, e.g. currently there is an
>>>>>>>> implicit expectation to get 2 GCMemoryManagers, even though some
>>>>>>>> GCs don't necessarily have two. The API allows for easy
>>>>>>>> extension of the situation.
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.01/
>>>>>>>>
>>>>>>>> I think this requires reviews from both the GC and
>>>>>>>> Serviceability team.
>>>>>>>>
>>>>>>>> Roman
>>>>>>>>
>>>>>>>>
>>>>>>>>> Currently, there's lots of GC specific code sprinkled over
>>>>>>>>> src/hotspot/share/services. This change introduces a GC
>>>>>>>>> interface for that, and moves all GC specific code to their
>>>>>>>>> respective src/hotspot/share/gc directory.
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.00/ 
>>>>>>>>> <http://cr.openjdk.java.net/%7Erkennke/8191564/webrev.00/>
>>>>>>>>>
>>>>>>>>> Testing: hotspot_gc and hotspot_serviceability, none showed
>>>>>>>>> regressions
>>>>>>>>>
>>>>>>>>> Built minimal and server without regressions
>>>>>>>>>
>>>>>>>>> What do you think?
>>>>>>>>>
>>>>>>>>> Roman
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191564: Refactor GC related servicability code into GC specific subclasses

Erik Helin-2
In reply to this post by Roman Kennke-6
On 11/28/2017 10:26 PM, Roman Kennke wrote:

> Hi Erik,
>
> thank you for reviewing!
>
> You are right. I think this is a leftover from when we tried to pass the
> GCMemoryManager* into the Generation constructor. The way it is done now
> (installing the GCMmemoryManager* later through set_memory_manager()) we
> can group all serviceability related set-up into
> initialize_serviceability():
>
> Differential:
> http://cr.openjdk.java.net/~rkennke/8191564/webrev.06.diff/
> Full:
> http://cr.openjdk.java.net/~rkennke/8191564/webrev.06/

Looks good to me, Reviewed. I also put version 04 through some testing
and it successfully passed hs-tier{1,2,3,4,5}.

Thanks,
Erik

> Notice that I hooked this up into CollectedHeap::post_initialize() and
> in doing so made that method concrete, and changed all subclass
> post_initialize() methods to call the super-class.
>
> Good now?
>
> Thanks, Roman
>
>
>> Hi Roman,
>>
>> This looks better now. Nice job.
>> I wonder though, is it possible to extract the creation of managers
>> and pools to a separate function for each collected heap?
>> Now I see managers are created in e.g. CMS constructor, and the pools
>> are created in CMSHeap::initialize(). Perhaps initialize could call
>> initialize_serviceability() that sets up those things, so that they
>> are nicely separated. Unless of course there is a good reason why the
>> presence of managers is needed earlier than that in the bootstrapping.
>>
>> Otherwise I think this looks good!
>>
>> Thanks,
>> /Erik
>>
>> On 2017-11-28 12:22, Roman Kennke wrote:
>>> Hi Erik,
>>>
>>> Thanks for your review!
>>>
>>> All of the things that you mentioned should be addressed in the
>>> following changes:
>>>
>>> Differential:
>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.05.diff/
>>> Full:
>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.05/
>>>
>>> Also, Erik D (H) was so kind to contribute an additional testcase,
>>> which is also included in the above patch.
>>>
>>> Ok now?
>>>
>>> Also, I need a review from serviceability-dev too!
>>>
>>> Thanks,
>>> Roman
>>>
>>>
>>>> 1) The code uses the "mgr" short name for "manager" in a bunch of
>>>> names. I would be happy if we could write out the whole thing
>>>> instead of the abbreviation.
>>>> 2) It would be great if a generation would have a pointer to its
>>>> manager, so we do not have to pass around the manager where we
>>>> already pass around the generation (such as
>>>> GenCollectedHeap::collect_generation).
>>>> The generation could be created first, then the pools, then the
>>>> managers, then do something like generation->set_memory_manager(x).
>>>> 3) In cmsHeap.cpp:54: maxSize should preferably not use camel case.
>>>>
>>>> Otherwise I think it looks good.
>>>>
>>>> Thanks,
>>>> /Erik
>>>>
>>>> On 2017-11-27 10:30, Roman Kennke wrote:
>>>>> Erik implemented a few more refactorings and touch-ups, and here is
>>>>> our final (pending reviews) webrev:
>>>>>
>>>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.04/
>>>>>
>>>>> Compared to webrev.02, it improves the handling of gc-end-message,
>>>>> avoids dragging the GCMemoryManager through Generation and a few
>>>>> little related refactorings.
>>>>>
>>>>> Ok to push now?
>>>>>
>>>>> Roman
>>>>>
>>>>>> After a few more discussions with Erik I made some more changes.
>>>>>>
>>>>>> MemoryService is now unaware of the number and meaning of GC
>>>>>> memory managers (minor vs major). This should be better for GCs
>>>>>> that don't make that distinction and/or use more different GCs
>>>>>> (e.g. minor, intermediate, full).
>>>>>>
>>>>>> This means that I needed to add some abstractions:
>>>>>> - GCMemoryManager now has gc_end_message() which is used by
>>>>>> GCNotifier::pushNotification().
>>>>>> - gc_begin() and gc_end() methods in MemoryService now accept a
>>>>>> GCMemoryManager* instead of bull full_gc
>>>>>> - Same for TraceMemoryManagerStats
>>>>>> - Generation now knows about the corresponding GCMemoryManager
>>>>>>
>>>>>> Please review the full change:
>>>>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.02/
>>>>>>
>>>>>> Thanks, Roman
>>>>>>
>>>>>>
>>>>>>> I had some off-band discussions with Erik Helin and re-did most
>>>>>>> of the changeset:
>>>>>>> - The GC interface now resides in CollectedHeap, specifically the
>>>>>>> two methods memory_managers() and memory_pools(), and is
>>>>>>> implemented in the various concrete subclasses.
>>>>>>> - Both methods return (by value) a
>>>>>>> GrowableArray<GCMemoryManager*> and GrowableArray<MemoryPool>
>>>>>>> respectively. Returning a stack-allocated GrowableArray seemed
>>>>>>> least complicated (avoid explicit cleanup of short-lived array
>>>>>>> object), and most future-proof, e.g. currently there is an
>>>>>>> implicit expectation to get 2 GCMemoryManagers, even though some
>>>>>>> GCs don't necessarily have two. The API allows for easy extension
>>>>>>> of the situation.
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.01/
>>>>>>>
>>>>>>> I think this requires reviews from both the GC and Serviceability
>>>>>>> team.
>>>>>>>
>>>>>>> Roman
>>>>>>>
>>>>>>>
>>>>>>>> Currently, there's lots of GC specific code sprinkled over
>>>>>>>> src/hotspot/share/services. This change introduces a GC
>>>>>>>> interface for that, and moves all GC specific code to their
>>>>>>>> respective src/hotspot/share/gc directory.
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.00/ 
>>>>>>>> <http://cr.openjdk.java.net/%7Erkennke/8191564/webrev.00/>
>>>>>>>>
>>>>>>>> Testing: hotspot_gc and hotspot_serviceability, none showed
>>>>>>>> regressions
>>>>>>>>
>>>>>>>> Built minimal and server without regressions
>>>>>>>>
>>>>>>>> What do you think?
>>>>>>>>
>>>>>>>> Roman
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8191564: Refactor GC related servicability code into GC specific subclasses

Erik Helin-2
In reply to this post by Roman Kennke-6
On 11/29/2017 09:39 AM, Roman Kennke wrote:
> Hi Erik,
>
> thanks for the review!
>
> I think this requires another reviewer from serviceability-dev. Who can
> I ping about this?

You could try the hotspot-runtime-dev email list, although I suspect
most of the runtime developers are on serviceability-dev list as well...

Thanks,
Erik

> Roman
>
>
>> Hi Roman,
>>
>> Looks good now. Thanks for doing this.
>>
>> Thanks,
>> /Erik
>>
>> On 2017-11-28 22:26, Roman Kennke wrote:
>>> Hi Erik,
>>>
>>> thank you for reviewing!
>>>
>>> You are right. I think this is a leftover from when we tried to pass
>>> the GCMemoryManager* into the Generation constructor. The way it is
>>> done now (installing the GCMmemoryManager* later through
>>> set_memory_manager()) we can group all serviceability related set-up
>>> into initialize_serviceability():
>>>
>>> Differential:
>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.06.diff/
>>> Full:
>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.06/
>>>
>>> Notice that I hooked this up into CollectedHeap::post_initialize()
>>> and in doing so made that method concrete, and changed all subclass
>>> post_initialize() methods to call the super-class.
>>>
>>> Good now?
>>>
>>> Thanks, Roman
>>>
>>>
>>>> Hi Roman,
>>>>
>>>> This looks better now. Nice job.
>>>> I wonder though, is it possible to extract the creation of managers
>>>> and pools to a separate function for each collected heap?
>>>> Now I see managers are created in e.g. CMS constructor, and the
>>>> pools are created in CMSHeap::initialize(). Perhaps initialize could
>>>> call initialize_serviceability() that sets up those things, so that
>>>> they are nicely separated. Unless of course there is a good reason
>>>> why the presence of managers is needed earlier than that in the
>>>> bootstrapping.
>>>>
>>>> Otherwise I think this looks good!
>>>>
>>>> Thanks,
>>>> /Erik
>>>>
>>>> On 2017-11-28 12:22, Roman Kennke wrote:
>>>>> Hi Erik,
>>>>>
>>>>> Thanks for your review!
>>>>>
>>>>> All of the things that you mentioned should be addressed in the
>>>>> following changes:
>>>>>
>>>>> Differential:
>>>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.05.diff/
>>>>> Full:
>>>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.05/
>>>>>
>>>>> Also, Erik D (H) was so kind to contribute an additional testcase,
>>>>> which is also included in the above patch.
>>>>>
>>>>> Ok now?
>>>>>
>>>>> Also, I need a review from serviceability-dev too!
>>>>>
>>>>> Thanks,
>>>>> Roman
>>>>>
>>>>>
>>>>>> 1) The code uses the "mgr" short name for "manager" in a bunch of
>>>>>> names. I would be happy if we could write out the whole thing
>>>>>> instead of the abbreviation.
>>>>>> 2) It would be great if a generation would have a pointer to its
>>>>>> manager, so we do not have to pass around the manager where we
>>>>>> already pass around the generation (such as
>>>>>> GenCollectedHeap::collect_generation).
>>>>>> The generation could be created first, then the pools, then the
>>>>>> managers, then do something like generation->set_memory_manager(x).
>>>>>> 3) In cmsHeap.cpp:54: maxSize should preferably not use camel case.
>>>>>>
>>>>>> Otherwise I think it looks good.
>>>>>>
>>>>>> Thanks,
>>>>>> /Erik
>>>>>>
>>>>>> On 2017-11-27 10:30, Roman Kennke wrote:
>>>>>>> Erik implemented a few more refactorings and touch-ups, and here
>>>>>>> is our final (pending reviews) webrev:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.04/
>>>>>>>
>>>>>>> Compared to webrev.02, it improves the handling of
>>>>>>> gc-end-message, avoids dragging the GCMemoryManager through
>>>>>>> Generation and a few little related refactorings.
>>>>>>>
>>>>>>> Ok to push now?
>>>>>>>
>>>>>>> Roman
>>>>>>>
>>>>>>>> After a few more discussions with Erik I made some more changes.
>>>>>>>>
>>>>>>>> MemoryService is now unaware of the number and meaning of GC
>>>>>>>> memory managers (minor vs major). This should be better for GCs
>>>>>>>> that don't make that distinction and/or use more different GCs
>>>>>>>> (e.g. minor, intermediate, full).
>>>>>>>>
>>>>>>>> This means that I needed to add some abstractions:
>>>>>>>> - GCMemoryManager now has gc_end_message() which is used by
>>>>>>>> GCNotifier::pushNotification().
>>>>>>>> - gc_begin() and gc_end() methods in MemoryService now accept a
>>>>>>>> GCMemoryManager* instead of bull full_gc
>>>>>>>> - Same for TraceMemoryManagerStats
>>>>>>>> - Generation now knows about the corresponding GCMemoryManager
>>>>>>>>
>>>>>>>> Please review the full change:
>>>>>>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.02/
>>>>>>>>
>>>>>>>> Thanks, Roman
>>>>>>>>
>>>>>>>>
>>>>>>>>> I had some off-band discussions with Erik Helin and re-did most
>>>>>>>>> of the changeset:
>>>>>>>>> - The GC interface now resides in CollectedHeap, specifically
>>>>>>>>> the two methods memory_managers() and memory_pools(), and is
>>>>>>>>> implemented in the various concrete subclasses.
>>>>>>>>> - Both methods return (by value) a
>>>>>>>>> GrowableArray<GCMemoryManager*> and GrowableArray<MemoryPool>
>>>>>>>>> respectively. Returning a stack-allocated GrowableArray seemed
>>>>>>>>> least complicated (avoid explicit cleanup of short-lived array
>>>>>>>>> object), and most future-proof, e.g. currently there is an
>>>>>>>>> implicit expectation to get 2 GCMemoryManagers, even though
>>>>>>>>> some GCs don't necessarily have two. The API allows for easy
>>>>>>>>> extension of the situation.
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.01/
>>>>>>>>>
>>>>>>>>> I think this requires reviews from both the GC and
>>>>>>>>> Serviceability team.
>>>>>>>>>
>>>>>>>>> Roman
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> Currently, there's lots of GC specific code sprinkled over
>>>>>>>>>> src/hotspot/share/services. This change introduces a GC
>>>>>>>>>> interface for that, and moves all GC specific code to their
>>>>>>>>>> respective src/hotspot/share/gc directory.
>>>>>>>>>>
>>>>>>>>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.00/ 
>>>>>>>>>> <http://cr.openjdk.java.net/%7Erkennke/8191564/webrev.00/>
>>>>>>>>>>
>>>>>>>>>> Testing: hotspot_gc and hotspot_serviceability, none showed
>>>>>>>>>> regressions
>>>>>>>>>>
>>>>>>>>>> Built minimal and server without regressions
>>>>>>>>>>
>>>>>>>>>> What do you think?
>>>>>>>>>>
>>>>>>>>>> Roman
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>