[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-hotspot-gc-dev
Subject: Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC resul
From: "Hohensee, Paul" <hohensee () amazon ! com>
Date: 2018-06-18 18:17:43
Message-ID: A052B3EC-2F30-4D09-B654-66B4152B03C1 () amazon ! com
[Download RAW message or body]
Thanks, Erik.
On 6/18/18, 10:26 AM, "Erik Helin" <erik.helin@oracle.com> wrote:
On 06/18/2018 06:14 PM, Hohensee, Paul wrote:
> Thanks, Eric!
>
> I'd push, but it seems I don't seem to have permission at the moment. Who \
should I contact to get that fixed?
That would be ops@openjdk.java.net.
Thanks,
Erik
> Thanks,
>
> Paul
>
> On 6/18/18, 7:09 AM, "Erik Helin" <erik.helin@oracle.com> wrote:
>
> On 06/16/2018 09:00 PM, Hohensee, Paul wrote:
> > Thanks for the re-review, Erik. New webrev with your fixes:
> >
> > http://cr.openjdk.java.net/~phh/8195115/webrev.04/
>
> The patch is good to go now, Reviewed.
>
> Thanks,
> Erik
>
> > Need another reviewer, please.
> >
> > Thanks,
> >
> > Paul
> >
> > On 6/16/18, 1:25 AM, "Erik Helin" <erik.helin@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.helin@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@openjdk.java.net on behalf of \
hohensee@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@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@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]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic