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

[Bengt Rutisson]

Hi Tony,

I think this looks really good. Thanks for cleaning it up!

Some minor suggestions:

The method G1CollectorPolicy::add_to_collection_set() is only used for 
adding old regions to the collection set. How about renaming it to 
something like add_old_region_to_collection_set()?

I would like to consider "cset" a prefix for the methods that return 
information about the collection set. That would mean that methods like

young_cset_region_num()
old_cset_region_num()
eden_cset_region_num()
survivor_cset_region_num()
init_cset_region_nums()

could be called something like:

cset_young_region_num()
cset_old_region_num()
cset_eden_region_num()
cset_survivor_region_num()
cset_init_region_nums()

If you do this change there are also some local variables that are used 
to call init_cset_region_nums() that would benefit from the same kind of 
renaming.

Finally, to me the *_num() name could indicate something else than the 
count(), size() or lenght(). How about renaming them *_num() mehods (and 
corresponding variables) to *_length() ? I am more used to seeing the 
"length" name when we use lists of regions in the G1 code.

Anyway, these were all tiny nit picks. Overall it looks good. Ship it!

Bengt




On 2011-10-14 12:58, Tony Printezis wrote:
> 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
>>>
>>>