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

[Erik Österlund]

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