[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC resul
From:       Erik Helin <erik.helin () oracle ! com>
Date:       2018-02-26 16:46:28
Message-ID: 9f972398-115f-06ad-1e0c-513abceb097a () oracle ! com
[Download RAW message or body]

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@openjdk.java.net on behalf of hohensee@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.
> 
> 
> 


[prev in list] [next in list] [prev in thread] [next in thread]