Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[Hohensee, Paul]

Thanks, Erik.

On 6/18/18, 10:26 AM, "Erik Helin"  wrote:

On 06/18/2018 06:14 PM, Hohensee, Paul wrote:
> Thanks, Eric!
> 
> I'd push, but it seems I don't seem to have permission at the moment. Who 
should I contact to get that fixed?

That would be o...@openjdk.java.net.

Thanks,
Erik

> Thanks,
> 
> Paul
> 
> On 6/18/18, 7:09 AM, "Erik Helin"  wrote:
> 
>  On 06/16/2018 09:00 PM, Hohensee, Paul wrote:
>  > Thanks for the re-review, Erik. New webrev with your fixes:
>  >
>  > http://cr.openjdk.java.net/~phh/8195115/webrev.04/
>  
>  The patch is good to go now, Reviewed.
>  
>  Thanks,
>  Erik
>  
>  > Need another reviewer, please.
>  >
>  > Thanks,
>  >
>  > Paul
>  >
>  > On 6/16/18, 1:25 AM, "Erik Helin"  wrote:
>  >
>  >  On 06/15/2018 10:21 PM, Hohensee, Paul wrote:
>  >  > After some difficulty with the submit cluster, with which 
Erik helped me out, the patch passes. It also passed fastdebug hotspot tier 1 
testing on my Mac laptop, which former includes the new test.
>  >  >
>  >  > I had to increase -Xmx and -Xms to 12m in order to get 
TestOldGenCollectionUsage to pass on the submit cluster, though the old 10m 
works fine on my Mac. New webrev:
>  >
>  >  Thanks, the change of -Xmx and -Xms to 12m now also makes the 
test pass
>  >  on my workstation.
>  >
>  >  > http://cr.openjdk.java.net/~phh/8195115/webrev.03/
>  >
>  >  There seems to be some trailing whitespace in the patch, have 
you run
>  >  jcheck (or `hg diff` which highlights trailing whitespace in 
red)?
>  >  Please see
>  >
>  >  +TraceMemoryManagerStats tms(&_memory_manager, gc_cause(),
>  >  +collector_state()->yc_type() 
== Mixed
>  >  /* allMemoryPoolsAffected */);
>  >  +
>  >^ whitespace
>  >
>  >  and
>  >
>  >  +int MemoryManager::add_pool(MemoryPool* pool) {
>  >  +  int index = _num_pools;
>  > ^ whitespace
>  >
>  >  Another small comment, I would have written
>  >
>  >  +void GCMemoryManager::add_pool(MemoryPool* pool) {
>  >  +  int index = MemoryManager::add_pool(pool);
>  >  +  _pool_always_affected_by_gc[index] = true;
>  >  +}
>  >  +
>  >  +void GCMemoryManager::add_pool(MemoryPool* pool, bool
>  >  always_affected_by_gc) {
>  >  +  int index = MemoryManager::add_pool(pool);
>  >  +  _pool_always_affected_by_gc[index] = always_affected_by_gc;
>  >  +}
>  >  +
>  >
>  >  as
>  >
>  >  +void GCMemoryManager::add_pool(MemoryPool* pool) {
>  >  +  add_pool(pool, true);
>  >  +}
>  >  +
>  >  +void GCMemoryManager::add_pool(MemoryPool* pool, bool
>  >  always_affected_by_gc) {
>  >  +  int index = MemoryManager::add_pool(pool);
>  >  +  _pool_always_affected_by_gc[index] = always_affected_by_gc;
>  >  +}
>  >  +
>  >
>  >  to not have to two duplicate implementations of
>  >  GCMemoryManager::add_pool. Would you mind updating the patch 
with this
>  >  change (and remove the trailing whitespace)?
>  >
>  >  Thanks,
>  >  Erik
>  >
>  >  > Thanks,
>  >  >
>  >  > Paul
>  >  >
>  >  > On 6/12/18, 6:52 AM, "Erik Helin"  
wrote:
>  >  >
>  >  >  (adding back serviceability-dev, please keep both 
hotspot-gc-dev and
>  >  >  serviceability-dev)
>  >  >
>  >  >  Hi Paul,
>  >  >
>  >  >  before I start re-reviewing, did you test the new 
version of the patch
>  >  >  via the jdk/submit repository [0]?
>  >  >
>  >  >  Thanks,
>  >  >  Erik
>  >  >
>  >  >  [0]: http://hg.openjdk.java.net/jdk/submit
>  >  >
>  >  >  On 06/09/2018 03:29 PM, Hohensee, Paul wrote:
>  >  >  > Didn't seem to make it to hotspot-gc-dev...
>  >  >  >
>  >  >  > On 6/8/18, 10:14 AM, "serviceability-dev on behalf 
of Hohensee, Paul"  wrote:
>  >  >  >
>  >  >  >  Back after a long hiatus...
>  >  >  >
>  >  >  >  Thanks, 

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[Erik Helin]

On 06/18/2018 06:14 PM, Hohensee, Paul wrote:

Thanks, Eric!

I'd push, but it seems I don't seem to have permission at the moment. Who 
should I contact to get that fixed?


That would be o...@openjdk.java.net.

Thanks,
Erik


Thanks,

Paul

On 6/18/18, 7:09 AM, "Erik Helin"  wrote:

 On 06/16/2018 09:00 PM, Hohensee, Paul wrote:
 > Thanks for the re-review, Erik. New webrev with your fixes:
 >
 > http://cr.openjdk.java.net/~phh/8195115/webrev.04/
 
 The patch is good to go now, Reviewed.
 
 Thanks,

 Erik
 
 > Need another reviewer, please.

 >
 > Thanks,
 >
 > Paul
 >
 > On 6/16/18, 1:25 AM, "Erik Helin"  wrote:
 >
 >  On 06/15/2018 10:21 PM, Hohensee, Paul wrote:
 >  > After some difficulty with the submit cluster, with which Erik 
helped me out, the patch passes. It also passed fastdebug hotspot tier 1 testing on 
my Mac laptop, which former includes the new test.
 >  >
 >  > I had to increase -Xmx and -Xms to 12m in order to get 
TestOldGenCollectionUsage to pass on the submit cluster, though the old 10m works 
fine on my Mac. New webrev:
 >
 >  Thanks, the change of -Xmx and -Xms to 12m now also makes the test 
pass
 >  on my workstation.
 >
 >  > http://cr.openjdk.java.net/~phh/8195115/webrev.03/
 >
 >  There seems to be some trailing whitespace in the patch, have you 
run
 >  jcheck (or `hg diff` which highlights trailing whitespace in red)?
 >  Please see
 >
 >  +TraceMemoryManagerStats tms(&_memory_manager, gc_cause(),
 >  +collector_state()->yc_type() == 
Mixed
 >  /* allMemoryPoolsAffected */);
 >  +
 >^ whitespace
 >
 >  and
 >
 >  +int MemoryManager::add_pool(MemoryPool* pool) {
 >  +  int index = _num_pools;
 > ^ whitespace
 >
 >  Another small comment, I would have written
 >
 >  +void GCMemoryManager::add_pool(MemoryPool* pool) {
 >  +  int index = MemoryManager::add_pool(pool);
 >  +  _pool_always_affected_by_gc[index] = true;
 >  +}
 >  +
 >  +void GCMemoryManager::add_pool(MemoryPool* pool, bool
 >  always_affected_by_gc) {
 >  +  int index = MemoryManager::add_pool(pool);
 >  +  _pool_always_affected_by_gc[index] = always_affected_by_gc;
 >  +}
 >  +
 >
 >  as
 >
 >  +void GCMemoryManager::add_pool(MemoryPool* pool) {
 >  +  add_pool(pool, true);
 >  +}
 >  +
 >  +void GCMemoryManager::add_pool(MemoryPool* pool, bool
 >  always_affected_by_gc) {
 >  +  int index = MemoryManager::add_pool(pool);
 >  +  _pool_always_affected_by_gc[index] = always_affected_by_gc;
 >  +}
 >  +
 >
 >  to not have to two duplicate implementations of
 >  GCMemoryManager::add_pool. Would you mind updating the patch with 
this
 >  change (and remove the trailing whitespace)?
 >
 >  Thanks,
 >  Erik
 >
 >  > Thanks,
 >  >
 >  > Paul
 >  >
 >  > On 6/12/18, 6:52 AM, "Erik Helin"  wrote:
 >  >
 >  >  (adding back serviceability-dev, please keep both 
hotspot-gc-dev and
 >  >  serviceability-dev)
 >  >
 >  >  Hi Paul,
 >  >
 >  >  before I start re-reviewing, did you test the new version of 
the patch
 >  >  via the jdk/submit repository [0]?
 >  >
 >  >  Thanks,
 >  >  Erik
 >  >
 >  >  [0]: http://hg.openjdk.java.net/jdk/submit
 >  >
 >  >  On 06/09/2018 03:29 PM, Hohensee, Paul wrote:
 >  >  > Didn't seem to make it to hotspot-gc-dev...
 >  >  >
 >  >  > On 6/8/18, 10:14 AM, "serviceability-dev on behalf of Hohensee, 
Paul"  wrote:
 >  >  >
 >  >  >  Back after a long hiatus...
 >  >  >
 >  >  >  Thanks, Eric, for your review. Here's a new webrev 
incorporating your recommendations.
 >  >  >
 >  >  >  Bug: https://bugs.openjdk.java.net/browse/JDK-8195115
 >  >  >  Webrev: 
http://cr.openjdk.java.net/~phh/8195115/webrev.02/
 >  >  >
 >  >  >  TIA for your re-review. Plus, may I have another 
reviewer look at it please?
 >  >  >
 >  >  >  Paul
 >  >  >
 >  >  >  On 2/26/18, 8:47 AM, "Erik Helin" 
 wrote:
 >  >  >
 >  >  >  Hi Paul,
 >  >  >
 >  >  >  a couple of comments on the patch:
 >  >  >
 >  >  >  - memoryService.hpp:
 >  >  

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[Hohensee, Paul]

Thanks, Eric!

I'd push, but it seems I don't seem to have permission at the moment. Who 
should I contact to get that fixed?

Thanks,

Paul

On 6/18/18, 7:09 AM, "Erik Helin"  wrote:

On 06/16/2018 09:00 PM, Hohensee, Paul wrote:
> Thanks for the re-review, Erik. New webrev with your fixes:
> 
> http://cr.openjdk.java.net/~phh/8195115/webrev.04/

The patch is good to go now, Reviewed.

Thanks,
Erik

> Need another reviewer, please.
> 
> Thanks,
> 
> Paul
> 
> On 6/16/18, 1:25 AM, "Erik Helin"  wrote:
> 
>  On 06/15/2018 10:21 PM, Hohensee, Paul wrote:
>  > After some difficulty with the submit cluster, with which Erik 
helped me out, the patch passes. It also passed fastdebug hotspot tier 1 
testing on my Mac laptop, which former includes the new test.
>  >
>  > I had to increase -Xmx and -Xms to 12m in order to get 
TestOldGenCollectionUsage to pass on the submit cluster, though the old 10m 
works fine on my Mac. New webrev:
>  
>  Thanks, the change of -Xmx and -Xms to 12m now also makes the test 
pass
>  on my workstation.
>  
>  > http://cr.openjdk.java.net/~phh/8195115/webrev.03/
>  
>  There seems to be some trailing whitespace in the patch, have you run
>  jcheck (or `hg diff` which highlights trailing whitespace in red)?
>  Please see
>  
>  +TraceMemoryManagerStats tms(&_memory_manager, gc_cause(),
>  +collector_state()->yc_type() == 
Mixed
>  /* allMemoryPoolsAffected */);
>  +
>^ whitespace
>  
>  and
>  
>  +int MemoryManager::add_pool(MemoryPool* pool) {
>  +  int index = _num_pools;
> ^ whitespace
>  
>  Another small comment, I would have written
>  
>  +void GCMemoryManager::add_pool(MemoryPool* pool) {
>  +  int index = MemoryManager::add_pool(pool);
>  +  _pool_always_affected_by_gc[index] = true;
>  +}
>  +
>  +void GCMemoryManager::add_pool(MemoryPool* pool, bool
>  always_affected_by_gc) {
>  +  int index = MemoryManager::add_pool(pool);
>  +  _pool_always_affected_by_gc[index] = always_affected_by_gc;
>  +}
>  +
>  
>  as
>  
>  +void GCMemoryManager::add_pool(MemoryPool* pool) {
>  +  add_pool(pool, true);
>  +}
>  +
>  +void GCMemoryManager::add_pool(MemoryPool* pool, bool
>  always_affected_by_gc) {
>  +  int index = MemoryManager::add_pool(pool);
>  +  _pool_always_affected_by_gc[index] = always_affected_by_gc;
>  +}
>  +
>  
>  to not have to two duplicate implementations of
>  GCMemoryManager::add_pool. Would you mind updating the patch with 
this
>  change (and remove the trailing whitespace)?
>  
>  Thanks,
>  Erik
>  
>  > Thanks,
>  >
>  > Paul
>  >
>  > On 6/12/18, 6:52 AM, "Erik Helin"  wrote:
>  >
>  >  (adding back serviceability-dev, please keep both 
hotspot-gc-dev and
>  >  serviceability-dev)
>  >
>  >  Hi Paul,
>  >
>  >  before I start re-reviewing, did you test the new version of 
the patch
>  >  via the jdk/submit repository [0]?
>  >
>  >  Thanks,
>  >  Erik
>  >
>  >  [0]: http://hg.openjdk.java.net/jdk/submit
>  >
>  >  On 06/09/2018 03:29 PM, Hohensee, Paul wrote:
>  >  > Didn't seem to make it to hotspot-gc-dev...
>  >  >
>  >  > On 6/8/18, 10:14 AM, "serviceability-dev on behalf of 
Hohensee, Paul"  wrote:
>  >  >
>  >  >  Back after a long hiatus...
>  >  >
>  >  >  Thanks, Eric, for your review. Here's a new webrev 
incorporating your recommendations.
>  >  >
>  >  >  Bug: https://bugs.openjdk.java.net/browse/JDK-8195115
>  >  >  Webrev: 
http://cr.openjdk.java.net/~phh/8195115/webrev.02/
>  >  >
>  >  >  TIA for your re-review. Plus, may I have another 
reviewer look at it please?
>  >  >
>  >  >  Paul
>  >  >
>  >  >  On 2/26/18, 8:47 AM, "Erik Helin" 
 wrote:
>  >  >
>  >  >  Hi Paul,
>  >  >
>  >  >  a couple of comments on the patch:
>  >  >
>  >  >  - memoryService.hpp:
>  >  > + 150   bool countCollection,
>  >  > + 151   bool 

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[Erik Helin]

On 06/16/2018 09:00 PM, Hohensee, Paul wrote:

Thanks for the re-review, Erik. New webrev with your fixes:

http://cr.openjdk.java.net/~phh/8195115/webrev.04/


The patch is good to go now, Reviewed.

Thanks,
Erik


Need another reviewer, please.

Thanks,

Paul

On 6/16/18, 1:25 AM, "Erik Helin"  wrote:

 On 06/15/2018 10:21 PM, Hohensee, Paul wrote:
 > After some difficulty with the submit cluster, with which Erik helped me 
out, the patch passes. It also passed fastdebug hotspot tier 1 testing on my Mac 
laptop, which former includes the new test.
 >
 > I had to increase -Xmx and -Xms to 12m in order to get 
TestOldGenCollectionUsage to pass on the submit cluster, though the old 10m works 
fine on my Mac. New webrev:
 
 Thanks, the change of -Xmx and -Xms to 12m now also makes the test pass

 on my workstation.
 
 > http://cr.openjdk.java.net/~phh/8195115/webrev.03/
 
 There seems to be some trailing whitespace in the patch, have you run

 jcheck (or `hg diff` which highlights trailing whitespace in red)?
 Please see
 
 +TraceMemoryManagerStats tms(&_memory_manager, gc_cause(),

 +collector_state()->yc_type() == Mixed
 /* allMemoryPoolsAffected */);
 +
   ^ whitespace
 
 and
 
 +int MemoryManager::add_pool(MemoryPool* pool) {

 +  int index = _num_pools;
^ whitespace
 
 Another small comment, I would have written
 
 +void GCMemoryManager::add_pool(MemoryPool* pool) {

 +  int index = MemoryManager::add_pool(pool);
 +  _pool_always_affected_by_gc[index] = true;
 +}
 +
 +void GCMemoryManager::add_pool(MemoryPool* pool, bool
 always_affected_by_gc) {
 +  int index = MemoryManager::add_pool(pool);
 +  _pool_always_affected_by_gc[index] = always_affected_by_gc;
 +}
 +
 
 as
 
 +void GCMemoryManager::add_pool(MemoryPool* pool) {

 +  add_pool(pool, true);
 +}
 +
 +void GCMemoryManager::add_pool(MemoryPool* pool, bool
 always_affected_by_gc) {
 +  int index = MemoryManager::add_pool(pool);
 +  _pool_always_affected_by_gc[index] = always_affected_by_gc;
 +}
 +
 
 to not have to two duplicate implementations of

 GCMemoryManager::add_pool. Would you mind updating the patch with this
 change (and remove the trailing whitespace)?
 
 Thanks,

 Erik
 
 > Thanks,

 >
 > Paul
 >
 > On 6/12/18, 6:52 AM, "Erik Helin"  wrote:
 >
 >  (adding back serviceability-dev, please keep both hotspot-gc-dev and
 >  serviceability-dev)
 >
 >  Hi Paul,
 >
 >  before I start re-reviewing, did you test the new version of the 
patch
 >  via the jdk/submit repository [0]?
 >
 >  Thanks,
 >  Erik
 >
 >  [0]: http://hg.openjdk.java.net/jdk/submit
 >
 >  On 06/09/2018 03:29 PM, Hohensee, Paul wrote:
 >  > Didn't seem to make it to hotspot-gc-dev...
 >  >
 >  > On 6/8/18, 10:14 AM, "serviceability-dev on behalf of Hohensee, Paul" 
 wrote:
 >  >
 >  >  Back after a long hiatus...
 >  >
 >  >  Thanks, Eric, for your review. Here's a new webrev 
incorporating your recommendations.
 >  >
 >  >  Bug: https://bugs.openjdk.java.net/browse/JDK-8195115
 >  >  Webrev: http://cr.openjdk.java.net/~phh/8195115/webrev.02/
 >  >
 >  >  TIA for your re-review. Plus, may I have another reviewer 
look at it please?
 >  >
 >  >  Paul
 >  >
 >  >  On 2/26/18, 8:47 AM, "Erik Helin"  
wrote:
 >  >
 >  >  Hi Paul,
 >  >
 >  >  a couple of comments on the patch:
 >  >
 >  >  - memoryService.hpp:
 >  > + 150   bool countCollection,
 >  > + 151   bool allMemoryPoolsAffected = 
true);
 >  >
 >  > There is no need to use a default value for the 
parameter
 >  > allMemoryPoolsAffected here. Skipping the default 
value also allows
 >  > you to put allMemoryPoolsAffected to 
TraceMemoryManager::initialize
 >  > in the same relative position as for the constructor 
parameter (this
 >  > will make the code more uniform and easier to follow).
 >  >
 >  >  - memoryManager.cpp
 >  >
 >  > Instead of adding a default parameter, maybe add a 
new method?
 >  > Something like 
GCMemoryManager::add_not_always_affected_pool()
 >  > (I couldn't come up with a shorter name at the 
moment).
 >  >
 >  >  - TestMixedOldGenCollectionUsage.java
 >

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[mandy chung]

Looks fine to me.

Mandy

On 6/16/18 12:00 PM, Hohensee, Paul wrote:

Thanks for the re-review, Erik. New webrev with your fixes:

http://cr.openjdk.java.net/~phh/8195115/webrev.04/

Need another reviewer, please.

Thanks,

Paul

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[Hohensee, Paul]

Thanks for the re-review, Erik. New webrev with your fixes:

http://cr.openjdk.java.net/~phh/8195115/webrev.04/

Need another reviewer, please.

Thanks,

Paul

On 6/16/18, 1:25 AM, "Erik Helin"  wrote:

On 06/15/2018 10:21 PM, Hohensee, Paul wrote:
> After some difficulty with the submit cluster, with which Erik helped me 
out, the patch passes. It also passed fastdebug hotspot tier 1 testing on my 
Mac laptop, which former includes the new test.
> 
> I had to increase -Xmx and -Xms to 12m in order to get 
TestOldGenCollectionUsage to pass on the submit cluster, though the old 10m 
works fine on my Mac. New webrev:

Thanks, the change of -Xmx and -Xms to 12m now also makes the test pass 
on my workstation.

> http://cr.openjdk.java.net/~phh/8195115/webrev.03/

There seems to be some trailing whitespace in the patch, have you run 
jcheck (or `hg diff` which highlights trailing whitespace in red)? 
Please see

+TraceMemoryManagerStats tms(&_memory_manager, gc_cause(),
+collector_state()->yc_type() == Mixed 
/* allMemoryPoolsAffected */);
+
  ^ whitespace

and

+int MemoryManager::add_pool(MemoryPool* pool) {
+  int index = _num_pools;
   ^ whitespace

Another small comment, I would have written

+void GCMemoryManager::add_pool(MemoryPool* pool) {
+  int index = MemoryManager::add_pool(pool);
+  _pool_always_affected_by_gc[index] = true;
+}
+
+void GCMemoryManager::add_pool(MemoryPool* pool, bool 
always_affected_by_gc) {
+  int index = MemoryManager::add_pool(pool);
+  _pool_always_affected_by_gc[index] = always_affected_by_gc;
+}
+

as

+void GCMemoryManager::add_pool(MemoryPool* pool) {
+  add_pool(pool, true);
+}
+
+void GCMemoryManager::add_pool(MemoryPool* pool, bool 
always_affected_by_gc) {
+  int index = MemoryManager::add_pool(pool);
+  _pool_always_affected_by_gc[index] = always_affected_by_gc;
+}
+

to not have to two duplicate implementations of 
GCMemoryManager::add_pool. Would you mind updating the patch with this 
change (and remove the trailing whitespace)?

Thanks,
Erik

> Thanks,
> 
> Paul
> 
> On 6/12/18, 6:52 AM, "Erik Helin"  wrote:
> 
>  (adding back serviceability-dev, please keep both hotspot-gc-dev and
>  serviceability-dev)
>  
>  Hi Paul,
>  
>  before I start re-reviewing, did you test the new version of the 
patch
>  via the jdk/submit repository [0]?
>  
>  Thanks,
>  Erik
>  
>  [0]: http://hg.openjdk.java.net/jdk/submit
>  
>  On 06/09/2018 03:29 PM, Hohensee, Paul wrote:
>  > Didn't seem to make it to hotspot-gc-dev...
>  >
>  > On 6/8/18, 10:14 AM, "serviceability-dev on behalf of Hohensee, 
Paul"  wrote:
>  >
>  >  Back after a long hiatus...
>  >
>  >  Thanks, Eric, for your review. Here's a new webrev 
incorporating your recommendations.
>  >
>  >  Bug: https://bugs.openjdk.java.net/browse/JDK-8195115
>  >  Webrev: http://cr.openjdk.java.net/~phh/8195115/webrev.02/
>  >
>  >  TIA for your re-review. Plus, may I have another reviewer 
look at it please?
>  >
>  >  Paul
>  >
>  >  On 2/26/18, 8:47 AM, "Erik Helin"  
wrote:
>  >
>  >  Hi Paul,
>  >
>  >  a couple of comments on the patch:
>  >
>  >  - memoryService.hpp:
>  > + 150   bool countCollection,
>  > + 151   bool allMemoryPoolsAffected = 
true);
>  >
>  > There is no need to use a default value for the 
parameter
>  > allMemoryPoolsAffected here. Skipping the default 
value also allows
>  > you to put allMemoryPoolsAffected to 
TraceMemoryManager::initialize
>  > in the same relative position as for the constructor 
parameter (this
>  > will make the code more uniform and easier to follow).
>  >
>  >  - memoryManager.cpp
>  >
>  > Instead of adding a default parameter, maybe add a new 
method?
>  > Something like 
GCMemoryManager::add_not_always_affected_pool()
>  > (I couldn't come up with a shorter name at the moment).
>  >
>  >  - TestMixedOldGenCollectionUsage.java
>  >
>  > The test is too strict about how and when collections 
should
>  > occur. Tests written this way often become very 
brittle, they might
> 

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[Erik Helin]

On 06/15/2018 10:21 PM, Hohensee, Paul wrote:

After some difficulty with the submit cluster, with which Erik helped me out, 
the patch passes. It also passed fastdebug hotspot tier 1 testing on my Mac 
laptop, which former includes the new test.

I had to increase -Xmx and -Xms to 12m in order to get 
TestOldGenCollectionUsage to pass on the submit cluster, though the old 10m 
works fine on my Mac. New webrev:


Thanks, the change of -Xmx and -Xms to 12m now also makes the test pass 
on my workstation.



http://cr.openjdk.java.net/~phh/8195115/webrev.03/


There seems to be some trailing whitespace in the patch, have you run 
jcheck (or `hg diff` which highlights trailing whitespace in red)? 
Please see


+TraceMemoryManagerStats tms(&_memory_manager, gc_cause(),
+collector_state()->yc_type() == Mixed 
/* allMemoryPoolsAffected */);

+
 ^ whitespace

and

+int MemoryManager::add_pool(MemoryPool* pool) {
+  int index = _num_pools;
  ^ whitespace

Another small comment, I would have written

+void GCMemoryManager::add_pool(MemoryPool* pool) {
+  int index = MemoryManager::add_pool(pool);
+  _pool_always_affected_by_gc[index] = true;
+}
+
+void GCMemoryManager::add_pool(MemoryPool* pool, bool 
always_affected_by_gc) {

+  int index = MemoryManager::add_pool(pool);
+  _pool_always_affected_by_gc[index] = always_affected_by_gc;
+}
+

as

+void GCMemoryManager::add_pool(MemoryPool* pool) {
+  add_pool(pool, true);
+}
+
+void GCMemoryManager::add_pool(MemoryPool* pool, bool 
always_affected_by_gc) {

+  int index = MemoryManager::add_pool(pool);
+  _pool_always_affected_by_gc[index] = always_affected_by_gc;
+}
+

to not have to two duplicate implementations of 
GCMemoryManager::add_pool. Would you mind updating the patch with this 
change (and remove the trailing whitespace)?


Thanks,
Erik


Thanks,

Paul

On 6/12/18, 6:52 AM, "Erik Helin"  wrote:

 (adding back serviceability-dev, please keep both hotspot-gc-dev and
 serviceability-dev)
 
 Hi Paul,
 
 before I start re-reviewing, did you test the new version of the patch

 via the jdk/submit repository [0]?
 
 Thanks,

 Erik
 
 [0]: http://hg.openjdk.java.net/jdk/submit
 
 On 06/09/2018 03:29 PM, Hohensee, Paul wrote:

 > Didn't seem to make it to hotspot-gc-dev...
 >
 > On 6/8/18, 10:14 AM, "serviceability-dev on behalf of Hohensee, Paul" 
 wrote:
 >
 >  Back after a long hiatus...
 >
 >  Thanks, Eric, for your review. Here's a new webrev incorporating 
your recommendations.
 >
 >  Bug: https://bugs.openjdk.java.net/browse/JDK-8195115
 >  Webrev: http://cr.openjdk.java.net/~phh/8195115/webrev.02/
 >
 >  TIA for your re-review. Plus, may I have another reviewer look at 
it please?
 >
 >  Paul
 >
 >  On 2/26/18, 8:47 AM, "Erik Helin"  wrote:
 >
 >  Hi Paul,
 >
 >  a couple of comments on the patch:
 >
 >  - memoryService.hpp:
 > + 150   bool countCollection,
 > + 151   bool allMemoryPoolsAffected = true);
 >
 > There is no need to use a default value for the parameter
 > allMemoryPoolsAffected here. Skipping the default value also 
allows
 > you to put allMemoryPoolsAffected to 
TraceMemoryManager::initialize
 > in the same relative position as for the constructor 
parameter (this
 > will make the code more uniform and easier to follow).
 >
 >  - memoryManager.cpp
 >
 > Instead of adding a default parameter, maybe add a new 
method?
 > Something like 
GCMemoryManager::add_not_always_affected_pool()
 > (I couldn't come up with a shorter name at the moment).
 >
 >  - TestMixedOldGenCollectionUsage.java
 >
 > The test is too strict about how and when collections should
 > occur. Tests written this way often become very brittle, 
they might
 > e.g. fail to finish a concurrent mark on time on a very 
slow, single
 > core, machine. It is better to either force collections by 
using the
 > WhiteBox API or make the test more lenient.
 >
 >  Thanks,
 >  Erik
 >
 >  On 02/22/2018 09:54 PM, Hohensee, Paul wrote:
 >  > Ping for a review please.
 >  >
 >  > Thanks,
 >  >
 >  > Paul
 >  >
 >  > On 2/16/18, 12:26 PM, "serviceability-dev on behalf of Hohensee, 
Paul"  wrote:
 >  >
 >  >  The CSR https://bugs.openjdk.java.net/browse/JDK-8196719 
for the original fix has been approved, so I’m back to requesting a code review, 
please.
 >  >
 

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[Hohensee, Paul]

After some difficulty with the submit cluster, with which Erik helped me out, 
the patch passes. It also passed fastdebug hotspot tier 1 testing on my Mac 
laptop, which former includes the new test.

I had to increase -Xmx and -Xms to 12m in order to get 
TestOldGenCollectionUsage to pass on the submit cluster, though the old 10m 
works fine on my Mac. New webrev:

http://cr.openjdk.java.net/~phh/8195115/webrev.03/

Thanks,

Paul

On 6/12/18, 6:52 AM, "Erik Helin"  wrote:

(adding back serviceability-dev, please keep both hotspot-gc-dev and 
serviceability-dev)

Hi Paul,

before I start re-reviewing, did you test the new version of the patch 
via the jdk/submit repository [0]?

Thanks,
Erik

[0]: http://hg.openjdk.java.net/jdk/submit

On 06/09/2018 03:29 PM, Hohensee, Paul wrote:
> Didn't seem to make it to hotspot-gc-dev...
> 
> On 6/8/18, 10:14 AM, "serviceability-dev on behalf of Hohensee, Paul" 
 
wrote:
> 
>  Back after a long hiatus...
>  
>  Thanks, Eric, for your review. Here's a new webrev incorporating 
your recommendations.
>  
>  Bug: https://bugs.openjdk.java.net/browse/JDK-8195115
>  Webrev: http://cr.openjdk.java.net/~phh/8195115/webrev.02/
>  
>  TIA for your re-review. Plus, may I have another reviewer look at it 
please?
>  
>  Paul
>  
>  On 2/26/18, 8:47 AM, "Erik Helin"  wrote:
>  
>  Hi Paul,
>  
>  a couple of comments on the patch:
>  
>  - memoryService.hpp:
> + 150   bool countCollection,
> + 151   bool allMemoryPoolsAffected = true);
>  
> There is no need to use a default value for the parameter
> allMemoryPoolsAffected here. Skipping the default value also 
allows
> you to put allMemoryPoolsAffected to 
TraceMemoryManager::initialize
> in the same relative position as for the constructor 
parameter (this
> will make the code more uniform and easier to follow).
>  
>  - memoryManager.cpp
>  
> Instead of adding a default parameter, maybe add a new method?
> Something like GCMemoryManager::add_not_always_affected_pool()
> (I couldn't come up with a shorter name at the moment).
>  
>  - TestMixedOldGenCollectionUsage.java
>  
> The test is too strict about how and when collections should
> occur. Tests written this way often become very brittle, they 
might
> e.g. fail to finish a concurrent mark on time on a very slow, 
single
> core, machine. It is better to either force collections by 
using the
> WhiteBox API or make the test more lenient.
>  
>  Thanks,
>  Erik
>  
>  On 02/22/2018 09:54 PM, Hohensee, Paul wrote:
>  > Ping for a review please.
>  >
>  > Thanks,
>  >
>  > Paul
>  >
>  > On 2/16/18, 12:26 PM, "serviceability-dev on behalf of 
Hohensee, Paul"  wrote:
>  >
>  >  The CSR https://bugs.openjdk.java.net/browse/JDK-8196719 
for the original fix has been approved, so I’m back to requesting a code 
review, please.
>  >
>  >  Bug: https://bugs.openjdk.java.net/browse/JDK-8195115
>  >  Webrev: 
http://cr.openjdk.java.net/~phh/8195115/webrev.hs.01/
>  >
>  >  Passed a submit repo run, passes its jtreg test, and a 
JDK8 version is in production use at Amazon.
>  >
>  >  From the original RR:
>  >
>  >  > The bug is that from the JMX point of view, G1’s 
incremental collector
>  >  > (misnamed as the “G1 Young Generation” collector) 
only affects G1’s
>  >  > survivor and eden spaces. In fact, mixed 
collections run by this
>  >  > collector also affect the G1 old generation.
>  >  >
>  >  > This proposed fix is to record, for each of a JMX 
garbage collector's
>  >  > memory pools, whether that memory pool is affected 
by all collections
>  >  > using that collector. And, for each collection, 
record whether or not
>  >  > all the collector's memory pools are affected. 
After each collection,
>  >  > for each memory pool, if either all the collector's 
memory pools were
>  >  > affected or the memory pool is affected for all 
collections, record
>  >  > CollectionUsage for 

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[Hohensee, Paul]

Back after a long hiatus...

Thanks, Eric, for your review. Here's a new webrev incorporating your 
recommendations.

Bug: https://bugs.openjdk.java.net/browse/JDK-8195115
Webrev: http://cr.openjdk.java.net/~phh/8195115/webrev.02/

TIA for your re-review. Plus, may I have another reviewer look at it please?

Paul

On 2/26/18, 8:47 AM, "Erik Helin"  wrote:

Hi Paul,

a couple of comments on the patch:

- memoryService.hpp:
   + 150   bool countCollection,
   + 151   bool allMemoryPoolsAffected = true);

   There is no need to use a default value for the parameter
   allMemoryPoolsAffected here. Skipping the default value also allows
   you to put allMemoryPoolsAffected to TraceMemoryManager::initialize
   in the same relative position as for the constructor parameter (this
   will make the code more uniform and easier to follow).

- memoryManager.cpp

   Instead of adding a default parameter, maybe add a new method?
   Something like GCMemoryManager::add_not_always_affected_pool()
   (I couldn't come up with a shorter name at the moment).

- TestMixedOldGenCollectionUsage.java

   The test is too strict about how and when collections should
   occur. Tests written this way often become very brittle, they might
   e.g. fail to finish a concurrent mark on time on a very slow, single
   core, machine. It is better to either force collections by using the
   WhiteBox API or make the test more lenient.

Thanks,
Erik

On 02/22/2018 09:54 PM, Hohensee, Paul wrote:
> Ping for a review please.
> 
> Thanks,
> 
> Paul
> 
> On 2/16/18, 12:26 PM, "serviceability-dev on behalf of Hohensee, Paul" 
 
wrote:
> 
>  The CSR https://bugs.openjdk.java.net/browse/JDK-8196719 for the 
original fix has been approved, so I’m back to requesting a code review, please.
>  
>  Bug: https://bugs.openjdk.java.net/browse/JDK-8195115
>  Webrev: http://cr.openjdk.java.net/~phh/8195115/webrev.hs.01/
>  
>  Passed a submit repo run, passes its jtreg test, and a JDK8 version 
is in production use at Amazon.
>  
>  From the original RR:
>  
>  > The bug is that from the JMX point of view, G1’s incremental 
collector
>  > (misnamed as the “G1 Young Generation” collector) only affects 
G1’s
>  > survivor and eden spaces. In fact, mixed collections run by 
this
>  > collector also affect the G1 old generation.
>  >
>  > This proposed fix is to record, for each of a JMX garbage 
collector's
>  > memory pools, whether that memory pool is affected by all 
collections
>  > using that collector. And, for each collection, record whether 
or not
>  > all the collector's memory pools are affected. After each 
collection,
>  > for each memory pool, if either all the collector's memory 
pools were
>  > affected or the memory pool is affected for all collections, 
record
>  > CollectionUsage for that pool.
>  >
>  > For collectors other than G1 Young Generation, all pools are 
recorded as
>  > affected by all collections and every collection is recorded as
>  > affecting all the collector’s memory pools. For the G1 Young 
Generation
>  > collector, the G1 Old Gen pool is recorded as not being 
affected by all
>  > collections, and non-mixed collections are recorded as not 
affecting all
>  > memory pools. The result is that for non-mixed collections,
>  > CollectionUsage is recorded after a collection only the G1 
Eden Space
>  > and G1 Survivor Space pools, while for mixed collections 
CollectionUsage
>  > is recorded for G1 Old Gen as well.
>  >
>  > Other than the effect of the fix on G1 Old Gen MemoryPool.
>  > CollectionUsage, the only external behavior change is that
>  > GarbageCollectorMXBean.getMemoryPoolNames will now return 3 
pool names
>  > rather than 2.
>  >
>  > With this fix, a collector’s memory pools can be divided into 
two
>  > disjoint subsets, one that participates in all collections and 
one that
>  > doesn’t. This is a bit more general than the minimum necessary 
to fix
>  > G1, but not by much. Because I expect it to apply to other 
incremental
>  > region-based collectors, I went with the more general 
solution. I
>  > minimized the amount of code I had to touch by using default 
parameters
>  > for GCMemoryManager::add_pool and the TraceMemoryManagerStats 
constructors.
> 
> 
> 

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[Erik Helin]

Hi Paul,

a couple of comments on the patch:

- memoryService.hpp:
  + 150   bool countCollection,
  + 151   bool allMemoryPoolsAffected = true);

  There is no need to use a default value for the parameter
  allMemoryPoolsAffected here. Skipping the default value also allows
  you to put allMemoryPoolsAffected to TraceMemoryManager::initialize
  in the same relative position as for the constructor parameter (this
  will make the code more uniform and easier to follow).

- memoryManager.cpp

  Instead of adding a default parameter, maybe add a new method?
  Something like GCMemoryManager::add_not_always_affected_pool()
  (I couldn't come up with a shorter name at the moment).

- TestMixedOldGenCollectionUsage.java

  The test is too strict about how and when collections should
  occur. Tests written this way often become very brittle, they might
  e.g. fail to finish a concurrent mark on time on a very slow, single
  core, machine. It is better to either force collections by using the
  WhiteBox API or make the test more lenient.

Thanks,
Erik

On 02/22/2018 09:54 PM, Hohensee, Paul wrote:

Ping for a review please.

Thanks,

Paul

On 2/16/18, 12:26 PM, "serviceability-dev on behalf of Hohensee, Paul" 
 wrote:

 The CSR https://bugs.openjdk.java.net/browse/JDK-8196719 for the original 
fix has been approved, so I’m back to requesting a code review, please.
 
 Bug: https://bugs.openjdk.java.net/browse/JDK-8195115

 Webrev: http://cr.openjdk.java.net/~phh/8195115/webrev.hs.01/
 
 Passed a submit repo run, passes its jtreg test, and a JDK8 version is in production use at Amazon.
 
 From the original RR:
 
 > The bug is that from the JMX point of view, G1’s incremental collector

 > (misnamed as the “G1 Young Generation” collector) only affects G1’s
 > survivor and eden spaces. In fact, mixed collections run by this
 > collector also affect the G1 old generation.
 >
 > This proposed fix is to record, for each of a JMX garbage collector's
 > memory pools, whether that memory pool is affected by all collections
 > using that collector. And, for each collection, record whether or not
 > all the collector's memory pools are affected. After each collection,
 > for each memory pool, if either all the collector's memory pools were
 > affected or the memory pool is affected for all collections, record
 > CollectionUsage for that pool.
 >
 > For collectors other than G1 Young Generation, all pools are 
recorded as
 > affected by all collections and every collection is recorded as
 > affecting all the collector’s memory pools. For the G1 Young 
Generation
 > collector, the G1 Old Gen pool is recorded as not being affected by 
all
 > collections, and non-mixed collections are recorded as not affecting 
all
 > memory pools. The result is that for non-mixed collections,
 > CollectionUsage is recorded after a collection only the G1 Eden Space
 > and G1 Survivor Space pools, while for mixed collections 
CollectionUsage
 > is recorded for G1 Old Gen as well.
 >
 > Other than the effect of the fix on G1 Old Gen MemoryPool.
 > CollectionUsage, the only external behavior change is that
 > GarbageCollectorMXBean.getMemoryPoolNames will now return 3 pool 
names
 > rather than 2.
 >
 > With this fix, a collector’s memory pools can be divided into two
 > disjoint subsets, one that participates in all collections and one 
that
 > doesn’t. This is a bit more general than the minimum necessary to fix
 > G1, but not by much. Because I expect it to apply to other 
incremental
 > region-based collectors, I went with the more general solution. I
 > minimized the amount of code I had to touch by using default 
parameters
 > for GCMemoryManager::add_pool and the TraceMemoryManagerStats 
constructors.

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[Hohensee, Paul]

Ping for a review please.

Thanks,

Paul

On 2/16/18, 12:26 PM, "serviceability-dev on behalf of Hohensee, Paul" 
 
wrote:

The CSR https://bugs.openjdk.java.net/browse/JDK-8196719 for the original 
fix has been approved, so I’m back to requesting a code review, please.

Bug: https://bugs.openjdk.java.net/browse/JDK-8195115
Webrev: http://cr.openjdk.java.net/~phh/8195115/webrev.hs.01/

Passed a submit repo run, passes its jtreg test, and a JDK8 version is in 
production use at Amazon.

From the original RR:

> The bug is that from the JMX point of view, G1’s incremental collector
> (misnamed as the “G1 Young Generation” collector) only affects G1’s
> survivor and eden spaces. In fact, mixed collections run by this
> collector also affect the G1 old generation.
>
> This proposed fix is to record, for each of a JMX garbage collector's
> memory pools, whether that memory pool is affected by all collections
> using that collector. And, for each collection, record whether or not
> all the collector's memory pools are affected. After each collection,
> for each memory pool, if either all the collector's memory pools were
> affected or the memory pool is affected for all collections, record
> CollectionUsage for that pool.
>
> For collectors other than G1 Young Generation, all pools are recorded 
as
> affected by all collections and every collection is recorded as
> affecting all the collector’s memory pools. For the G1 Young 
Generation
> collector, the G1 Old Gen pool is recorded as not being affected by 
all
> collections, and non-mixed collections are recorded as not affecting 
all
> memory pools. The result is that for non-mixed collections,
> CollectionUsage is recorded after a collection only the G1 Eden Space
> and G1 Survivor Space pools, while for mixed collections 
CollectionUsage
> is recorded for G1 Old Gen as well.
>
> Other than the effect of the fix on G1 Old Gen MemoryPool.
> CollectionUsage, the only external behavior change is that
> GarbageCollectorMXBean.getMemoryPoolNames will now return 3 pool names
> rather than 2.
>
> With this fix, a collector’s memory pools can be divided into two
> disjoint subsets, one that participates in all collections and one 
that
> doesn’t. This is a bit more general than the minimum necessary to fix
> G1, but not by much. Because I expect it to apply to other incremental
> region-based collectors, I went with the more general solution. I
> minimized the amount of code I had to touch by using default 
parameters
> for GCMemoryManager::add_pool and the TraceMemoryManagerStats 
constructors.

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[Hohensee, Paul]

perm gen pool was removed in JDK 8, the default pools 
changed when
>  Parallel Old became default old collector way back in 
JDK 7 and changed
>  again when G1 became the default collector in JDK 9.
>  
>  Thanks,
>  Erik
>  
>  > Thanks,
>  >
>  > Paul
>  >
>  > On 1/30/18, 5:51 AM, "Erik Helin" 
<erik.he...@oracle.com> wrote:
>  >
>  >  On 01/30/2018 03:07 AM, Hohensee, Paul wrote:
>  >  > That’s one reviewer who’s ok with a short term 
patch. Anyone else? And,
>  >  > any reviewers for said short term patch? :)
>  >
>  >  Well, the patch is not really complete as it is. 
The problem is the
>  >  definitions of the MemoryPoolMXBeans and 
GarbageCollectorMXBeans, which,
>  >  as I tried to hint at in my first email, is a 
mess for G1. The names and
>  >  implementations of these MemoryPoolMXBeans and 
GarbageCollectionMXBeans
>  >  for G1 are very old, G1 has changed a lot since 
those were implemented
>  >  (hence my suggestion for finally fixing this).
>  >
>  >  The issue with your patch is that the 
MemoryPoolMXBean named "G1 Old
>  >  Gen" consists of both old and humongous regions 
(it will also include
>  >  archive regions). Old regions can be collected by 
mixed, concurrent and
>  >  full collections. Humongous regions can be 
collected by young, mixed or
>  >  full collections and the concurrent cycle. 
Archive regions will never be
>  >  collected. Your patch will update the pool in the 
case of a mixed
>  >  collection collecting old regions or humongous 
regions, but misses the
>  >  following cases:
>  >  - a young collection collecting humongous regions
>  >  - a concurrent cycle collecting humongous regions
>  >  - a concurrent cycle collecting old regions
>  >
>  >  Unfortunately I could not come up with a good way 
to solve the above
>  >  without re-designing the pools. I'm not sure 
about accepting your patch
>  >  as is, since it might cause even more confusion 
for users compared to
>  >  the current (already confusing) situation.
>  >
>  >  One idea we have discussed is to implement the 
re-design but also add a
>  >  flag, -XX:+UseG1LegacyPoolsAndBeans (false by 
default), to allow for a
>  >  smoother transition. Would that solution work for 
you?
>  >
>  >  Thanks,
>  >  Erik
>  >
>  >  > Thanks,
>  >  >
>  >  > Paul
>  >  >
>  >  > *From: *mandy chung <mandy.ch...@oracle.com>
>  >  > *Organization: *Oracle Corporation
>  >  > *Date: *Monday, January 29, 2018 at 1:41 PM
>  >  > *To: *"Hohensee, Paul" <hohen...@amazon.com>
>  >  > *Cc: *"serviceability-dev@openjdk.java.net"
>  >  > <serviceability-dev@openjdk.java.net>, 
"hotspot-gc-...@openjdk.java.net"
>  >  > <hotspot-gc-...@openjdk.java.net>
>  >  > *Subject: *Re: RFR (S): 8195115: G1 Old Gen 
MemoryPool
>  >  > CollectionUsage.used values don't reflect mixed 
GC results
>  >  >
>  >  > I created  JDK-8196362 to look into whether it 
makes sense to provide
>  >  > some categorization to differentiate eden space 
vs the heap space for
>  >  > long-lived objects.
>  >  >
>

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[Hohensee, Paul]

 
see humongous regions. Monitoring apps that just add up info about all a 
collector’s pools won’t see a difference. I may be corrected (by Kirk, 
perhaps), but imo it’s not as bad a compatibility issue as one might think, 
because the type of app that uses a lot of humongous regions isn’t all that 
common. E.g., apps that cache data in the heap in the form of large compressed 
arrays, and apps with large hashmap bucket list arrays. The heaps such apps use 
are very often large enough to use 32mb regions, hence need really big objects 
to actually allocate humongous regions.
>  
>  So why not enable backwards compatibility by allowing a user 
to set the
>  flag -XX:+UseG1LegacyPoolsAndBeans? It is not that 
cumbersome for us to
>  maintain the current definition of memory pools and 
collectors. Such a
>  flag allows us to start over and do this right and a user 
who relies on
>  the current behavior can get that by just setting a flag. 
Doing such a
>  change in a major release also allows us to clearly 
highlight the change
>  in the release notes (users are more prepared for larger 
changes in a
>  major release and that they might have to add flags to keep 
old behavior).
>  
>  It is not uncommon for memory pools to change in major 
releases. The
>  perm gen pool was removed in JDK 8, the default pools 
changed when
>  Parallel Old became default old collector way back in JDK 7 
and changed
>  again when G1 became the default collector in JDK 9.
>  
>  Thanks,
>  Erik
>  
>  > Thanks,
>  >
>  > Paul
>  >
>  > On 1/30/18, 5:51 AM, "Erik Helin" <erik.he...@oracle.com> 
wrote:
>  >
>  >  On 01/30/2018 03:07 AM, Hohensee, Paul wrote:
>  >  > That’s one reviewer who’s ok with a short term 
patch. Anyone else? And,
>  >  > any reviewers for said short term patch? :)
>  >
>  >  Well, the patch is not really complete as it is. The 
problem is the
>  >  definitions of the MemoryPoolMXBeans and 
GarbageCollectorMXBeans, which,
>  >  as I tried to hint at in my first email, is a mess 
for G1. The names and
>  >  implementations of these MemoryPoolMXBeans and 
GarbageCollectionMXBeans
>  >  for G1 are very old, G1 has changed a lot since those 
were implemented
>  >  (hence my suggestion for finally fixing this).
>  >
>  >  The issue with your patch is that the 
MemoryPoolMXBean named "G1 Old
>  >  Gen" consists of both old and humongous regions (it 
will also include
>  >  archive regions). Old regions can be collected by 
mixed, concurrent and
>  >  full collections. Humongous regions can be collected 
by young, mixed or
>  >  full collections and the concurrent cycle. Archive 
regions will never be
>  >  collected. Your patch will update the pool in the 
case of a mixed
>  >  collection collecting old regions or humongous 
regions, but misses the
>  >  following cases:
>  >  - a young collection collecting humongous regions
>  >  - a concurrent cycle collecting humongous regions
>  >  - a concurrent cycle collecting old regions
>  >
>  >  Unfortunately I could not come up with a good way to 
solve the above
>  >  without re-designing the pools. I'm not sure about 
accepting your patch
>  >  as is, since it might cause even more confusion for 
users compared to
>  >  the current (already confusing) situation.
>  >
>  >  One idea we have discussed is to implement the 
re-design but also add a
    >          >      flag, -XX:+UseG1LegacyPoolsAndBeans (false by 
default), to allow for a
>      >  smoother transition. Would that solution work for you?
>  >
>  >  Thanks,
>  >  Erik
>  >
>  >  > Thanks,
>  >  &g

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[Erik Helin]

vior).
 
 It is not uncommon for memory pools to change in major releases. The

 perm gen pool was removed in JDK 8, the default pools changed when
 Parallel Old became default old collector way back in JDK 7 and 
changed
 again when G1 became the default collector in JDK 9.
 
 Thanks,

 Erik
 
 > Thanks,

 >
 > Paul
 >
 > On 1/30/18, 5:51 AM, "Erik Helin" <erik.he...@oracle.com> wrote:
 >
 >  On 01/30/2018 03:07 AM, Hohensee, Paul wrote:
 >  > That’s one reviewer who’s ok with a short term patch. 
Anyone else? And,
 >  > any reviewers for said short term patch? :)
 >
 >  Well, the patch is not really complete as it is. The 
