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

[Erik Österlund]

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