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

Roman Kennke rkennke at redhat.com
Thu Nov 30 14:22:09 UTC 2017 

Hi David,

I added the tag as you proposed:

Differential:
http://cr.openjdk.java.net/~rkennke/8191564/webrev.08.diff/
Full:
http://cr.openjdk.java.net/~rkennke/8191564/webrev.08/

Re-run tests without regressions.

Roman

> 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 serviceability-dev mailing list