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

[Mikael Gerdin]

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->g1_policy();
> 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 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.
>>
>> oops.
>>
>> /Mikael
>>
>>>
>>> /Mikael
>