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

[Tony Printezis]

Bengt,

On 10/31/2011 9:12 AM, Bengt Rutisson wrote:
>
> Hi Tony,
>
> I think this looks really good. Thanks for cleaning it up!

Np and thanks for looking at it.

> 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()?

Good point and will do. Quick "history": we used to add all regions to 
the CSet with that method. But, since John worked on the incremental 
CSet building, it's not needed any more for eden / survivors and it's 
currently only used for old regions. But, you're right, we should rename it.

> 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()

Well, it depends on how you see it. If you consider "cset_region" as a 
"region in the collection set", then young_cset_region is "a young 
region in the collection set" which is what I went for. I also like 
having the young_, old_, etc. as the first word in the name as (at least 
IMHO) it makes a bit clearer which regions we are referring to. Compare:

foo = cset_young_region_num() + cset_old_region_num()

to:

foo = young_cset_region_num() + old_cset_region_num()

I think the "young" and "old" in the names pop out a bit more in the 
latter version. I'm inclined to leave these names as they are.

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

(half -seriously) a lot of thought went into that name. :-) :-) :-) 
Let's see:

- count : This would be OK, but I don't think we use "count" in this 
context much in HotSpot.
- size : This is used instead of length / number in HotSpot and, IMHO, 
is misleading. A count of regions is not a size (the size would be how 
much space they take up).
- length : It implies a list to me and, yes, we do use lists to keep 
track of the CSet regions but maybe we'll do that in a different way in 
the future. (Note that the policy class should not really know how the 
regions are maintained; it should only care about how many of them there 
are.)

...which is why I used "num". Having said that, the HeapRegionSetBase 
class uses length for the number of regions so I'll replace num with 
length to be consistent with that. Sounds good?

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

Thanks!

Tony

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