Thanks,
Paul
On 6/18/18, 7:09 AM, "Erik Helin" <erik.he...@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.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.
> > > >
> > > >
> > > >
> > >
> > >
> > >
> > >
> >
> >
>
>
