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

Wed Nov 29 09:10:40 UTC 2017

On 11/29/2017 09:39 AM, Roman Kennke wrote:
> Hi Erik,
> 
> thanks for the review!
> 
> I think this requires another reviewer from serviceability-dev. Who can 
> I ping about this?

You could try the hotspot-runtime-dev email list, although I suspect 
most of the runtime developers are on serviceability-dev list as well...

Thanks,
Erik

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