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