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

[Roman Kennke]

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