problem is the
 >  definitions of the MemoryPoolMXBeans and 
GarbageCollectorMXBeans, which,
 >  as I tried to hint at in my first email, is a mess for G1. 
The names and
 >  implementations of these MemoryPoolMXBeans and 
GarbageCollectionMXBeans
 >  for G1 are very old, G1 has changed a lot since those were 
implemented
 >  (hence my suggestion for finally fixing this).
 >
 >  The issue with your patch is that the MemoryPoolMXBean named 
"G1 Old
 >  Gen" consists of both old and humongous regions (it will 
also include
 >  archive regions). Old regions can be collected by mixed, 
concurrent and
 >  full collections. Humongous regions can be collected by 
young, mixed or
 >  full collections and the concurrent cycle. Archive regions 
will never be
 >  collected. Your patch will update the pool in the case of a 
mixed
 >  collection collecting old regions or humongous regions, but 
misses the
 >  following cases:
 >  - a young collection collecting humongous regions
 >  - a concurrent cycle collecting humongous regions
 >  - a concurrent cycle collecting old regions
 >
 >  Unfortunately I could not come up with a good way to solve 
the above
 >  without re-designing the pools. I'm not sure about 
accepting your patch
 >  as is, since it might cause even more confusion for users 
compared to
 >  the current (already confusing) situation.
 >
 >  One idea we have discussed is to implement the re-design 
