RFR (M) JDK-8151178: Move the collection set out of the G1 collector policy

[Tom Benson]

Hi Mikael,
In g1CollectionSet.cpp, there are still some references to old names in 
comments:
    122   // The two "main" fields, _inc_cset_recorded_rs_lengths and
    123   // _inc_cset_predicted_elapsed_time_ms, are updated by the thread

    158   // We could have updated _inc_cset_recorded_rs_lengths and
    159   // _inc_cset_predicted_elapsed_time_ms directly but we'd need 
to do

Also extra blank lines at 335 and 353.

Also one ref to an old name in g1CollectionSet.hpp:
     55 // pause, and incremented in finalize_old_cset_part() when 
adding old regions

Did you mean to leave this in?
     174 // XXX MGFIXME detgc!

The initialization of the links between Policy and CollectionSet seems a 
little odd to me.  In the G1CollectedHeap constructor, the 
G1CollectedSet is created and given the policy ref, which it saves in 
its G1CollectorPolicy* _g1p :

    1742 G1CollectedHeap::G1CollectedHeap(G1CollectorPolicy* policy_) :
    1743 CollectedHeap(),
    1744 _g1_policy(policy_),
    1745 _collection_set(new G1CollectionSet(this, policy_)),

But it isn't until G1CollectedHeap::initialize() calls 
g1_policy()->init() that the ref to the G1CollectedSet will be stored in 
the G1CollectorPolicy:

    473 _g1 = G1CollectedHeap::heap();
    474 _collection_set = _g1->collection_set();

At a minimum, if feels like there should be an assertion here that 
_collection_set's "G1CollectorPolicy* _g1p" already points to This.   
But it might make more sense to have a way to init the CollectionSet's 
policy ptr at this point, rather than in its constructor.

Tom

On 3/4/2016 11:06 AM, Mikael Gerdin wrote:
> Hi Jesper,
>
> On 2016-03-04 15:57, Jesper Wilhelmsson wrote:
>> Hi Mikael,
>>
>> Looks OK.
>>
>>  From a purely aesthetic point of view I would appreciate if you cleaned
>> up lines 78, 145, 156 in g1CollectionSet.cpp
>
> Will do.
>
>>
>> Also, most files could use a new copyright date if you care about that
>> sort of thing :)
>
> I'll run the magic script :)
>
> Thanks for the review.
>
> /Mikael
>
>>
>> Thanks,
>> /Jesper
>>
>>
>> Den 4/3/16 kl. 09:35, skrev Mikael Gerdin:
>>> Hi all,
>>>
>>> As part of an attempt to make it easier for G1 to contain multiple
>>> collector
>>> policies I suggest that the collection set code should be moved out of
>>> the
>>> current G1 collector policy class into a class of its own.
>>>
>>> I've tried to hold back on doing further cleanups in this patch in
>>> order to make
>>> it easier to review the changes.
>>>
>>> I do have some ideas on how the collection set code could be further
>>> cleaned up
>>> but I'm not sure if I will have time to test them properly before FC.
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~mgerdin/8151178/version1/webrev_full
>>>
>>> I've also split the change into three webrevs in the version1 
>>> directory:
>>> webrev_1 consists of moving the code and changing code accessing
>>> collection set
>>> related methods
>>> webrev_2 consists of renaming almost all methods in the new class to
>>> not contain
>>> redundant collection set naming
>>> webrev_3 restores the functionality for collector policy extensions to
>>> hook into
>>> collection set selection.
>>>
>>> Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-8151178
>>>
>>> Testing: JPRT, RBT GC testing.
>>>
>>> Thanks6
>>> /Mikael