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

[Tony Printezis]

Hi all,

Thanks to John Cuthbertson for looking at this. Here are the updated 
webrevs based on his comments:

overall:

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

part 1:

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

part 2:

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

Tony

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:
>
> 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
>
>