(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]?


[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.
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, 
         >      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 
         >          > memory pools, whether that memory pool is affected by all 
         >          > using that collector. And, for each collection, record 
whether or not
         >          > all the collector's memory pools are affected. After each 
         >          > 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 
         >          > CollectionUsage is recorded after a collection only the 
G1 Eden Space
         >          > and G1 Survivor Space pools, while for mixed collections 
         >          > 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.

Reply via email to