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

[Roman Kennke]

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