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 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 today, so I’d 
>> be much obliged if someone could run it through mach5 and sponsor an 
>> eventual push. I successfully ran a JDK8 version of the patch through 
>> all the JDK8 jtreg tests as well as the JDK8 TCK.
>>
>> Thanks,
>>
>> Paul
>>