CRR (M): 7145441: G1: collection set chooser-related cleanup

[Tony Printezis]

Thanks Bengt, I applied both changed you recommended.

Tony

On 04/18/2012 08:31 AM, Bengt Rutisson wrote:
>
> Hi Tony,
>
> This looks good. Two minor things:
>
> void CollectionSetChooser::sort_regions()
>
> This for loop could be wrapped in #ifdef ASSERT:
>
>   for (uint i = 0; i < _length; i++) {
>     assert(regions_at(i) != NULL, "Should be true by sorting!");
>   }
>
> ParKnownGarbageHRClosure
> The _worker_id (which you renamed from _worker) is not used anymore 
> after you removed the logging. Remove the instance variable too?
>
> Otherwise looks good. Nice cleanup! Ship it!
> Bengt
>
>
> On 2012-04-13 20:20, Tony Printezis wrote:
>> Hi all,
>>
>> I'd like a couple of code reviews for the cleanup of the collection 
>> set chooser class. This is the whole webrev (BTW, it depends on the 
>> size_t -> uint changes that I will push soon):
>>
>> http://cr.openjdk.java.net/~tonyp/7145441/webrev.0/webrev.all/
>>
>> I broke down the change into the following five (a personal record I 
>> think!) parts:
>>
>>
>> 0. Remove a lot of unused code that was made redundant after the 
>> recent changes to the CSet chooser. I also removed the 
>> G1PrintParCleanupStats flag which we have not used for a while.
>>
>> http://cr.openjdk.java.net/~tonyp/7145441/webrev.0/webrev.0.G1CSetChooserCleanup0/ 
>>
>>
>>
>> 1. The naming style of methods / fields of the CSet chooser class was 
>> quite messy. I changed any names in ThisStyle to this_style. I also 
>> simplified some names which tended to be quite verbose and/or 
>> inaccurate (getParMarkedHeapRegionChunk() -> claim_array_chunk(), 
>> addMarkedHeapRegion() -> add_region(), etc.). Generally, not point in 
>> repeating "marked regions" given this is the only type of regions 
>> added to the CSet chooser.
>>
>> http://cr.openjdk.java.net/~tonyp/7145441/webrev.0/webrev.1.G1CSetChooserCleanup1/ 
>>
>>
>>
>> 2. Standardized the type for region nums / indexes to uint to be 
>> consistent with the rest of G1 (we had a mix of ints / jints). The 
>> problem with is is that we use a GrowableArray to keep track of all 
>> the regions added to the CSet chooser which uses ints for indexes and 
>> lengths. To avoid excessive casting I created wrappers that cast 
>> uints to ints and vice versa (couldn't come up with a more elegant 
>> solution).
>>
>> http://cr.openjdk.java.net/~tonyp/7145441/webrev.0/webrev.2.G1CSetChooserCleanup2/ 
>>
>>
>>
>> 3. I removed the sort_index field and method from the HeapRegion 
>> class, as it's not really needed any more. I also removed the 
>> assertMarkedBytesDataOK() method and, where appropriate, I call 
>> CSetChooser::verify() instead which does similar checks.
>>
>> http://cr.openjdk.java.net/~tonyp/7145441/webrev.0/webrev.3.G1CSetChooserCleanup3/ 
>>
>>
>>
>> 4. In the end, there was still some redundant code left: the 
>> total_live, total_used, etc. fields on the CalcLiveObjectsClosure and 
>> FinalCountDataUpdateClosure closures (they were used only in the 
>> G1PrintParCleanupStats output) and the known garbage, GC efficiency, 
>> etc. fields on the G1 policy (they were used in the previous 
>> implementation of "when to do mixed GCs" policy), etc. I removed all 
>> that too.
>>
>> http://cr.openjdk.java.net/~tonyp/7145441/webrev.0/webrev.4.G1CSetChooserCleanup4/ 
>>
>>
>>
>> Looking at the overall webrev, it's not too bad to review so I'd 
>> start from that one. However, I did maintain the changes on the 
>> separate patches for various reasons so I thought I'd post those here 
>> anyway.
>>
>> Tony
>>
>