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
