RFR: 8191564: Refactor GC related servicability code into GC specific subclasses
erik.osterlund at oracle.com
Mon Nov 27 16:23:35 UTC 2017
A few things:
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
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.
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:
> 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?
>> 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
>> - 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:
>> 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.
>>> I think this requires reviews from both the GC and Serviceability team.
>>>> 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.
>>>> Testing: hotspot_gc and hotspot_serviceability, none showed
>>>> Built minimal and server without regressions
>>>> What do you think?
More information about the hotspot-gc-dev