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

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

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

Roman Kennke-6
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

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

Ping?

Roman

Reply | Threaded
Open this post in threaded view
|

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

Roman Kennke-6
In reply to this post by Roman Kennke-6
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
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
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,

A few things:

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