RFR(L) 8154154: Separate G1 specific policy code from the CollectorPolicy class hierarchy

[Mikael Gerdin]

Hi Kim,

On 2016-04-15 01:06, Kim Barrett wrote:
>> On Apr 14, 2016, at 7:23 AM, Mikael Gerdin <mikael.gerdin at oracle.com> wrote:
>>
>> Hi all,
>>
>> Please review this change to split the class G1CollectorPolicy.
>>
>> G1 has traditionally had all its policy related code in G1CollectorPolicy.
>> This is somewhat problematic since the JVM heap bootstrap process is designed to first create a CollectorPolicy and then pass that to the CollectedHeap constructor.
>> To make it easier to understand which parts of the G1 policy are related to pre-heap-initialization and which parts are related to the runtime policy decisions I suggest that this is broken up into two classes:
>> - G1CollectorPolicy which implements the CollectorPolicy interface
>> - G1Policy which contains the implementation of the runtime policy for the G1 collector.
>>
>> While updating #includes to refer to the new file I've instead opted to remove them where there was in fact no reference to the G1 policy in the file.
>>
>> Initialization:
>> In this change I've tried to keep the initialization sort of unchanged. The G1CollectedHeap constructor calls create_g1_policy() fairly early on in case some other objects depend on the policy object being available during initialization.
>> I did also take the opportunity to fix the initialization of G1CollectionSet to pass the policy object to its constructor.
>>
>> Patch notes:
>> G1YoungGenSizer was slightly modified. Instead of having post_heap_initialize() figure out and set the MaxNewSize flag I've moved this to a method called from G1Policy::init(), at this point the max_young_length() can be safely determined and from what I can see nobody is expecting the value of the MaxNewSize to stay unchanged until the end of G1CollectedHeap::initialize()
>>
>> Testing: RBT GC Testing, JPRT, Performance testing on aurora.
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8154154
>> Webrev: http://cr.openjdk.java.net/~mgerdin/8154154/webrev.0/
>
> OK, this review was *much* easier than I expected.
>
> Looks good, with just a few very minor comments.

Thanks.
I'd like to pick up on your offer of deferring the cleanups to new RFEs 
since this change feels large enough as-is and I've tried to avoid 
changing too much of the semantics in this change.

>
> ------------------------------------------------------------------------------
> src/share/vm/gc/g1/concurrentG1Refine.cpp
>    30 #include "gc/g1/g1Policy.hpp"
>    59   G1Policy* policy = g1h->g1_policy();
>
> These changes will be obsoleted by my in-review changes for 8133051.
> One of use is going to have a (minor) merge collision to deal with.

Yep, I don't mind taking the merge hit since your change already has a 
bunch of reviews.

>
> ------------------------------------------------------------------------------
> src/share/vm/gc/g1/g1CollectorPolicy.hpp
>    45   void post_heap_initialize() {} // Nothing needed.
>
> With this change, I'm not sure post_heap_initialize is worth keeping.
> The only other implementation, GenCollectedHeap, is just an assert.
>
> Feel free to defer to a new RFE.
>
> ------------------------------------------------------------------------------
> src/share/vm/gc/g1/g1CollectorPolicy.hpp
>    43   G1CollectorPolicy* as_g1_policy() { return this; }
>
> [Change removed virtual qualifier.]
>
> Are as_g1_policy and is_g1_policy actually used for anything?  In a
> pre-change tree I see no callers of is_g1_policy, and the only caller
> of as_g1_policy is is_g1_policy.  Maybe these can just be deleted?
> That would be good, because having them in the CollectorPolicy API is
> pretty gross.
>
> Feel free to defer to a new RFE.
>
> ------------------------------------------------------------------------------
> src/share/vm/gc/g1/g1ConcurrentMark.cpp
>   742   G1CollectedHeap*   g1h = G1CollectedHeap::heap();
>   743   G1Policy* g1p = g1h->g1_policy();
>
> For consistency, the extra whitespace before g1h should probably also
> be removed.

Oops, yes.

>
> ------------------------------------------------------------------------------
> src/share/vm/gc/g1/g1YoungGenSizer.hpp
>    88   // Calculate the maximum length of the young gen given the number of regions
>    89   // depending on the sizing algorithm.
>    90   void compute_max_new_size(uint number_of_heap_regions);
>
> A name of calculate_xxx suggests to me that it computes and returns a
> value.  This function is called for side effect, e.g. it updates
> MaxNewSize.

I agree, how about record_max_new_size?

>
> ------------------------------------------------------------------------------
> src/share/vm/gc/g1/g1Policy.cpp
>    59   _short_lived_surv_rate_group =
>    60     new SurvRateGroup(&_predictor, "Short Lived", G1YoungSurvRateNumRegionsSummary);
>    61   _survivor_surv_rate_group =
>    62     new SurvRateGroup(&_predictor, "Survivor", G1YoungSurvRateNumRegionsSummary);
>    63
>    64   _phase_times = new G1GCPhaseTimes(ParallelGCThreads);
>
> [pre-existing]
> Is there any reason not to move these into the initialization list,
> rather than initializing via assignment?  And there's more like this
> later.
>
> Feel free to defer to a new RFE.
>
> ------------------------------------------------------------------------------
> src/share/vm/gc/g1/g1Policy.cpp
>    82 G1Policy::~G1Policy() {
>    83   delete _ihop_control;
>    84 }
>
> [pre-existing]
> There's a whole bunch of stuff that isn't being cleaned up here.
>
> Feel free to defer to a new RFE.
>
> ------------------------------------------------------------------------------
>