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

[Erik Helin]

On 2016-04-18, Mikael Gerdin wrote:
> Hi Erik,
> 
> On 2016-04-15 16:44, Erik Helin wrote:
> >On 2016-04-14, Mikael Gerdin 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/
> >
> >Nice work!
> >
> >I think the code that sets up MaxGCPauseMillis can be in
> >G1Policy instead of G1CollectorPolicy, that would be nicer IMO (since
> >then G1CollectorPolicy only handles the initial heap size). Would it
> >make sense to rename G1CollectorPolicy to G1InitialHeapSizePolicy?
> 
> I'd like to leave the MaxGCPauseMillis cleanup to another RFE, I'll take
> care of it.
> I'm not too fond of G1InitialHeapSizePolicy, I'd prefer to keep the current
> naming for now and plan to completely remove the requirement of a
> CollectorPolicy object completely in the future.

Alright, sounds good. Reviewed.

Thanks,
Erik

> /Mikael
> 
> >
> >Thanks,
> >Erik
> >
> >>Thanks
> >>/Mikael