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

On 11/30/17 10:26 AM, Roman Kennke wrote: Hi Mandy, thanks for reviewing! I think Erik Ö. already pushed it for me. A minor comment that GCMemoryManager could take an enum to indicate the type of this  GC action (major vs minor).   This can be a future cleanup. This may be overly restrict

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

Hi Mandy, thanks for reviewing! I think Erik Ö. already pushed it for me. A minor comment that GCMemoryManager could take an enum to indicate the type of this  GC action (major vs minor).   This can be a future cleanup. This may be overly restrictive: some GCs may not necessarily provide a m

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

On 11/30/17 6:22 AM, 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/ This looks really good.  This version looks much better than an early

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

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:

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

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

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

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 s

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

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/ Roman Hi Roman, Just glancing at this but did you use "hg rename

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

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.ne

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

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 f

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

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-de

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

On 11/28/2017 10:26 PM, 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())

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

Hi Erik, thanks for the review! I think this requires another reviewer from serviceability-dev. Who can I ping about this? 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

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

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 (insta

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

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

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

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

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

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 contribut

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

Hi Roman, 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 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 m

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

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 lit

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

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

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

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 Growable

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

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/

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

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/