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