CRR (M): 7097002: G1: remove a lot of unused / redundant code from the G1CollectorPolicy class

Tony Printezis tony.printezis at oracle.com
Thu Oct 6 20:03:27 UTC 2011


Hi all,

This is G1CollectorPolicy cleanup #2, now I got the first one (7088680) 
code reviewed (thanks to Ramki and Bengt). BTW, the webrevs below were 
done on top of the changes for 7088680. So if you want to apply them to 
your workspace you have to apply the patch for 7088680 first (at least 
until it's pushed to the GC repo). And the webrevs do not include the 
changes for 7088680 despite what the top-level webrev pages say (I checked).

The motivation for these changes was that there's a lot of unused and 
redundant code in the G1CollectorPolicy class that's been complicating 
some other changes I've been working on (simplify the pause prediction 
model, extend the ergo decision logging, revamp the eden / survivor 
lists, etc.). So, I thought I'd do this cleanup first so that we can get 
it tested separately (as it's not trivial, at least wrt its size) and to 
also keep the code reviews somewhat smaller and simpler.

The overall webrev is this one:

http://cr.openjdk.java.net/~tonyp/7097002/webrev.all/

Here it is in two parts in case it makes the code reviews easier. BTW, I 
could be persuaded to push each part as a separate changeset if you 
think they are kind of unrelated. That'd be easy to do (I have them as 
two separate MQ patches anyway).

1) Remove methods and fields only used by the PREDICTIONS_VERBOSE switch 
and G1PolicyVerbose flag, both of which are also removed.

http://cr.openjdk.java.net/~tonyp/7097002/webrev.0.G1PredRefactorVerboseRemoval/

We'll replace the functionality of PREDICTIONS_VERBOSE and 
G1PolicyVerbose with extra logging using G1's ergo decision logging 
framework. So, it's good to get rid of all the code that is otherwise 
not used. This is a pure code removal change, I don't believe I added or 
modified anything else.

2) Avoid redundant / unnecessary region counting.

http://cr.openjdk.java.net/~tonyp/7097002/webrev.1.G1PredRefactorOtherRemoval/

It turns out that we have no less than six (!!!) ways of counting 
regions in the CSet:

a) YoungList : maintains the count of survivor and eden regions that are 
added to it
b) _inc_cset_size : the number of young regions added to the incremental 
CSet, which is the same as the total count the YoungList maintains
c) _collection_set_size : the number of regions added to the CSet, which 
is the same as _inc_cset_size plus any old regions we add to the CSet
d) _inc_cset_young_index : used to set the young CSet index of the young 
regions (0 for the oldest region, 1 for the second oldest region, etc.) 
which is again essentially the same as _inc_cset_size
e) _young_cset_length : the number of young regions in the CSet, which 
is already counted by the YoungList
f) _recorded_young_regions / _recorded_non_young_regions : the number of 
young / old regions in the CSet, again the former is counted by the 
YoungList and the latter is included in _collection_set_size

I replaced all this by three fields that reflect the number of eden, 
survivor, and old regions in the CSet and a few accessor methods. The 
first two are set using the information the YoungList maintains, the 
third one is incremented every time we add an old region to the CSet.

The changes are mostly straightforward: remove the unnecessary fields 
and methods and replace them with the new accessor methods. The only 
slightly tricky part was to make sure the young CSet indexes were set 
correctly now that we don't maintain the _inc_cset_young_index (this 
just needed a small refactoring).

In addition, we maintain counts for the number of mutator allocation 
regions that we allocate. Given that we used to allow such allocations 
to be satisfied out of the old generation, we maintain two counts: one 
for young allocations and one for tenured allocations. It was helpful to 
know what percentage of allocations were satisfied out of the old gen. 
Given that all mutator allocation regions are now allocated out of the 
young gen, this information is irrelevant. So, I also removed those 
fields and associated code.

Tony




More information about the hotspot-gc-dev mailing list