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

Mikael Gerdin mikael.gerdin at oracle.com
Mon Mar 7 08:40:21 UTC 2016

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, inccsetrecordedrslengths and 123 // inccsetpredictedelapsedtimems, are updated by the thread

158 // We could have updated inccsetrecordedrslengths and 159 // inccsetpredictedelapsedtimems 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 finalizeoldcsetpart() 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 g1policy(policy), 1745 collectionset(new G1CollectionSet(this, policy)), But it isn't until G1CollectedHeap::initialize() calls g1policy()->init() that the ref to the G1CollectedSet will be stored in the G1CollectorPolicy: 473 g1 = G1CollectedHeap::heap(); 474 collectionset = g1->collectionset(); At a minimum, if feels like there should be an assertion here that collectionset'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/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