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

Roman Kennke rkennke at redhat.com
Thu Nov 30 11:30:24 UTC 2017 

Hi David,

yes I did, but it's probably got lost somewhere, while I was bouncing 
the patch between me and Erik D. (I noticed that the msg also got lost). 
Both reinstated:

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

Roman

> Hi Roman,
> 
> Just glancing at this but did you use "hg rename" to move the files? The 
> webrev suggests not.
> 
> Thanks,
> David
> 
> On 30/11/2017 1:38 AM, Roman Kennke wrote:
>> 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
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>

More information about the serviceability-dev mailing list