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

David Holmes david.holmes at oracle.com
Thu Nov 30 05:32:17 UTC 2017 

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 hotspot-runtime-dev mailing list