RFR(S): 8152101: Move G1 concurrent refinement adjustment code out of G1CollectorPolicy (original) (raw)

Mikael Gerdin mikael.gerdin at oracle.com
Mon Mar 21 07:39:49 UTC 2016

Hi Jon,

On 2016-03-18 18:19, Jon Masamitsu wrote:

Mikael,

Looks good.

Thanks.

Minor question that should not stall you from integrating. http://cr.openjdk.java.net/~mgerdin/8152101/webrev.0/src/share/vm/gc/g1/concurrentG1Refine.cpp.frames.html

How did you decide to extract the policy and pass it to the constructor here 58 G1CollectorPolicy* policy = g1h->g1policy(); 59 ConcurrentG1Refine* cg1r = new ConcurrentG1Refine(g1h, &policy->predictor()); as opposed to doing the same in the ConcurrentG1Refine constructor (since g1h is already passed to the constructor and available in the constructor to get the policy)? Nothing wrong with that but my natural inclination would have been to not pass another parameter to the constructor.

It was an attempt to decouple the cg1r from knowledge about how to find the G1Predictions instance. In its current form, G1 relies heavily on passing around pointers to the G1CollectedHeap both for initialization of objects and sometimes for the functionality of methods. This forces the objects and methods to contain knowledge about how to find a certain object by following (sometimes several) steps of pointers.

Instead of reducing the number of parameters passed to the constructor I was attempting to move towards reducing the size of the "API surface" which is exposed to the ConcurrentG1Refine constructor.

Side-tracking a bit...

Looking at the particular example of ConcurrentG1Refine there is actually only a small part of the code which uses the g1h parameter. The conc refine does not ues it, it passes it along to the hot card cache constructor. The hot card cache constructor stores it in a member which is never accessed and passes it along to the card counts object. The card counts object uses the heap pointer during initialization and for a call to heap_region_iterate during full gcs. Clearly there is only a minor dependency on the heap object here and I would argue that the code should not even pass the heap pointer through the constructors.

If G1CardCounts::initialize and G1CardCounts::clear_all had a g1h parameter then the entire chain of g1h constructor parameters I described above can be removed.

This simplification of the initialization sequence also makes it easier to write unit tests for the classes. In a unit testing context it is problematic to be forced to rely on initialization of large global objects, such as G1CollectedHeap, in order to create an instance of a class you want to test.

/Mikael

Jon On 3/17/2016 6:32 AM, Mikael Gerdin wrote: Hi again,

On 2016-03-17 14:27, Mikael Gerdin wrote: Hi all,

Here's yet another small G1CollectorPolicy cleanup patch. This time I want to move adjustconcurrentrefinement out of the policy into the ConcurrentG1Refine class. The method almost exclusively operates on the ConcurrentG1Refine so in my mind this makes perfect sense. Bug: https://bugs.openjdk.java.net/browse/JDK-8152101 Webrev: http://cr.openjdk.java.net/~mgerdin/8152101/webrev.0/ Testing: JPRT, RBT GC testing, Perf testing in aggregate with the previous changes showed NO significant changes. oops. /Mikael

/Mikael

More information about the hotspot-gc-dev mailing list