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

[Roman Kennke]

Hi Erik,

thanks for the review!

I think this requires another reviewer from serviceability-dev. Who can 
I ping about this?

Roman


> Hi Roman,
> 
> Looks good now. Thanks for doing this.
> 
> Thanks,
> /Erik
> 
> On 2017-11-28 22:26, Roman Kennke wrote:
>> 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
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>