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

classic Classic list List threaded Threaded
9 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
Including hotspot-runtime-dev...

I need one more review (esp. for the src/hotspot/share/services part):

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

Thanks, Roman


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

Reply | Threaded
Open this post in threaded view
|

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

David Holmes
Hi Roman,

Just glancing at this but did you use "hg rename" to move the files? The
webrev suggests not.

Thanks,
David

On 30/11/2017 1:38 AM, Roman Kennke wrote:

> Including hotspot-runtime-dev...
>
> I need one more review (esp. for the src/hotspot/share/services part):
>
> http://cr.openjdk.java.net/~rkennke/8191564/webrev.06/
>
> Thanks, Roman
>
>
>> 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
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|

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

Roman Kennke-6
Hi David,

yes I did, but it's probably got lost somewhere, while I was bouncing
the patch between me and Erik D. (I noticed that the msg also got lost).
Both reinstated:

http://cr.openjdk.java.net/~rkennke/8191564/webrev.07/

Roman

> Hi Roman,
>
> Just glancing at this but did you use "hg rename" to move the files? The
> webrev suggests not.
>
> Thanks,
> David
>
> On 30/11/2017 1:38 AM, Roman Kennke wrote:
>> Including hotspot-runtime-dev...
>>
>> I need one more review (esp. for the src/hotspot/share/services part):
>>
>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.06/
>>
>> Thanks, Roman
>>
>>
>>> 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
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>

Reply | Threaded
Open this post in threaded view
|

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

David Holmes
On 30/11/2017 9:30 PM, Roman Kennke wrote:
> Hi David,
>
> yes I did, but it's probably got lost somewhere, while I was bouncing
> the patch between me and Erik D. (I noticed that the msg also got lost).
> Both reinstated:
>
> http://cr.openjdk.java.net/~rkennke/8191564/webrev.07/

Thanks - that's much simpler to follow.

IIRC in the test I think you need "@requires vm.gc == null" to skip the
test if the framework is trying to run it with an explicit GC
configuration - else it may conflict with your hardwired GC selection.

The overall refactoring seems reasonable, but I can't really vouch for
its accuracy. I'm not clear how/if these service APIs are accesses from
the Java-level serviceability code.

David

> Roman
>
>> Hi Roman,
>>
>> Just glancing at this but did you use "hg rename" to move the files?
>> The webrev suggests not.
>>
>> Thanks,
>> David
>>
>> On 30/11/2017 1:38 AM, Roman Kennke wrote:
>>> Including hotspot-runtime-dev...
>>>
>>> I need one more review (esp. for the src/hotspot/share/services part):
>>>
>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.06/
>>>
>>> Thanks, Roman
>>>
>>>
>>>> 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
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|

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

Roman Kennke-6
Hi David,

I added the tag as you proposed:

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

Re-run tests without regressions.

Roman

> On 30/11/2017 9:30 PM, Roman Kennke wrote:
>> Hi David,
>>
>> yes I did, but it's probably got lost somewhere, while I was bouncing
>> the patch between me and Erik D. (I noticed that the msg also got
>> lost). Both reinstated:
>>
>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.07/
>
> Thanks - that's much simpler to follow.
>
> IIRC in the test I think you need "@requires vm.gc == null" to skip the
> test if the framework is trying to run it with an explicit GC
> configuration - else it may conflict with your hardwired GC selection.
>
> The overall refactoring seems reasonable, but I can't really vouch for
> its accuracy. I'm not clear how/if these service APIs are accesses from
> the Java-level serviceability code.
>
> David
>
>> Roman
>>
>>> Hi Roman,
>>>
>>> Just glancing at this but did you use "hg rename" to move the files?
>>> The webrev suggests not.
>>>
>>> Thanks,
>>> David
>>>
>>> On 30/11/2017 1:38 AM, Roman Kennke wrote:
>>>> Including hotspot-runtime-dev...
>>>>
>>>> I need one more review (esp. for the src/hotspot/share/services part):
>>>>
>>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.06/
>>>>
>>>> Thanks, Roman
>>>>
>>>>
>>>>> 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
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>

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.
You do need a class GCMemoryManager; declaration in generation.hpp to
build without precompiled headers though.
I do not need a new webrev for that change.