but also add a
 >  flag, -XX:+UseG1LegacyPoolsAndBeans (false by default), to 
allow for a
 >  smoother transition. Would that solution work for you?
 >
 >  Thanks,
 >  Erik
 >
 >  > Thanks,
 >  >
 >  > Paul
 >  >
 >  > *From: *mandy chung <mandy.ch...@oracle.com>
 >      > *Organization: *Oracle Corporation
     >  > *Date: *Monday, January 29, 2018 at 1:41 PM
 >  > *To: *"Hohensee, Paul" <hohen...@amazon.com>
 >  > *Cc: *"serviceability-dev@openjdk.java.net"
 >  > <serviceability-dev@openjdk.java.net>, 
"hotspot-gc-...@openjdk.java.net"
 >  > <hotspot-gc-...@openjdk.java.net>
 >  > *Subject: *Re: RFR (S): 8195115: G1 Old Gen MemoryPool
 >  > CollectionUsage.used values don't reflect mixed GC results
 >  >
 >  > I created  JDK-8196362 to look into whether it makes 
sense to provide
 >  > some categorization to differentiate eden space vs the 
heap space for
 >  > long-lived objects.
 >  >
 >  > W.r.t. to JDK-8195115, I have to defer to GC team to 
comment on the
 >  > mixed collection update.  If they are okay, I have no 
