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

Thu Nov 30 14:55:11 UTC 2017

Hi Roman,

Looks good.
You do need a class GCMemoryManager; declaration in generation.hpp to 
build without precompiled headers though.
I do not need a new webrev for that change.

Thanks,
/Erik

On 2017-11-30 15:22, Roman Kennke wrote:
> 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
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>
>