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

[Mikael Gerdin]

Hi Tom,

On 2016-03-04 17:58, Tom Benson wrote:
> 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!

I'll take care of the above!


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

I agree, that makes sense.

/Mikael

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