objection to
 >  > implement a short-term fix and do the proper G1 memory 
pools as a
 >  > separate patch.
 >  >
 >  > Mandy
 >  >
 >  > On 1/29/18 1:02 PM, Hohensee, Paul wrote:
 >  >
 >  > We don’t use getType, and you guessed correctly: we 
use the memory
 >  > pool name as an indicator of the specific 
characteristics of a
 

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[Hohensee, Paul]

ault old collector way back in JDK 7 and 
changed 
again when G1 became the default collector in JDK 9.

Thanks,
Erik

> Thanks,
> 
> Paul
> 
> On 1/30/18, 5:51 AM, "Erik Helin" <erik.he...@oracle.com> wrote:
> 
>  On 01/30/2018 03:07 AM, Hohensee, Paul wrote:
>  > That’s one reviewer who’s ok with a short term patch. 
Anyone else? And,
>  > any reviewers for said short term patch? :)
>  
>  Well, the patch is not really complete as it is. The problem 
is the
>  definitions of the MemoryPoolMXBeans and 
GarbageCollectorMXBeans, which,
>  as I tried to hint at in my first email, is a mess for G1. 
The names and
>  implementations of these MemoryPoolMXBeans and 
GarbageCollectionMXBeans
>  for G1 are very old, G1 has changed a lot since those were 
implemented
>  (hence my suggestion for finally fixing this).
>  
>  The issue with your patch is that the MemoryPoolMXBean named 
"G1 Old
>  Gen" consists of both old and humongous regions (it will 
also include
>  archive regions). Old regions can be collected by mixed, 
concurrent and
>  full collections. Humongous regions can be collected by 
young, mixed or
>  full collections and the concurrent cycle. Archive regions 
will never be
>  collected. Your patch will update the pool in the case of a 
mixed
>  collection collecting old regions or humongous regions, but 
misses the
>  following cases:
>  - a young collection collecting humongous regions
>  - a concurrent cycle collecting humongous regions
>  - a concurrent cycle collecting old regions
>  
>  Unfortunately I could not come up with a good way to solve 
the above
>  without re-designing the pools. I'm not sure about accepting 
your patch
>  as is, since it might cause even more confusion for users 
compared to
>  the current (already confusing) situation.
>  
>  One idea we have discussed is to implement the re-design but 
also add a
>  flag, -XX:+UseG1LegacyPoolsAndBeans (false by default), to 
allow for a
>  smoother transition. Would that solution work for you?
>  
>  Thanks,
>  Erik
>  
>  > Thanks,
>  >
>  > Paul
>  >
>  > *From: *mandy chung <mandy.ch...@oracle.com>
>  > *Organization: *Oracle Corporation
    >  > *Date: *Monday, January 29, 2018 at 1:41 PM
>  > *To: *"Hohensee, Paul" <hohen...@amazon.com>
>  > *Cc: *"serviceability-dev@openjdk.java.net"
>  > <serviceability-dev@openjdk.java.net>, 
"hotspot-gc-...@openjdk.java.net"
>  > <hotspot-gc-...@openjdk.java.net>
>  > *Subject: *Re: RFR (S): 8195115: G1 Old Gen MemoryPool
>  > CollectionUsage.used values don't reflect mixed GC results
>  >
>  > I created  JDK-8196362 to look into whether it makes sense 
to provide
>  > some categorization to differentiate eden space vs the 
heap space for
>  > long-lived objects.
>  >
>  > W.r.t. to JDK-8195115, I have to defer to GC team to 
comment on the
>  > mixed collection update.  If they are okay, I have no 
objection to
>  > implement a short-term fix and do the proper G1 memory 
pools as a
>  > separate patch.
>  >
>  > Mandy
>  >
>  > On 1/29/18 1:02 PM, Hohensee, Paul wrote:
>  >
>  > We don’t use getType, and you guessed correctly: we 
use the memory
>  > pool name as an indicator of the specific 
characteristics of a
>  > memory pool, in particular eden.
>  >
>  > What we want is an indication of long term heap 
occupancy. We
>  > calculate it using CollectionUsage for non-eden he

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[Hohensee, Paul]

e 
implemented
>  (hence my suggestion for finally fixing this).
>  
>  The issue with your patch is that the MemoryPoolMXBean named "G1 
Old
>  Gen" consists of both old and humongous regions (it will also 
include
>  archive regions). Old regions can be collected by mixed, 
concurrent and
>  full collections. Humongous regions can be collected by young, 
mixed or
>  full collections and the concurrent cycle. Archive regions will 
never be
>  collected. Your patch will update the pool in the case of a mixed
>  collection collecting old regions or humongous regions, but 
misses the
>  following cases:
>  - a young collection collecting humongous regions
>  - a concurrent cycle collecting humongous regions
>  - a concurrent cycle collecting old regions
>  
>  Unfortunately I could not come up with a good way to solve the 
above
>  without re-designing the pools. I'm not sure about accepting 
your patch
>  as is, since it might cause even more confusion for users 
compared to
>  the current (already confusing) situation.
>  
>  One idea we have discussed is to implement the re-design but 
also add a
>  flag, -XX:+UseG1LegacyPoolsAndBeans (false by default), to allow 
for a
>  smoother transition. Would that solution work for you?
>  
>  Thanks,
>  Erik
>  
>  > Thanks,
>  >
>  > Paul
>  >
>      > *From: *mandy chung <mandy.ch...@oracle.com>
>  > *Organization: *Oracle Corporation
>  > *Date: *Monday, January 29, 2018 at 1:41 PM
>  > *To: *"Hohensee, Paul" <hohen...@amazon.com>
>  > *Cc: *"serviceability-dev@openjdk.java.net"
>  > <serviceability-dev@openjdk.java.net>, 
"hotspot-gc-...@openjdk.java.net"
>  > <hotspot-gc-...@openjdk.java.net>
>  > *Subject: *Re: RFR (S): 8195115: G1 Old Gen MemoryPool
>  > CollectionUsage.used values don't reflect mixed GC results
>  >
>  > I created  JDK-8196362 to look into whether it makes sense to 
provide
>  > some categorization to differentiate eden space vs the heap 
space for
>  > long-lived objects.
>  >
>  > W.r.t. to JDK-8195115, I have to defer to GC team to comment 
on the
>  > mixed collection update.  If they are okay, I have no 
objection to
>  > implement a short-term fix and do the proper G1 memory pools 
as a
>  > separate patch.
>  >
>  > Mandy
>  >
>  > On 1/29/18 1:02 PM, Hohensee, Paul wrote:
>  >
>  > We don’t use getType, and you guessed correctly: we use 
the memory
>  > pool name as an indicator of the specific characteristics 
of a
>  > memory pool, in particular eden.
>  >
>  > What we want is an indication of long term heap occupancy. 
We
>  > calculate it using CollectionUsage for non-eden heap 
memory pools,
>  > regardless of collector. We don’t use JMX notification, 
rather we
>  > periodically poll CollectionUsage for memory pools with 
names that
>  > contain “Old”, “Tenured”, or “Survivor”. We get the memory 
pools
>  > from the GarbageCollectorMXBeans (we don’t care what the 
collector
>  > names are). For the named memory pools, we sum 
CollectionUsage.used
>  > and divide by the sum of CollectionUsage.max to get a long 
term heap
>  > occupancy percentage. We don’t want to include eden 
because it’s
>  > really just an allocation buffer and not part of the 
storage for
>  > long-lived objects. I suppose we could use a negative test 
instead
>  > by using all memory pools with names that don’t include 
“Eden”.
>  >
>  > The bug is that the “G1 Old Gen” memory pool isn’t being 
updated
>  > when the “G1 Young Generation” collector runs a mixed 
collection. As
>  > far as JMX is concerned, that collector only knows about 
eden and
>  > the survivor space. The patch adds the old gen to 

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[Hohensee, Paul]

  following cases:
>  - a young collection collecting humongous regions
>  - a concurrent cycle collecting humongous regions
>  - a concurrent cycle collecting old regions
>  
>  Unfortunately I could not come up with a good way to solve the above
>  without re-designing the pools. I'm not sure about accepting your 
patch
>  as is, since it might cause even more confusion for users compared to
>  the current (already confusing) situation.
>  
>  One idea we have discussed is to implement the re-design but also 
add a
>  flag, -XX:+UseG1LegacyPoolsAndBeans (false by default), to allow for 
a
>  smoother transition. Would that solution work for you?
>  
>  Thanks,
>  Erik
>  
>  > Thanks,
>  >
>  > Paul
>  >
    >  > *From: *mandy chung <mandy.ch...@oracle.com>
    >  > *Organization: *Oracle Corporation
