RFR (M) JDK-8151178: Move the collection set out of the G1 collector policy
tom.benson at oracle.com
Fri Mar 4 16:58:00 UTC 2016
In g1CollectionSet.cpp, there are still some references to old names in
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
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_) :
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
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.
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.
>> 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
>>> policies I suggest that the collection set code should be moved out of
>>> 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.
>>> I've also split the change into three webrevs in the version1
>>> 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.
>>> Testing: JPRT, RBT GC testing.
More information about the hotspot-gc-dev