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

[Tony Printezis]

Hi again,

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:

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

The first two parts are exactly what I had below:

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

The new part is this:

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

Thanks!

Tony

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