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-boun...@openjdk.java.net on behalf of hohen...@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-8195115Webrev: 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.
