RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results
hohensee at amazon.com
Fri Jun 8 17:13:35 UTC 2018
Back after a long hiatus...
Thanks, Eric, for your review. Here's a new webrev incorporating your recommendations.
TIA for your re-review. Plus, may I have another reviewer look at it please?
On 2/26/18, 8:47 AM, "Erik Helin" <erik.helin at oracle.com> wrote:
a couple of comments on the patch:
+ 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).
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).
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.
On 02/22/2018 09:54 PM, Hohensee, Paul wrote:
> Ping for a review please.
> 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.
More information about the hotspot-gc-dev