Thanks,
/Erik

On 2017-11-30 15:22, Roman Kennke wrote:

> Hi David,
>
> I added the tag as you proposed:
>
> Differential:
> http://cr.openjdk.java.net/~rkennke/8191564/webrev.08.diff/
> Full:
> http://cr.openjdk.java.net/~rkennke/8191564/webrev.08/
>
> Re-run tests without regressions.
>
> Roman
>
>> On 30/11/2017 9:30 PM, Roman Kennke wrote:
>>> Hi David,
>>>
>>> yes I did, but it's probably got lost somewhere, while I was
>>> bouncing the patch between me and Erik D. (I noticed that the msg
>>> also got lost). Both reinstated:
>>>
>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.07/
>>
>> Thanks - that's much simpler to follow.
>>
>> IIRC in the test I think you need "@requires vm.gc == null" to skip
>> the test if the framework is trying to run it with an explicit GC
>> configuration - else it may conflict with your hardwired GC selection.
>>
>> The overall refactoring seems reasonable, but I can't really vouch
>> for its accuracy. I'm not clear how/if these service APIs are
>> accesses from the Java-level serviceability code.
>>
>> David
>>
>>> Roman
>>>
>>>> Hi Roman,
>>>>
>>>> Just glancing at this but did you use "hg rename" to move the
>>>> files? The webrev suggests not.
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>> On 30/11/2017 1:38 AM, Roman Kennke wrote:
>>>>> Including hotspot-runtime-dev...
>>>>>
>>>>> I need one more review (esp. for the src/hotspot/share/services
>>>>> part):
>>>>>
>>>>> http://cr.openjdk.java.net/~rkennke/8191564/webrev.06/
>>>>>
>>>>> Thanks, Roman
>>>>>
>>>>>
>>>>>> 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
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>
>

Reply | Threaded
Open this post in threaded view
|

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

Mandy Chung
In reply to this post by Roman Kennke-6


On 11/30/17 6:22 AM, Roman Kennke wrote:
> Hi David,
>
> I added the tag as you proposed:
>
> Differential:
> http://cr.openjdk.java.net/~rkennke/8191564/webrev.08.diff/
> Full:
> http://cr.openjdk.java.net/~rkennke/8191564/webrev.08/
>

This looks really good.  This version looks much better than an early
version I looked at (thanks for regenerating webrev with hg rename).

A minor comment that GCMemoryManager could take an enum to indicate the
type of this  GC action (major vs minor).   This can be a future cleanup.

thanks
Mandy
Reply | Threaded
Open this post in threaded view
|

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

Roman Kennke-6
Hi Mandy,

thanks for reviewing! I think Erik Ö. already pushed it for me.

> A minor comment that GCMemoryManager could take an enum to indicate the
> type of this  GC action (major vs minor).   This can be a future cleanup.

This may be overly restrictive: some GCs may not necessarily provide a
major/minor distinction, but have only one GC manager (e.g. epsilon), or
may provide 3 or more GC managers (e.g. 'minor' partial (or young) GC,
'intermediate' concurrent GC, 'intermediate or major' concurrent full
GC, 'major' STW last-ditch full GC...).


Roman
Reply | Threaded
Open this post in threaded view
|

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

Mandy Chung


On 11/30/17 10:26 AM, Roman Kennke wrote:

> Hi Mandy,
>
> thanks for reviewing! I think Erik Ö. already pushed it for me.
>
>> A minor comment that GCMemoryManager could take an enum to indicate
>> the type of this  GC action (major vs minor).   This can be a future
>> cleanup.
>
> This may be overly restrictive: some GCs may not necessarily provide a
> major/minor distinction, but have only one GC manager (e.g. epsilon),
> or may provide 3 or more GC managers (e.g. 'minor' partial (or young)
> GC, 'intermediate' concurrent GC, 'intermediate or major' concurrent
> full GC, 'major' STW last-ditch full GC...).

Right - there will be more than 2 enums and major and major are just
examples that could let GC notififer to determine the message based on
the type.  This is minor and just a thought that can be considered in
the future.

Thanks.
Mandy