>  > *Date: *Monday, January 29, 2018 at 1:41 PM
>  > *To: *"Hohensee, Paul" <hohen...@amazon.com>
>  > *Cc: *"serviceability-dev@openjdk.java.net"
>  > <serviceability-dev@openjdk.java.net>, 
"hotspot-gc-...@openjdk.java.net"
>  > <hotspot-gc-...@openjdk.java.net>
>  > *Subject: *Re: RFR (S): 8195115: G1 Old Gen MemoryPool
>  > CollectionUsage.used values don't reflect mixed GC results
>  >
>  > I created  JDK-8196362 to look into whether it makes sense to 
provide
>  > some categorization to differentiate eden space vs the heap space 
for
>  > long-lived objects.
>  >
>  > W.r.t. to JDK-8195115, I have to defer to GC team to comment on the
>  > mixed collection update.  If they are okay, I have no objection to
>  > implement a short-term fix and do the proper G1 memory pools as a
>  > separate patch.
>  >
>  > Mandy
>  >
>  > On 1/29/18 1:02 PM, Hohensee, Paul wrote:
>  >
>  > We don’t use getType, and you guessed correctly: we use the 
memory
>  > pool name as an indicator of the specific characteristics of a
>  > memory pool, in particular eden.
>  >
>  > What we want is an indication of long term heap occupancy. We
>  > calculate it using CollectionUsage for non-eden heap memory 
pools,
>  > regardless of collector. We don’t use JMX notification, rather 
we
>  > periodically poll CollectionUsage for memory pools with names 
that
>  > contain “Old”, “Tenured”, or “Survivor”. We get the memory 
pools
>  > from the GarbageCollectorMXBeans (we don’t care what the 
collector
>  > names are). For the named memory pools, we sum 
CollectionUsage.used
>  > and divide by the sum of CollectionUsage.max to get a long 
term heap
>  > occupancy percentage. We don’t want to include eden because 
it’s
>  > really just an allocation buffer and not part of the storage 
for
>  > long-lived objects. I suppose we could use a negative test 
instead
>  > by using all memory pools with names that don’t include “Eden”.
>  >
>  > The bug is that the “G1 Old Gen” memory pool isn’t being 
updated
>  > when the “G1 Young Generation” collector runs a mixed 
collection. As
>  > far as JMX is concerned, that collector only knows about eden 
and
>  > the survivor space. The patch adds the old gen to the memory 
pools
>  > it knows about and has mixed collections update the old gen’s
>  > CollectionUsage.
>  >
>  > I managed to get a submit repo run to succeed last week and it 
found
>  > a problem. I’ve uploaded a new webrev that fixes the failure 
of the
>  > jtreg test TestMemoryMXBeansAndPoolsPresence.java due to the 
young
>  > gen collector being expected to know only about eden and the
>  > survivor space.
>  >
>  > http://cr.openjdk.java.net/~phh/8195115/webrev.hs.01/
>  > <http://cr.openjdk.java.net/%7Ephh/8195115/webrev.hs.01/>
>  >
>  > Waiting on the submit repo to come back with a result on it.
>  >
>  > Thanks,
>  >
>  > Paul
>  >
>  > *From: *m

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[Kirk Pepperdine]

/2018 03:07 AM, Hohensee, Paul wrote:
>> That’s one reviewer who’s ok with a short term patch. Anyone else? And, 
>> any reviewers for said short term patch? :)
> 
>Well, the patch is not really complete as it is. The problem is the 
>definitions of the MemoryPoolMXBeans and GarbageCollectorMXBeans, which, 
>as I tried to hint at in my first email, is a mess for G1. The names and 
>implementations of these MemoryPoolMXBeans and GarbageCollectionMXBeans 
>for G1 are very old, G1 has changed a lot since those were implemented 
>(hence my suggestion for finally fixing this).
> 
>The issue with your patch is that the MemoryPoolMXBean named "G1 Old 
>Gen" consists of both old and humongous regions (it will also include 
>archive regions). Old regions can be collected by mixed, concurrent and 
>full collections. Humongous regions can be collected by young, mixed or 
>full collections and the concurrent cycle. Archive regions will never be 
>collected. Your patch will update the pool in the case of a mixed 
>collection collecting old regions or humongous regions, but misses the 
>following cases:
>- a young collection collecting humongous regions
>- a concurrent cycle collecting humongous regions
>- a concurrent cycle collecting old regions
> 
>Unfortunately I could not come up with a good way to solve the above 
>without re-designing the pools. I'm not sure about accepting your patch 
>as is, since it might cause even more confusion for users compared to 
>the current (already confusing) situation.
> 
>One idea we have discussed is to implement the re-design but also add a 
>flag, -XX:+UseG1LegacyPoolsAndBeans (false by default), to allow for a 
>smoother transition. Would that solution work for you?
> 
>Thanks,
>Erik
> 
>> Thanks,
>> 
>> Paul
>> 
>> *From: *mandy chung <mandy.ch...@oracle.com>
>> *Organization: *Oracle Corporation
>> *Date: *Monday, January 29, 2018 at 1:41 PM
>> *To: *"Hohensee, Paul" <hohen...@amazon.com>
>> *Cc: *"serviceability-dev@openjdk.java.net" 
>> <serviceability-dev@openjdk.java.net>, "hotspot-gc-...@openjdk.java.net" 
>> <hotspot-gc-...@openjdk.java.net>
>> *Subject: *Re: RFR (S): 8195115: G1 Old Gen MemoryPool 
>> CollectionUsage.used values don't reflect mixed GC results
>> 
>> I created  JDK-8196362 to look into whether it makes sense to provide 
>> some categorization to differentiate eden space vs the heap space for 
>> long-lived objects.
>> 
>> W.r.t. to JDK-8195115, I have to defer to GC team to comment on the 
>> mixed collection update.  If they are okay, I have no objection to 
>> implement a short-term fix and do the proper G1 memory pools as a 
>> separate patch.
>> 
>> Mandy
>> 
>> On 1/29/18 1:02 PM, Hohensee, Paul wrote:
>> 
>>We don’t use getType, and you guessed correctly: we use the memory
>>pool name as an indicator of the specific characteristics of a
>>memory pool, in particular eden.
>> 
>>What we want is an indication of long term heap occupancy. We
>>calculate it using CollectionUsage for non-eden heap memory pools,
>>regardless of collector. We don’t use JMX notification, rather we
>>periodically poll CollectionUsage for memory pools with names that
>>contain “Old”, “Tenured”, or “Survivor”. We get the memory pools
>>from the GarbageCollectorMXBeans (we don’t care what the collector
>>names are). For the named memory pools, we sum CollectionUsage.used
>>and divide by the sum of CollectionUsage.max to get a long term heap
>>occupancy percentage. We don’t want to include eden because it’s
>>really just an allocation buffer and not part of the storage for
>>long-lived objects. I suppose we could use a negative test instead
>>by using all memory pools with names that don’t include “Eden”.
>> 
>>The bug is that the “G1 Old Gen” memory pool isn’t being updated
>>when the “G1 Young Generation” collector runs a mixed collection. As
>>far as JMX is concerned, that collector only knows about eden and
>>the survivor space. The patch adds the old gen to the memory pools
>>it knows about and has mixed collections update the old gen’s
>>CollectionUsage.
>> 
>>I managed to get a submit repo run to succeed last week and it found
>>a problem. I’ve uploaded a new webrev that fixes the failure of the
>>jtreg test TestMemoryMXBeansAndPoolsPresence.java due to the young
>>gen collector being expected to know o

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[Erik Helin]

rt over and do this right and a user who relies on 
the current behavior can get that by just setting a flag. Doing such a 
change in a major release also allows us to clearly highlight the change 
in the release notes (users are more prepared for larger changes in a 
major release and that they might have to add flags to keep old behavior).


It is not uncommon for memory pools to change in major releases. The 
perm gen pool was removed in JDK 8, the default pools changed when 
Parallel Old became default old collector way back in JDK 7 and changed 
again when G1 became the default collector in JDK 9.


Thanks,
Erik


Thanks,

Paul

On 1/30/18, 5:51 AM, "Erik Helin" <erik.he...@oracle.com> wrote:

 On 01/30/2018 03:07 AM, Hohensee, Paul wrote:
 > That’s one reviewer who’s ok with a short term patch. Anyone else? And,
 > any reviewers for said short term patch? :)
 
 Well, the patch is not really complete as it is. The problem is the

 definitions of the MemoryPoolMXBeans and GarbageCollectorMXBeans, which,
 as I tried to hint at in my first email, is a mess for G1. The names and
 implementations of these MemoryPoolMXBeans and GarbageCollectionMXBeans
 for G1 are very old, G1 has changed a lot since those were implemented
 (hence my suggestion for finally fixing this).
 
 The issue with your patch is that the MemoryPoolMXBean named "G1 Old

 Gen" consists of both old and humongous regions (it will also include
 archive regions). Old regions can be collected by mixed, concurrent and
 full collections. Humongous regions can be collected by young, mixed or
 full collections and the concurrent cycle. Archive regions will never be
 collected. Your patch will update the pool in the case of a mixed
 collection collecting old regions or humongous regions, but misses the
 following cases:
 - a young collection collecting humongous regions
 - a concurrent cycle collecting humongous regions
 - a concurrent cycle collecting old regions
 
 Unfortunately I could not come up with a good way to solve the above

 without re-designing the pools. I'm not sure about accepting your patch
 as is, since it might cause even more confusion for users compared to
 the current (already confusing) situation.
 
 One idea we have discussed is to implement the re-design but also add a

 flag, -XX:+UseG1LegacyPoolsAndBeans (false by default), to allow for a
 smoother transition. Would that solution work for you?
 
 Thanks,

 Erik
 
 > Thanks,

 >
 > Paul
 >
 > *From: *mandy chung <mandy.ch...@oracle.com>
 > *Organization: *Oracle Corporation
 > *Date: *Monday, January 29, 2018 at 1:41 PM
 > *To: *"Hohensee, Paul" <hohen...@amazon.com>
     > *Cc: *"serviceability-dev@openjdk.java.net"
     > <serviceability-dev@openjdk.java.net>, "hotspot-gc-...@openjdk.java.net"
 > <hotspot-gc-...@openjdk.java.net>
 > *Subject: *Re: RFR (S): 8195115: G1 Old Gen MemoryPool
 > CollectionUsage.used values don't reflect mixed GC results
 >
 > I created  JDK-8196362 to look into whether it makes sense to provide
 > some categorization to differentiate eden space vs the heap space for
 > long-lived objects.
 >
 > W.r.t. to JDK-8195115, I have to defer to GC team to comment on the
 > mixed collection update.  If they are okay, I have no objection to
 > implement a short-term fix and do the proper G1 memory pools as a
 > separate patch.
 >
 > Mandy
 >
 > On 1/29/18 1:02 PM, Hohensee, Paul wrote:
 >
 > We don’t use getType, and you guessed correctly: we use the memory
 > pool name as an indicator of the specific characteristics of a
 > memory pool, in particular eden.
 >
 > What we want is an indication of long term heap occupancy. We
 > calculate it using CollectionUsage for non-eden heap memory pools,
 > regardless of collector. We don’t use JMX notification, rather we
 > periodically poll CollectionUsage for memory pools with names that
 > contain “Old”, “Tenured”, or “Survivor”. We get the memory pools
 > from the GarbageCollectorMXBeans (we don’t care what the collector
 > names are). For the named memory pools, we sum CollectionUsage.used
 > and divide by the sum of CollectionUsage.max to get a long term heap
 > occupancy percentage. We don’t want to include eden because it’s
 > really just an allocation buffer and not part of the storage for
 > long-lived objects. I suppose we could use a negative test instead
 > by using all memory pools with names that don’t include “Eden”.
 >
  

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[Hohensee, Paul]

It’s true that my patch doesn’t completely solve the larger problem, but it 
fixes the most immediately important part of it, particularly for JDK8 where 
current expected behavior is entrenched. If we’re going to fix the larger 
problem, imo we should file another bug/rfe to do it. I’d be happy to fix that 
one too, once we have a spec.

What do you think of my suggestions? To summarize:

- Keep the existing memory pools and add humongous and archive pools.
- Make the archive pool part of the old gen, and generalize it to all 
collectors.
- Split humongous regions off from the old gen pool into their own pool. The 
old gen and humongous pools are disjoint region sets.
- Keep the existing “G1 Young Generation” and “G1 Old Generation” collectors 
and their associated memory pools (net of this patch). We add the humongous 
pool to them.
- Add “G1 Full” as an alias of the existing “G1 Old Generation” collector.
- Add the “G1 Young”, “G1 Mixed” and “G1 Concurrent Cycle” collectors.
- Set the G1 old gen memory pool max size to –Xmx, the archive space max size 
to whatever it is, and the rest of the G1 memory pool max sizes to -1 == 
undefined, as now.

The resulting memory pools:

“G1 Eden Space”
“G1 Survivor Space”
“G1 Old Gen”
“G1 Humongous Space”
“Archive Space”

The resulting collectors and their memory pools:

“G1 Young Generation” (the existing young/mixed collector), “G1 Old 
Generation”/”G1 Full”, “G1 Mixed”
- “G1 Eden Space”
- “G1 Survivor Space”
- “G1 Old Gen”
- “G1 Humongous Space”
“G1 Young”
- “G1 Eden Space”
- “G1 Survivor Space”
- “G1 Humongous Space”
“G1 Concurrent Cycle”
- “G1 Old Gen”
- “G1 Humongous Space”

I’m not religious about the names, but I like my suggestions. :)

The significant addition to my previous email, and an incompatible change, is 
splitting humongous regions off from the old gen pool. This means that apps 
that specifically monitor old gen occupancy will no longer see humongous 
regions. Monitoring apps that just add up info about all a collector’s pools 
won’t see a difference. I may be corrected (by Kirk, perhaps), but imo it’s not 
as bad a compatibility issue as one might think, because the type of app that 
uses a lot of humongous regions isn’t all that common. E.g., apps that cache 
data in the heap in the form of large compressed arrays, and apps with large 
hashmap bucket list arrays. The heaps such apps use are very often large enough 
to use 32mb regions, hence need really big objects to actually allocate 
humongous regions.

Thanks,

Paul

On 1/30/18, 5:51 AM, "Erik Helin" <erik.he...@oracle.com> wrote:

On 01/30/2018 03:07 AM, Hohensee, Paul wrote:
> That’s one reviewer who’s ok with a short term patch. Anyone else? And, 
> any reviewers for said short term patch? :)

Well, the patch is not really complete as it is. The problem is the 
definitions of the MemoryPoolMXBeans and GarbageCollectorMXBeans, which, 
as I tried to hint at in my first email, is a mess for G1. The names and 
implementations of these MemoryPoolMXBeans and GarbageCollectionMXBeans 
for G1 are very old, G1 has changed a lot since those were implemented 
(hence my suggestion for finally fixing this).

