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

[Kim Barrett]

> 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.

------------------------------------------------------------------------------ 
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.

------------------------------------------------------------------------------ 
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.

------------------------------------------------------------------------------
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. 

------------------------------------------------------------------------------ 
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. 

------------------------------------------------------------------------------