Thanks for the re-review, Erik. New webrev with your fixes: http://cr.openjdk.java.net/~phh/8195115/webrev.04/
Need another reviewer, please. Thanks, Paul On 6/16/18, 1:25 AM, "Erik Helin" <erik.he...@oracle.com> wrote: On 06/15/2018 10:21 PM, Hohensee, Paul wrote: > 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: Thanks, the change of -Xmx and -Xms to 12m now also makes the test pass on my workstation. > http://cr.openjdk.java.net/~phh/8195115/webrev.03/ There seems to be some trailing whitespace in the patch, have you run jcheck (or `hg diff` which highlights trailing whitespace in red)? Please see + TraceMemoryManagerStats tms(&_memory_manager, gc_cause(), + collector_state()->yc_type() == Mixed /* allMemoryPoolsAffected */); + ^---- whitespace and +int MemoryManager::add_pool(MemoryPool* pool) { + int index = _num_pools; ^---- whitespace Another small comment, I would have written +void GCMemoryManager::add_pool(MemoryPool* pool) { + int index = MemoryManager::add_pool(pool); + _pool_always_affected_by_gc[index] = true; +} + +void GCMemoryManager::add_pool(MemoryPool* pool, bool always_affected_by_gc) { + int index = MemoryManager::add_pool(pool); + _pool_always_affected_by_gc[index] = always_affected_by_gc; +} + as +void GCMemoryManager::add_pool(MemoryPool* pool) { + add_pool(pool, true); +} + +void GCMemoryManager::add_pool(MemoryPool* pool, bool always_affected_by_gc) { + int index = MemoryManager::add_pool(pool); + _pool_always_affected_by_gc[index] = always_affected_by_gc; +} + to not have to two duplicate implementations of GCMemoryManager::add_pool. Would you mind updating the patch with this change (and remove the trailing whitespace)? Thanks, Erik > Thanks, > > Paul > > On 6/12/18, 6:52 AM, "Erik Helin" <erik.he...@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-boun...@openjdk.java.net on behalf of hohen...@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.he...@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-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-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. > > > > > > > > > > > > > > > > > > >