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

Fri Nov 4 10:55:43 UTC 2011

Tony,

On 2011-11-03 18:11, Tony Printezis wrote:
> Bengt,
>
> The latest webrev that includes the renaming you suggested (*num -> 
> *length and add_to_collection_set() -> add_old_region_to_cset()) is here:
>
> http://cr.openjdk.java.net/~tonyp/7097002/webrev.3/webrev.all/
>
> Here are the changes compared to the previous webrev if you don't want 
> to look at the whole thing:
>
> http://cr.openjdk.java.net/~tonyp/7097002/webrev.3/webrev.1.G1PredRefactorLatest/ 
>
>
> I don't think I missed any other suggestion you made, right?

Yes, this looks good. Thanks!

Bengt

>
> Tony
>
> On 11/01/2011 09:28 AM, Bengt Rutisson wrote:
>>
>> Tony,
>>
>> On 2011-10-31 18:04, Tony Printezis wrote:
>>> 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.
>>
>> Good. And thanks for the background info.
>>
>>>
>>>> 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.
>>
>> Ok. That's fine.
>>
>>>> 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?
>>
>> Sounds good. :-)
>>
>> Bengt
>>
>>>> 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
>>>>>>>
>>>>>>>
>>>>
>>