The issue with your patch is that the MemoryPoolMXBean named "G1 Old 
Gen" consists of both old and humongous regions (it will also include 
archive regions). Old regions can be collected by mixed, concurrent and 
full collections. Humongous regions can be collected by young, mixed or 
full collections and the concurrent cycle. Archive regions will never be 
collected. Your patch will update the pool in the case of a mixed 
collection collecting old regions or humongous regions, but misses the 
following cases:
- a young collection collecting humongous regions
- a concurrent cycle collecting humongous regions
- a concurrent cycle collecting old regions

Unfortunately I could not come up with a good way to solve the above 
without re-designing the pools. I'm not sure about accepting your patch 
as is, since it might cause even more confusion for users compared to 
the current (already confusing) situation.

One idea we have discussed is to implement the re-design but also add a 
flag, -XX:+UseG1LegacyPoolsAndBeans (false by default), to allow for a 
smoother transition. Would that solution work for you?

Thanks,
Erik

> Thanks,
> 
> Paul
> 
> *From: *mandy chung <mandy.ch...@oracle.com>
> *Organization: *Oracle Corporation
> *Date: *Monday, January 29, 2018 at 1:41 PM
> *To: *"Hohensee, Paul" <hohen...@amazon.com>
> *Cc: *"serviceability-dev@openjdk.java.net" 
> <serviceability-dev@openjdk.java.net>, "hotspot-gc-...@openjdk.java.net" 
> <hotspot-gc-...@openjdk.java.net>
> *Subject: *Re: RFR (S

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[Erik Helin]

On 01/30/2018 03:07 AM, Hohensee, Paul wrote:
That’s one reviewer who’s ok with a short term patch. Anyone else? And, 
any reviewers for said short term patch? :)


Well, the patch is not really complete as it is. The problem is the 
definitions of the MemoryPoolMXBeans and GarbageCollectorMXBeans, which, 
as I tried to hint at in my first email, is a mess for G1. The names and 
implementations of these MemoryPoolMXBeans and GarbageCollectionMXBeans 
for G1 are very old, G1 has changed a lot since those were implemented 
(hence my suggestion for finally fixing this).


The issue with your patch is that the MemoryPoolMXBean named "G1 Old 
Gen" consists of both old and humongous regions (it will also include 
archive regions). Old regions can be collected by mixed, concurrent and 
full collections. Humongous regions can be collected by young, mixed or 
full collections and the concurrent cycle. Archive regions will never be 
collected. Your patch will update the pool in the case of a mixed 
collection collecting old regions or humongous regions, but misses the 
following cases:

- a young collection collecting humongous regions
- a concurrent cycle collecting humongous regions
- a concurrent cycle collecting old regions

Unfortunately I could not come up with a good way to solve the above 
without re-designing the pools. I'm not sure about accepting your patch 
as is, since it might cause even more confusion for users compared to 
the current (already confusing) situation.


One idea we have discussed is to implement the re-design but also add a 
flag, -XX:+UseG1LegacyPoolsAndBeans (false by default), to allow for a 
smoother transition. Would that solution work for you?


Thanks,
Erik


Thanks,

Paul

*From: *mandy chung <mandy.ch...@oracle.com>
*Organization: *Oracle Corporation
*Date: *Monday, January 29, 2018 at 1:41 PM
*To: *"Hohensee, Paul" <hohen...@amazon.com>
*Cc: *"serviceability-dev@openjdk.java.net" 
<serviceability-dev@openjdk.java.net>, "hotspot-gc-...@openjdk.java.net" 
<hotspot-gc-...@openjdk.java.net>
*Subject: *Re: RFR (S): 8195115: G1 Old Gen MemoryPool 
CollectionUsage.used values don't reflect mixed GC results


I created  JDK-8196362 to look into whether it makes sense to provide 
some categorization to differentiate eden space vs the heap space for 
long-lived objects.


W.r.t. to JDK-8195115, I have to defer to GC team to comment on the 
mixed collection update.  If they are okay, I have no objection to 
implement a short-term fix and do the proper G1 memory pools as a 
separate patch.


Mandy

On 1/29/18 1:02 PM, Hohensee, Paul wrote:

We don’t use getType, and you guessed correctly: we use the memory
pool name as an indicator of the specific characteristics of a
memory pool, in particular eden.

What we want is an indication of long term heap occupancy. We
calculate it using CollectionUsage for non-eden heap memory pools,
regardless of collector. We don’t use JMX notification, rather we
periodically poll CollectionUsage for memory pools with names that
contain “Old”, “Tenured”, or “Survivor”. We get the memory pools
from the GarbageCollectorMXBeans (we don’t care what the collector
names are). For the named memory pools, we sum CollectionUsage.used
and divide by the sum of CollectionUsage.max to get a long term heap
occupancy percentage. We don’t want to include eden because it’s
really just an allocation buffer and not part of the storage for
long-lived objects. I suppose we could use a negative test instead
by using all memory pools with names that don’t include “Eden”.

The bug is that the “G1 Old Gen” memory pool isn’t being updated
when the “G1 Young Generation” collector runs a mixed collection. As
far as JMX is concerned, that collector only knows about eden and
the survivor space. The patch adds the old gen to the memory pools
it knows about and has mixed collections update the old gen’s
CollectionUsage.

I managed to get a submit repo run to succeed last week and it found
a problem. I’ve uploaded a new webrev that fixes the failure of the
jtreg test TestMemoryMXBeansAndPoolsPresence.java due to the young
gen collector being expected to know only about eden and the
survivor space.

http://cr.openjdk.java.net/~phh/8195115/webrev.hs.01/
<http://cr.openjdk.java.net/%7Ephh/8195115/webrev.hs.01/>

Waiting on the submit repo to come back with a result on it.

Thanks,

Paul

*From: *mandy chung <mandy.ch...@oracle.com>
<mailto:mandy.ch...@oracle.com>
*Organization: *Oracle Corporation
*Date: *Monday, January 29, 2018 at 10:52 AM
*To: *"Hohensee, Paul" <hohen...@amazon.com>
<mailto:hohen...@amazon.com>, Erik Helin <erik.he...@oracle.com>
<mailto:erik.he...@oracle.com>, David Holmes
<david.hol...@orac

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[Hohensee, Paul]

That’s one reviewer who’s ok with a short term patch. Anyone else? And, any 
reviewers for said short term patch? :)

Thanks,

Paul

From: mandy chung <mandy.ch...@oracle.com>
Organization: Oracle Corporation
Date: Monday, January 29, 2018 at 1:41 PM
To: "Hohensee, Paul" <hohen...@amazon.com>
Cc: "serviceability-dev@openjdk.java.net" 
<serviceability-dev@openjdk.java.net>, "hotspot-gc-...@openjdk.java.net" 
<hotspot-gc-...@openjdk.java.net>
Subject: Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used 
values don't reflect mixed GC results

I created  JDK-8196362 to look into whether it makes sense to provide some 
categorization to differentiate eden space vs the heap space for long-lived 
objects.

W.r.t. to JDK-8195115, I have to defer to GC team to comment on the mixed 
collection update.  If they are okay, I have no objection to implement a 
short-term fix and do the proper G1 memory pools as a separate patch.

Mandy
On 1/29/18 1:02 PM, Hohensee, Paul wrote:
We don’t use getType, and you guessed correctly: we use the memory pool name as 
an indicator of the specific characteristics of a memory pool, in particular 
eden.

What we want is an indication of long term heap occupancy. We calculate it 
using CollectionUsage for non-eden heap memory pools, regardless of collector. 
We don’t use JMX notification, rather we periodically poll CollectionUsage for 
memory pools with names that contain “Old”, “Tenured”, or “Survivor”. We get 
the memory pools from the GarbageCollectorMXBeans (we don’t care what the 
collector names are). For the named memory pools, we sum CollectionUsage.used 
and divide by the sum of CollectionUsage.max to get a long term heap occupancy 
percentage. We don’t want to include eden because it’s really just an 
allocation buffer and not part of the storage for long-lived objects. I suppose 
we could use a negative test instead by using all memory pools with names that 
don’t include “Eden”.

The bug is that the “G1 Old Gen” memory pool isn’t being updated when the “G1 
Young Generation” collector runs a mixed collection. As far as JMX is 
concerned, that collector only knows about eden and the survivor space. The 
patch adds the old gen to the memory pools it knows about and has mixed 
collections update the old gen’s CollectionUsage.

I managed to get a submit repo run to succeed last week and it found a problem. 
I’ve uploaded a new webrev that fixes the failure of the jtreg test 
TestMemoryMXBeansAndPoolsPresence.java due to the young gen collector being 
expected to know only about eden and the survivor space.

http://cr.openjdk.java.net/~phh/8195115/webrev.hs.01/<http://cr.openjdk.java.net/%7Ephh/8195115/webrev.hs.01/>

Waiting on the submit repo to come back with a result on it.

Thanks,

Paul

From: mandy chung <mandy.ch...@oracle.com><mailto:mandy.ch...@oracle.com>
Organization: Oracle Corporation
Date: Monday, January 29, 2018 at 10:52 AM
To: "Hohensee, Paul" <hohen...@amazon.com><mailto:hohen...@amazon.com>, Erik 
Helin <erik.he...@oracle.com><mailto:erik.he...@oracle.com>, David Holmes 
<david.hol...@oracle.com><mailto:david.hol...@oracle.com>
Cc: 
"serviceability-dev@openjdk.java.net"<mailto:serviceability-dev@openjdk.java.net>
 
<serviceability-dev@openjdk.java.net><mailto:serviceability-dev@openjdk.java.net>,
 "hotspot-gc-...@openjdk.java.net"<mailto:hotspot-gc-...@openjdk.java.net> 
<hotspot-gc-...@openjdk.java.net><mailto:hotspot-gc-...@openjdk.java.net>
Subject: Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used 
values don't reflect mixed GC results


On 1/29/18 10:35 AM, mandy chung wrote:
Thanks for the reply Paul.   Try to understand a little more on the specific 
from gc-specific memory pool you depend on.
On 1/29/18 8:27 AM, Hohensee, Paul wrote:
A name change would affect Amazon’s heap monitoring, and thus I expect it would 
affect other users as well.

As long as there are gc-specific memory pools, we’re going to need to be able 
to identify them, and right now that’s done via name.

MemoryPoolMXBean::getType returns "heap" memory type for GC-specific memory 
pools.  Are you using this method?  Do you use the name to build in specific 
characteristic of a memory pool (e.g. eden vs old gen)?




All the mxbeans are identified by name, so that’s a general design principle. 
The only way I can think of to get rid of name dependency would be to figure 
out what abstract metrics users want to monitor and implement them for all 
collectors. HeapUsage (instantaneous occupancy) is one, CollectionUsage 
(long-lived occupancy) is another, both of these for the entire heap, not just 
particular memory pools.

The sum of HeapUsage and CollectionUsage of all heap memory pools was expected 
to give an incorrect approximation for the entire heap usage.  Are you seein

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[Kirk Pepperdine]

> On Jan 29, 2018, at 5:27 PM, Hohensee, Paul  wrote:
> 
> A name change would affect Amazon’s heap monitoring, and thus I expect it 
> would affect other users as well.

I can name a number of tools that would be disrupted by this type of change. 
Additionally tooling would be complicated by the need to support both the old 
and new versions. The current names have been with us for years and they are 
well known, well documented and well understood. Given the level of disruption 
this change is likely to cause IMHO you’d need a very very good reason to want 
to make it.
>  
> As long as there are gc-specific memory pools, we’re going to need to be able 
> to identify them, and right now that’s done via name. All the mxbeans are 
> identified by name, so that’s a general design principle. The only way I can 
> think of to get rid of name dependency would be to figure out what abstract 
> metrics users want to monitor and implement them for all collectors. 
> HeapUsage (instantaneous occupancy) is one, CollectionUsage (long-lived 
> occupancy) is another, both of these for the entire heap, not just particular 
> memory pools. That said, imo there will always be a demand for the ability to 
> get collector and memory pool specific details, so I don’t see a way to get 
> around providing named entities.

Agreed…tuning strategies are implementation dependent and sensitive to specific 
versions. One is always going to need to know this information.

Kind regards,
Kirk Pepperdine

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[mandy chung]

I created  JDK-8196362 to look into whether it makes sense to provide 
some categorization to differentiate eden space vs the heap space for 
long-lived objects.


W.r.t. to JDK-8195115, I have to defer to GC team to comment on the 
mixed collection update.  If they are okay, I have no objection to 
implement a short-term fix and do the proper G1 memory pools as a 
separate patch.


Mandy

On 1/29/18 1:02 PM, Hohensee, Paul wrote:


We don’t use getType, and you guessed correctly: we use the memory 
pool name as an indicator of the specific characteristics of a memory 
pool, in particular eden.


What we want is an indication of long term heap occupancy. We 
calculate it using CollectionUsage for non-eden heap memory pools, 
regardless of collector. We don’t use JMX notification, rather we 
periodically poll CollectionUsage for memory pools with names that 
contain “Old”, “Tenured”, or “Survivor”. We get the memory pools from 
the GarbageCollectorMXBeans (we don’t care what the collector names 
are). For the named memory pools, we sum CollectionUsage.used and 
divide by the sum of CollectionUsage.max to get a long term heap 
occupancy percentage. We don’t want to include eden because it’s 
really just an allocation buffer and not part of the storage for 
long-lived objects. I suppose we could use a negative test instead by 
using all memory pools with names that don’t include “Eden”.


The bug is that the “G1 Old Gen” memory pool isn’t being updated when 
the “G1 Young Generation” collector runs a mixed collection. As far as 
JMX is concerned, that collector only knows about eden and the 
survivor space. The patch adds the old gen to the memory pools it 
knows about and has mixed collections update the old gen’s 
CollectionUsage.


I managed to get a submit repo run to succeed last week and it found a 
problem. I’ve uploaded a new webrev that fixes the failure of the 
jtreg test TestMemoryMXBeansAndPoolsPresence.java due to the young gen 
collector being expected to know only about eden and the survivor space.


http://cr.openjdk.java.net/~phh/8195115/webrev.hs.01/ 
<http://cr.openjdk.java.net/%7Ephh/8195115/webrev.hs.01/>


Waiting on the submit repo to come back with a result on it.

Thanks,

Paul

*From: *mandy chung <mandy.ch...@oracle.com>
*Organization: *Oracle Corporation
*Date: *Monday, January 29, 2018 at 10:52 AM
*To: *"Hohensee, Paul" <hohen...@amazon.com>, Erik Helin 
<erik.he...@oracle.com>, David Holmes <david.hol...@oracle.com>
*Cc: *"serviceability-dev@openjdk.java.net" 
<serviceability-dev@openjdk.java.net>, 
"hotspot-gc-...@openjdk.java.net" <hotspot-gc-...@openjdk.java.net>
*Subject: *Re: RFR (S): 8195115: G1 Old Gen MemoryPool 
CollectionUsage.used values don't reflect mixed GC results


On 1/29/18 10:35 AM, mandy chung wrote:

Thanks for the reply Paul.   Try to understand a little more on
the specific from gc-specific memory pool you depend on.

On 1/29/18 8:27 AM, Hohensee, Paul wrote:

A name change would affect Amazon’s heap monitoring, and thus
I expect it would affect other users as well.

As long as there are gc-specific memory pools, we’re going to
need to be able to identify them, and right now that’s done
via name.


MemoryPoolMXBean::getType returns "heap" memory type for
GC-specific memory pools.  Are you using this method?  Do you use
the name to build in specific characteristic of a memory pool
(e.g. eden vs old gen)?



All the mxbeans are identified by name, so that’s a general
design principle. The only way I can think of to get rid of
name dependency would be to figure out what abstract metrics
users want to monitor and implement them for all collectors.
HeapUsage (instantaneous occupancy) is one, CollectionUsage
(long-lived occupancy) is another, both of these for the
entire heap, not just particular memory pools.


