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

[Roman Kennke]

Including hotspot-runtime-dev...

I need one more review (esp. for the src/hotspot/share/services part):

http://cr.openjdk.java.net/~rkennke/8191564/webrev.06/

Thanks, Roman


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