CRR (M): 7097002: G1: remove a lot of unused / redundant code from the G1CollectorPolicy class
Tony Printezis
tony.printezis at oracle.com
Thu Nov 3 17:11:06 UTC 2011
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?
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
>>>>>>
>>>>>>
>>>
>
More information about the hotspot-gc-dev
mailing list