The sum of HeapUsage and CollectionUsage of all heap memory pools
was expected to give an incorrect approximation for the entire
heap usage.  Are you seeing issue/bug with the sum result?


typo: s/an incorrect approximation/an approximation.

Mandy


Mandy


That said, imo there will always be a demand for the ability
to get collector and memory pool specific details, so I don’t
see a way to get around providing named entities.

Paul

*From: *mandy chung <mandy.ch...@oracle.com>
<mailto:mandy.ch...@oracle.com>
*Organization: *Oracle Corporation
*Date: *Friday, January 26, 2018 at 2:38 PM
*To: *"Hohensee, Paul" <hohen...@amazon.com>
<mailto:hohen...@amazon.com>, Erik Helin
<erik.he...@oracle.com> <mailto:erik.he...@oracle.com>, David
Holmes <david.hol...@oracle.com> &l

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[David Holmes]

On the CSR question, yes this would need a CSR just to ensure the 
compatibility issues have been covered.


David

On 25/01/2018 11:20 PM, Erik Helin wrote:

Hi Paul,

thanks for your interest in this area and for your patch! The 
GarbageCollectorMXBean and MemoryPoolMXBean support for G1 is in need of 
some updates, so thanks for working on this.


Looking at your patch, I'm not sure that this is the direction we want 
to go in. I discussed this a bit with Thomas and Stefan J, and our 
current line of thinking is the following:


- Memory pools (MemoryMXBean):
   - "G1 Eden Regions"
   - "G1 Survivor Regions"
   - "G1 Old Regions"
   - "G1 Humongous Regions"
   - "G1 Archive Regions" (if CDS and/or AppCDS is used)

`init` for these pools would be 0, `used` would be total size of the 
"live" objects in the used regions of that type, `committed` the total 
size of the used regions of that that type and `max` would be 
MaxHeapSize. Note that "live" here means live from the GCs point of 
view, i.e. an object might be dead in an old region but the GC will 
consider that object live until a concurrent cycle has marked through 
the heap and deemed it dead.


- Collectors (GarbageCollectorMXBean):
   - "G1 Young Collector" with the pools
     - "G1 Eden Regions"
     - "G1 Survivor Regions"
     - "G1 Humongous Regions" (due to early reclamation)
   - "G1 Mixed Collector" with the pools
     - "G1 Eden Regions"
     - "G1 Survivor Regions"
     - "G1 Old Regions"
     - "G1 Humongous Regions" (due to early reclamation)
   - "G1 Full Collector" with the pools
     - "G1 Eden Regions"
     - "G1 Survivor Regions"
     - "G1 Old Regions"
     - "G1 Humongous Regions" (can collect empty humongous regions)
   - "G1 Concurrent Cycle" with the pools
     - "G1 Old Regions" (can collect empty old regions)
     - "G1 Humongous Regions" (can collect empty humongous regions)

Note that with this design, the 
GarbageCollectorMXBean::getCollectionTime() method for "G1 Concurrent 
Cycle" would be the wall clock time from start of the first initial mark 
to end of the last cleanup (also including the time of any eventual 
young collection during the concurrent cycle). So 
GarbageCollectorMXBean::getCollectionTime() would be a mix of concurrent 
and STW time for the GarbageCollectorMXBean with name "G1 Concurrent 
Cycle".


Also note that the MemoryPoolMXBean with name "G1 Archive Regions" will 
not be attached to any GarbageCollectorMXBean, since those regions will 
never be collected.


What do you think about this design, would it work for your use case?

If we want to go ahead with this design, then I think we might have to 
file a CSR. David (who is the HotSpot CSR representative), do we have to 
file a CSR for changing the names of MemoryPoolMXBeans and 
GarbageCollectorMXBeans?


Thanks,
Erik

On 01/20/2018 12:40 AM, Hohensee, Paul wrote:

I’d appreciate a review please.

Bug: https://bugs.openjdk.java.net/browse/JDK-8195115

Webrev: http://cr.openjdk.java.net/~phh/8195115/webrev.00/

The bug is that from the JMX point of view, G1’s incremental collector 
(misnamed as the “G1 Young Generation” collector) only affects G1’s 
survivor and eden spaces. In fact, mixed collections run by this 
collector also affect the G1 old generation.


This proposed fix is to record, for each of a JMX garbage collector's 
memory pools, whether that memory pool is affected by all collections 
using that collector. And, for each collection, record whether or not 
all the collector's memory pools are affected. After each collection, 
for each memory pool, if either all the collector's memory pools were 
affected or the memory pool is affected for all collections, record 
CollectionUsage for that pool.


For collectors other than G1 Young Generation, all pools are recorded 
as affected by all collections and every collection is recorded as 
affecting all the collector’s memory pools. For the G1 Young 
Generation collector, the G1 Old Gen pool is recorded as not being 
affected by all collections, and non-mixed collections are recorded as 
not affecting all memory pools. The result is that for non-mixed 
collections, CollectionUsage is recorded after a collection only the 
G1 Eden Space and G1 Survivor Space pools, while for mixed collections 
CollectionUsage is recorded for G1 Old Gen as well.


Other than the effect of the fix on G1 Old Gen MemoryPool. 
CollectionUsage, the only external behavior change is that 
GarbageCollectorMXBean.getMemoryPoolNames will now return 3 pool names 
rather than 2.


With this fix, a collector’s memory pools can be divided into two 
disjoint subsets, one that participates in all collections and one 
that doesn’t. This is a bit more general than the minimum necessary to 
fix G1, but not by much. Because I expect it to apply to other 
incremental region-based collectors, I went with the more general 
solution. I minimized the amount of code I had to touch by using 
default parameters for 

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[Hohensee, Paul]

We don’t use getType, and you guessed correctly: we use the memory pool name as 
an indicator of the specific characteristics of a memory pool, in particular 
eden.

What we want is an indication of long term heap occupancy. We calculate it 
using CollectionUsage for non-eden heap memory pools, regardless of collector. 
We don’t use JMX notification, rather we periodically poll CollectionUsage for 
memory pools with names that contain “Old”, “Tenured”, or “Survivor”. We get 
the memory pools from the GarbageCollectorMXBeans (we don’t care what the 
collector names are). For the named memory pools, we sum CollectionUsage.used 
and divide by the sum of CollectionUsage.max to get a long term heap occupancy 
percentage. We don’t want to include eden because it’s really just an 
allocation buffer and not part of the storage for long-lived objects. I suppose 
we could use a negative test instead by using all memory pools with names that 
don’t include “Eden”.

The bug is that the “G1 Old Gen” memory pool isn’t being updated when the “G1 
Young Generation” collector runs a mixed collection. As far as JMX is 
concerned, that collector only knows about eden and the survivor space. The 
patch adds the old gen to the memory pools it knows about and has mixed 
collections update the old gen’s CollectionUsage.

I managed to get a submit repo run to succeed last week and it found a problem. 
I’ve uploaded a new webrev that fixes the failure of the jtreg test 
TestMemoryMXBeansAndPoolsPresence.java due to the young gen collector being 
expected to know only about eden and the survivor space.

http://cr.openjdk.java.net/~phh/8195115/webrev.hs.01/

Waiting on the submit repo to come back with a result on it.

Thanks,

Paul

From: mandy chung <mandy.ch...@oracle.com>
Organization: Oracle Corporation
Date: Monday, January 29, 2018 at 10:52 AM
To: "Hohensee, Paul" <hohen...@amazon.com>, Erik Helin <erik.he...@oracle.com>, 
David Holmes <david.hol...@oracle.com>
Cc: "serviceability-dev@openjdk.java.net" 
<serviceability-dev@openjdk.java.net>, "hotspot-gc-...@openjdk.java.net" 
<hotspot-gc-...@openjdk.java.net>
Subject: Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used 
values don't reflect mixed GC results


On 1/29/18 10:35 AM, mandy chung wrote:
Thanks for the reply Paul.   Try to understand a little more on the specific 
from gc-specific memory pool you depend on.
On 1/29/18 8:27 AM, Hohensee, Paul wrote:
A name change would affect Amazon’s heap monitoring, and thus I expect it would 
affect other users as well.

As long as there are gc-specific memory pools, we’re going to need to be able 
to identify them, and right now that’s done via name.

MemoryPoolMXBean::getType returns "heap" memory type for GC-specific memory 
pools.  Are you using this method?  Do you use the name to build in specific 
characteristic of a memory pool (e.g. eden vs old gen)?



All the mxbeans are identified by name, so that’s a general design principle. 
The only way I can think of to get rid of name dependency would be to figure 
out what abstract metrics users want to monitor and implement them for all 
collectors. HeapUsage (instantaneous occupancy) is one, CollectionUsage 
(long-lived occupancy) is another, both of these for the entire heap, not just 
particular memory pools.

The sum of HeapUsage and CollectionUsage of all heap memory pools was expected 
to give an incorrect approximation for the entire heap usage.  Are you seeing 
issue/bug with the sum result?

typo: s/an incorrect approximation/an approximation.

Mandy


Mandy


That said, imo there will always be a demand for the ability to get collector 
and memory pool specific details, so I don’t see a way to get around providing 
named entities.

Paul

From: mandy chung <mandy.ch...@oracle.com><mailto:mandy.ch...@oracle.com>
Organization: Oracle Corporation
Date: Friday, January 26, 2018 at 2:38 PM
To: "Hohensee, Paul" <hohen...@amazon.com><mailto:hohen...@amazon.com>, Erik 
Helin <erik.he...@oracle.com><mailto:erik.he...@oracle.com>, David Holmes 
<david.hol...@oracle.com><mailto:david.hol...@oracle.com>
Cc: 
"serviceability-dev@openjdk.java.net"<mailto:serviceability-dev@openjdk.java.net>
 
<serviceability-dev@openjdk.java.net><mailto:serviceability-dev@openjdk.java.net>,
 "hotspot-gc-...@openjdk.java.net"<mailto:hotspot-gc-...@openjdk.java.net> 
<hotspot-gc-...@openjdk.java.net><mailto:hotspot-gc-...@openjdk.java.net>
Subject: Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used 
values don't reflect mixed GC results


On 1/25/18 1:04 PM, Hohensee, Paul wrote:

> The JMX API spec doesn’t specify what the memory pool or garbage > collector 
> names are, but the current names are de-facto part of the > API, so if we 
> change the existing ones,

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[mandy chung]

On 1/29/18 10:35 AM, mandy chung wrote:
Thanks for the reply Paul.   Try to understand a little more on the 
specific from gc-specific memory pool you depend on.


On 1/29/18 8:27 AM, Hohensee, Paul wrote:


A name change would affect Amazon’s heap monitoring, and thus I 
expect it would affect other users as well.


As long as there are gc-specific memory pools, we’re going to need to 
be able to identify them, and right now that’s done via name.




MemoryPoolMXBean::getType returns "heap" memory type for GC-specific 
memory pools.  Are you using this method?  Do you use the name to 
build in specific characteristic of a memory pool (e.g. eden vs old gen)?



All the mxbeans are identified by name, so that’s a general design 
principle. The only way I can think of to get rid of name dependency 
would be to figure out what abstract metrics users want to monitor 
and implement them for all collectors. HeapUsage (instantaneous 
occupancy) is one, CollectionUsage (long-lived occupancy) is another, 
both of these for the entire heap, not just particular memory pools.




The sum of HeapUsage and CollectionUsage of all heap memory pools was 
expected to give an incorrect approximation for the entire heap 
usage.  Are you seeing issue/bug with the sum result?




typo: s/an incorrect approximation/an approximation.

Mandy


Mandy

That said, imo there will always be a demand for the ability to get 
collector and memory pool specific details, so I don’t see a way to 
get around providing named entities.


Paul

*From: *mandy chung <mandy.ch...@oracle.com>
*Organization: *Oracle Corporation
*Date: *Friday, January 26, 2018 at 2:38 PM
*To: *"Hohensee, Paul" <hohen...@amazon.com>, Erik Helin 
<erik.he...@oracle.com>, David Holmes <david.hol...@oracle.com>
*Cc: *"serviceability-dev@openjdk.java.net" 
<serviceability-dev@openjdk.java.net>, 
"hotspot-gc-...@openjdk.java.net" <hotspot-gc-...@openjdk.java.net>
*Subject: *Re: RFR (S): 8195115: G1 Old Gen MemoryPool 
CollectionUsage.used values don't reflect mixed GC results


On 1/25/18 1:04 PM, Hohensee, Paul wrote:


> The JMX API spec doesn’t specify what the memory pool or garbage > 
collector names are, but the current names are de-facto part of the > 
API, so if we change the existing ones, imo a CSR should be filed.


The names are implementation details but I can see how an application
might be impacted if they depend on it.  CSR approval is not strictly
necessary while I think filing one to document the change would be
good.

Does the name change impact any application you know of?  I'm trying to
understand if any improvement to API is needed so that applications
don't need to depend on the names.


Mandy

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[mandy chung]

Thanks for the reply Paul.   Try to understand a little more on the 
specific from gc-specific memory pool you depend on.


On 1/29/18 8:27 AM, Hohensee, Paul wrote:


A name change would affect Amazon’s heap monitoring, and thus I expect 
it would affect other users as well.


As long as there are gc-specific memory pools, we’re going to need to 
be able to identify them, and right now that’s done via name.




MemoryPoolMXBean::getType returns "heap" memory type for GC-specific 
memory pools.  Are you using this method?  Do you use the name to build 
in specific characteristic of a memory pool (e.g. eden vs old gen)?



All the mxbeans are identified by name, so that’s a general design 
principle. The only way I can think of to get rid of name dependency 
would be to figure out what abstract metrics users want to monitor and 
implement them for all collectors. HeapUsage (instantaneous occupancy) 
is one, CollectionUsage (long-lived occupancy) is another, both of 
these for the entire heap, not just particular memory pools.




The sum of HeapUsage and CollectionUsage of all heap memory pools was 
expected to give an incorrect approximation for the entire heap usage.  
Are you seeing issue/bug with the sum result?


Mandy

That said, imo there will always be a demand for the ability to get 
collector and memory pool specific details, so I don’t see a way to 
get around providing named entities.


Paul

*From: *mandy chung <mandy.ch...@oracle.com>
*Organization: *Oracle Corporation
*Date: *Friday, January 26, 2018 at 2:38 PM
*To: *"Hohensee, Paul" <hohen...@amazon.com>, Erik Helin 
<erik.he...@oracle.com>, David Holmes <david.hol...@oracle.com>
*Cc: *"serviceability-dev@openjdk.java.net" 
<serviceability-dev@openjdk.java.net>, 
"hotspot-gc-...@openjdk.java.net" <hotspot-gc-...@openjdk.java.net>
*Subject: *Re: RFR (S): 8195115: G1 Old Gen MemoryPool 
CollectionUsage.used values don't reflect mixed GC results


On 1/25/18 1:04 PM, Hohensee, Paul wrote:


> The JMX API spec doesn’t specify what the memory pool or garbage > 
collector names are, but the current names are de-facto part of the > 
API, so if we change the existing ones, imo a CSR should be filed.


The names are implementation details but I can see how an application
might be impacted if they depend on it.  CSR approval is not strictly
necessary while I think filing one to document the change would be
good.

Does the name change impact any application you know of?  I'm trying to
understand if any improvement to API is needed so that applications
don't need to depend on the names.


Mandy

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[Hohensee, Paul]

A name change would affect Amazon’s heap monitoring, and thus I expect it would 
affect other users as well.

