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
