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

[Erik Österlund]

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