RFR(S): 8152101: Move G1 concurrent refinement adjustment code out of G1CollectorPolicy
mikael.gerdin at oracle.com
Mon Mar 21 07:39:49 UTC 2016
On 2016-03-18 18:19, Jon Masamitsu wrote:
> Looks good.
> Minor question that should not stall you from integrating.
> How did you decide to extract the policy and pass it to the
> constructor here
> 58 G1CollectorPolicy* policy = g1h->g1_policy();
> 59 ConcurrentG1Refine* cg1r = new ConcurrentG1Refine(g1h,
> 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
> 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
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.
> 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 adjust_concurrent_refinement 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.
More information about the hotspot-gc-dev