(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.
>
>
>