As long as there are gc-specific memory pools, we’re going to need to be able 
to identify them, and right now that’s done via name. All the mxbeans are 
identified by name, so that’s a general design principle. The only way I can 
think of to get rid of name dependency would be to figure out what abstract 
metrics users want to monitor and implement them for all collectors. HeapUsage 
(instantaneous occupancy) is one, CollectionUsage (long-lived occupancy) is 
another, both of these for the entire heap, not just particular memory pools. 
That said, imo there will always be a demand for the ability to get collector 
and memory pool specific details, so I don’t see a way to get around providing 
named entities.

Paul

From: mandy chung <mandy.ch...@oracle.com>
Organization: Oracle Corporation
Date: Friday, January 26, 2018 at 2:38 PM
To: "Hohensee, Paul" <hohen...@amazon.com>, Erik Helin <erik.he...@oracle.com>, 
David Holmes <david.hol...@oracle.com>
Cc: "serviceability-dev@openjdk.java.net" 
<serviceability-dev@openjdk.java.net>, "hotspot-gc-...@openjdk.java.net" 
<hotspot-gc-...@openjdk.java.net>
Subject: Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used 
values don't reflect mixed GC results


On 1/25/18 1:04 PM, Hohensee, Paul wrote:

> The JMX API spec doesn’t specify what the memory pool or garbage > collector 
> names are, but the current names are de-facto part of the > API, so if we 
> change the existing ones, imo a CSR should be filed.

The names are implementation details but I can see how an application

might be impacted if they depend on it.  CSR approval is not strictly

necessary while I think filing one to document the change would be

good.


Does the name change impact any application you know of?  I'm trying to

understand if any improvement to API is needed so that applications

don't need to depend on the names.

Mandy

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[mandy chung]

On 1/25/18 1:04 PM, Hohensee, Paul wrote:

The JMX API spec doesn’t specify what the memory pool or garbage  > collector names are, but the current names are de-facto part of the 

> API, so if we change the existing ones, imo a CSR should be filed.

The names are implementation details but I can see how an application
might be impacted if they depend on it.  CSR approval is not strictly
necessary while I think filing one to document the change would be
good.


Does the name change impact any application you know of?  I'm trying to
understand if any improvement to API is needed so that applications
don't need to depend on the names.


Mandy

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[mandy chung]

Hi Erik,

The proposal you outline below is reasonable.  The API was designed to
allow any number of memory pools managed by a memory manager that
can represent different phases of a garbage collector or other
resource manager to expose various metrics.  How G1 exposes these
monitoring metrics is implementation specific.

More comments inlined below.

On 1/25/18, 5:27 AM, "Erik Helin"  wrote:



 Hi Paul,
 
 thanks for your interest in this area and for your patch! The

 GarbageCollectorMXBean and MemoryPoolMXBean support for G1 is in need of
 some updates, so thanks for working on this.
 
 Looking at your patch, I'm not sure that this is the direction we want

 to go in. I discussed this a bit with Thomas and Stefan J, and our
 current line of thinking is the following:
 
 - Memory pools (MemoryMXBean):

- "G1 Eden Regions"
- "G1 Survivor Regions"
- "G1 Old Regions"
- "G1 Humongous Regions"
- "G1 Archive Regions" (if CDS and/or AppCDS is used)
 


Can you describe more about G1 archive regions?  Is it an immutable region
that no object will be added or removed?


 `init` for these pools would be 0, `used` would be total size of the
 "live" objects in the used regions of that type, `committed` the total
 size of the used regions of that that type and `max` would be
 MaxHeapSize. Note that "live" here means live from the GCs point of
 view, i.e. an object might be dead in an old region but the GC will
 consider that object live until a concurrent cycle has marked through
 the heap and deemed it dead.


As specified in MemoryPoolMXBean spec, for MemoryPoolMXBean::getUsage of
a garbage-collected memory pool, the amount of used memory includes the
memory occupied by all objects in the pool including both reachable
and unreachable objects.

OTOH, getCollectionUsage returns the memory usage after JVM most recently
expended effort in recycling unused objects in this memory pool.  It
would depend on what a GC cycle is defined for G1 and at what phase
G1 can record the "collection usage" with low overhead.  Definitely
the API does not request G1 to perform any GC-like action and "collection
usage" reports how much memory is used at most recent GC cycle after some
memory has been reclaimed.

 
 - Collectors (GarbageCollectorMXBean):

- "G1 Young Collector" with the pools
  - "G1 Eden Regions"
  - "G1 Survivor Regions"
  - "G1 Humongous Regions" (due to early reclamation)
- "G1 Mixed Collector" with the pools
  - "G1 Eden Regions"
  - "G1 Survivor Regions"
  - "G1 Old Regions"
  - "G1 Humongous Regions" (due to early reclamation)
- "G1 Full Collector" with the pools
  - "G1 Eden Regions"
  - "G1 Survivor Regions"
  - "G1 Old Regions"
  - "G1 Humongous Regions" (can collect empty humongous regions)
- "G1 Concurrent Cycle" with the pools
  - "G1 Old Regions" (can collect empty old regions)
  - "G1 Humongous Regions" (can collect empty humongous regions)
 
 Note that with this design, the

 GarbageCollectorMXBean::getCollectionTime() method for "G1 Concurrent
 Cycle" would be the wall clock time from start of the first initial mark
 to end of the last cleanup (also including the time of any eventual
 young collection during the concurrent cycle). So
 GarbageCollectorMXBean::getCollectionTime() would be a mix of concurrent
 and STW time for the GarbageCollectorMXBean with name "G1 Concurrent 
Cycle".
 


GarbageCollectorMXBean API was defined prior to G1.  It's a future
enhancement to investigate how to represent the concurrent garbage
collection metrics for better monitoring purpose.  Do you have any
thought for monitoring of G1 GC metrics besides memory usage?


 Also note that the MemoryPoolMXBean with name "G1 Archive Regions" will
 not be attached to any GarbageCollectorMXBean, since those regions will
 never be collected.
 
 What do you think about this design, would it work for your use case?
 
 If we want to go ahead with this design, then I think we might have to

 file a CSR. David (who is the HotSpot CSR representative), do we have to
 file a CSR for changing the names of MemoryPoolMXBeans and
 GarbageCollectorMXBeans?


The names are not a supported interface but I'm not surprised applications
may depend on the names.  CSR as well as a release note to document
this change is reasonable.

Mandy

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[Hohensee, Paul]

Hi Erik & co, thanks for looking at this.

Would you be ok with pushing this fix (it really is a bug!) and then me doing a 
followup RFE? That way, I can backport the fix to 8u and eventually remove the 
patch I’ve already pushed to our OpenJDK8 internal release.

Some background. We used to measure heap occupancy using MemoryPoolMXBean.Usage 
and alarm on it when it got ‘too high’. The problem is that it’s an 
instantaneous measure and therefore includes an unknown amount of garbage, so 
you can’t determine where to set an alarm, because you don’t know how much of 
it will be collected in the near future. We want to detect steadily increasing 
memory use such as you’d see with a leak, so we’re switching to 
MemoryPoolMXBean.CollectionUsage, which is the usage immediately after the last 
GC that affected the memory pool. We have a synthetic metric that’s defined for 
all collectors as (sum of CollectionUsage.used for non-eden pools) / (sum of 
CollectionUsage.max-if-defined for all non-eden pools), and alarm on that. 
‘max-if-defined’ means zero if JMX returns -1, which is the ‘undefined’ value.

The JMX API spec doesn’t specify what the memory pool or garbage collector 
names are, but the current names are de-facto part of the API, so if we change 
the existing ones, imo a CSR should be filed. To avoid that, how about we keep 
the existing memory pool names the same and, in the spirit of compatibility, 
make the new ones similar to the existing ones. I.e.,

“G1 Eden Space”
“G1 Survivor Space”
“G1 Old Gen”
“G1 Humongous Space”
“G1 Archive Space”

Alternatively, we can make the existing names aliases for the new ones, though 
“Regions” seems a bit G1-specific to me and doesn’t convey the relationship to 
the equivalent spaces in the other collectors. Especially if there’s an archive 
space for the other collectors (see below).

There’s no specification requirement that memory pools be disjoint. What do you 
think of defining the archive space as a subset of the old gen? That way 
existing code can stay the same (it typically just iterates over a collector’s 
pools as above), and new code can decide exactly what it wants to report. 
Do/should the non-G1 collectors have an archive space too? If so, we should 
just call it “Archive Space” and not make it G1 specific. I’d guess for non-G1 
collectors it’s just the initial live prefix of the old gen and therefore 
ignored during a collection. In this scheme, the initial value of ‘used’ for 
all collectors’ oldgens would be the size of the archive space. All the archive 
space’s MemoryUsage attributes would have the same value.

Not attaching the archive space to any collector seems correct, because a 
collector’s memory pools are defined to be the ones that it affects.

Currently, Usage.max and CollectionUsage.max for all G1 memory pools other than 
the oldgen is -1, for “undefined”. For the oldgen it’s –Xmx. This makes it easy 
to generate aggregate metrics for all the collectors by just summing used 
values and dividing by the sum of max values as described above. It would be 
nice to keep this characteristic. Otherwise we’d have to write special-case 
code for G1, and change existing code to check for which JDK we’re running on.

I’m uncertain whether your definition of the memory pool usage fields is for 
MemoryPool.Usage or MemoryPool.CollectionUsage. Seems like the former, which is 
fine and matches the current definition, except for max.

All the existing collector names have an implicit “ Collector” suffix, e.g., 
“ConcurrentMarkSweep” means “ConcurrentMarkSweep Collector”. So, I’d use

“G1 Young”
“G1 Mixed”
“G1 Full”
“G1 Concurrent Cycle”

keep the existing “G1 Young Collection” as an alias for both “G1 Young” and “G1 
Mixed”, and keep the existing “G1 Old Collection” as an alias for “G1 Full”. If 
you accept this and the memory pool name suggestions, then strictly speaking 
you don’t need a CSR.

The definition of the “G1 Concurrent Cycle” elapsed time makes sense to me. The 
young collection would still be reported separately, right?

Paul

On 1/25/18, 5:27 AM, "Erik Helin"  wrote:

Hi Paul,

thanks for your interest in this area and for your patch! The 
GarbageCollectorMXBean and MemoryPoolMXBean support for G1 is in need of 
some updates, so thanks for working on this.

Looking at your patch, I'm not sure that this is the direction we want 
to go in. I discussed this a bit with Thomas and Stefan J, and our 
current line of thinking is the following:

- Memory pools (MemoryMXBean):
   - "G1 Eden Regions"
   - "G1 Survivor Regions"
   - "G1 Old Regions"
   - "G1 Humongous Regions"
   - "G1 Archive Regions" (if CDS and/or AppCDS is used)

`init` for these pools would be 0, `used` would be total size of the 
"live" objects in the used regions of that type, `committed` the total 
size of the used regions of that that type and `max` would be 

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[Erik Helin]

Hi Paul,

thanks for your interest in this area and for your patch! The 
GarbageCollectorMXBean and MemoryPoolMXBean support for G1 is in need of 
some updates, so thanks for working on this.


Looking at your patch, I'm not sure that this is the direction we want 
to go in. I discussed this a bit with Thomas and Stefan J, and our 
current line of thinking is the following:


- Memory pools (MemoryMXBean):
  - "G1 Eden Regions"
  - "G1 Survivor Regions"
  - "G1 Old Regions"
  - "G1 Humongous Regions"
  - "G1 Archive Regions" (if CDS and/or AppCDS is used)

`init` for these pools would be 0, `used` would be total size of the 
"live" objects in the used regions of that type, `committed` the total 
size of the used regions of that that type and `max` would be 
MaxHeapSize. Note that "live" here means live from the GCs point of 
view, i.e. an object might be dead in an old region but the GC will 
consider that object live until a concurrent cycle has marked through 
the heap and deemed it dead.


- Collectors (GarbageCollectorMXBean):
  - "G1 Young Collector" with the pools
- "G1 Eden Regions"
- "G1 Survivor Regions"
- "G1 Humongous Regions" (due to early reclamation)
  - "G1 Mixed Collector" with the pools
- "G1 Eden Regions"
- "G1 Survivor Regions"
- "G1 Old Regions"
- "G1 Humongous Regions" (due to early reclamation)
  - "G1 Full Collector" with the pools
- "G1 Eden Regions"
- "G1 Survivor Regions"
- "G1 Old Regions"
- "G1 Humongous Regions" (can collect empty humongous regions)
  - "G1 Concurrent Cycle" with the pools
- "G1 Old Regions" (can collect empty old regions)
- "G1 Humongous Regions" (can collect empty humongous regions)

Note that with this design, the 
GarbageCollectorMXBean::getCollectionTime() method for "G1 Concurrent 
Cycle" would be the wall clock time from start of the first initial mark 
to end of the last cleanup (also including the time of any eventual 
young collection during the concurrent cycle). So 
GarbageCollectorMXBean::getCollectionTime() would be a mix of concurrent 
and STW time for the GarbageCollectorMXBean with name "G1 Concurrent Cycle".


Also note that the MemoryPoolMXBean with name "G1 Archive Regions" will 
not be attached to any GarbageCollectorMXBean, since those regions will 
never be collected.


What do you think about this design, would it work for your use case?

If we want to go ahead with this design, then I think we might have to 
file a CSR. David (who is the HotSpot CSR representative), do we have to 
file a CSR for changing the names of MemoryPoolMXBeans and 
GarbageCollectorMXBeans?


Thanks,
Erik

On 01/20/2018 12:40 AM, Hohensee, Paul wrote:

I’d appreciate a review please.

Bug: https://bugs.openjdk.java.net/browse/JDK-8195115

Webrev: http://cr.openjdk.java.net/~phh/8195115/webrev.00/

The bug is that from the JMX point of view, G1’s incremental collector 
(misnamed as the “G1 Young Generation” collector) only affects G1’s 
survivor and eden spaces. In fact, mixed collections run by this 
collector also affect the G1 old generation.


This proposed fix is to record, for each of a JMX garbage collector's 
memory pools, whether that memory pool is affected by all collections 
using that collector. And, for each collection, record whether or not 
all the collector's memory pools are affected. After each collection, 
for each memory pool, if either all the collector's memory pools were 
affected or the memory pool is affected for all collections, record 
CollectionUsage for that pool.


For collectors other than G1 Young Generation, all pools are recorded as 
affected by all collections and every collection is recorded as 
affecting all the collector’s memory pools. For the G1 Young Generation 
collector, the G1 Old Gen pool is recorded as not being affected by all 
collections, and non-mixed collections are recorded as not affecting all 
memory pools. The result is that for non-mixed collections, 
CollectionUsage is recorded after a collection only the G1 Eden Space 
and G1 Survivor Space pools, while for mixed collections CollectionUsage 
is recorded for G1 Old Gen as well.


Other than the effect of the fix on G1 Old Gen MemoryPool. 
CollectionUsage, the only external behavior change is that 
GarbageCollectorMXBean.getMemoryPoolNames will now return 3 pool names 
rather than 2.


With this fix, a collector’s memory pools can be divided into two 
disjoint subsets, one that participates in all collections and one that 
doesn’t. This is a bit more general than the minimum necessary to fix 
G1, but not by much. Because I expect it to apply to other incremental 
region-based collectors, I went with the more general solution. I 
minimized the amount of code I had to touch by using default parameters 
for GCMemoryManager::add_pool and the TraceMemoryManagerStats constructors.


Tested by running the new jtreg test included in the webrev. I tried to 
use the submit repo, but it was out of order earlier