CRR (M): 7097002: G1: remove a lot of unused / redundant code from the G1CollectorPolicy class
tony.printezis at oracle.com
Fri Oct 14 03:58:53 PDT 2011
While working on the prediction code I found another 200 lines that can
be removed (I think this was part of the original prediction code which
has been superseded by the current version). I'd like to piggy-back this
removal on this CR so that I don't open a new one. It also fits within
the "cleanup" scope of this CR anyway. Here's the latest overall webrev:
The first two parts are exactly what I had below:
The new part is this:
On 10/12/2011 02:22 PM, Tony Printezis wrote:
> Hi all,
> Thanks to John Cuthbertson for looking at this. Here are the updated
> webrevs based on his comments:
> part 1:
> part 2:
> On 10/06/2011 04:03 PM, Tony Printezis wrote:
>> 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:
>> 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.
>> 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.
>> 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
>> 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.
More information about the hotspot-gc-dev