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" <erik.helin at oracle.com> 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" <serviceability-dev-bounces at openjdk.java.net on behalf of hohensee at amazon.com> 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" <erik.helin at oracle.com> 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" <serviceability-dev-bounces at openjdk.java.net on behalf of hohensee at amazon.com> 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.
    >          >
    >          >
    >          >
    >          
    >      
    >      
    >