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

Tom Benson tom.benson at oracle.com
Fri Mar 4 16:58:00 UTC 2016

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/webrevfull I've also split the change into three webrevs in the version1 directory: webrev1 consists of moving the code and changing code accessing collection set related methods webrev2 consists of renaming almost all methods in the new class to not contain redundant collection set naming webrev3 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

More information about the hotspot-gc-dev mailing list