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

[Mikael Gerdin]

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.

/Mikael

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