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

[John Cuthbertson]

Hi Tony,

This looks good to me. Very nice cleanup.

JohnC

On 04/13/12 11: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
>