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

David Holmes david.holmes at oracle.com
Thu Nov 30 12:21:25 UTC 2017 

On 30/11/2017 9:30 PM, Roman Kennke wrote:
> 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/

Thanks - that's much simpler to follow.

IIRC in the test I think you need "@requires vm.gc == null" to skip the 
test if the framework is trying to run it with an explicit GC 
configuration - else it may conflict with your hardwired GC selection.

The overall refactoring seems reasonable, but I can't really vouch for 
its accuracy. I'm not clear how/if these service APIs are accesses from 
the Java-level serviceability code.

David

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