CRR (M): 7114678: G1: various small fixes, code cleanup, and refactoring

[Tony Printezis]

Hi all,

I have an updated webrev for these changes:

http://cr.openjdk.java.net/~tonyp/7114678/webrev.1/webrev.all/

The breakdown is as before:

http://cr.openjdk.java.net/~tonyp/7114678/webrev.1/webrev.0.G1LongLineFix/
http://cr.openjdk.java.net/~tonyp/7114678/webrev.1/webrev.1.G1PreHRIter/

(the latter is similar to the first version, with some additional 
changes to deal with Jon's dynamic GC worker changeset)

And I have some additional refactoring which will help to keep some 
loops that use the iterators more concise:

http://cr.openjdk.java.net/~tonyp/7114678/webrev.1/webrev.2.G1PreHRIterRefactor/

Here's the description from the CR:

* We have a lot of switch statements on the VerifyOption embedded in the 
code. I abstracted them away in dedicated methods on G1CollectedHeap.

* Introduced the CSetChooserParUpdater class which is used to add 
regions to the CollectionSetChooser array, either in parallel or (for 
completeness) serially. Currently, the code for the latter is embedded 
in the ParKnownGarbageHRClosure class and it's good to abstract it away.

Tony

On 11/22/2011 10:49 AM, Tony Printezis wrote:
> Hi all,
>
> I have a bunch of small fixes I'd like to get reviewed. A lot of these 
> changes are a prelude to changing the way we iterate over heap regions 
> in G1 and I'd like to push them separately so that they can get tested 
> separately.
>
> The webrev is here:
>
> http://cr.openjdk.java.net/~tonyp/7114678/webrev.0/webrev.all/
>
> It might be easier to look at it in two pieces given that one very 
> long line in g1CollectedHeap.cpp is making some view of the webrev a 
> bit hard to read (and with apologies as said long line was my fault). 
> So, here's the webrev to break down this line:
>
> http://cr.openjdk.java.net/~tonyp/7114678/webrev.0/webrev.0.G1LongLineFix/ 
>
>
> And here's the rest:
>
> http://cr.openjdk.java.net/~tonyp/7114678/webrev.0/webrev.1.G1PreHRIter/
>
> I pasted the list of fixes (taken from the CR) below. Even though 
> there are lots of them, most of them are small / very self contained. 
> If you feel I should push some of these separately, I'll be happy to 
> do so.
>
> Tony
>
>
> These fixes include (in no particular order):
>
> * Split up a very long single line "Heap Regions: (Y=young(eden), 
> SU=young(survivor), ..." into multiple ones.
>
> * Changed some of the iterations (in particular, all parallel 
> iterations) to skip continues humongous regions. This will ultimately 
> make the parallel iterator easier to write. In some cases, I had to 
> put logic in the body of the iteration to handle humongous regions 
> specially (e.g., when resetting GC time stamps).
>
> * Introduced G1CollectedHeap::reset_gc_time_stamps(HeapRegion* hr) 
> which resets the GC time stamp of the given region, as well as any 
> associated continues humongous regions if hr is starts humongous.
>
> * Introduced and call (non-product) check_gc_time_stamps() method that 
> ensures the GC time stamps of all regions have been correctly reset. 
> (If this sounds a bit paranoid: the GC time stamps getting out of sync 
> can result in really subtle and hard-to-reproduce/detect bugs.)
>
> * Factored out two heap region iterations into separate methods: 
> clear_rsets_post_compaction() (the iteration with 
> PostMCRemSetClearClosure) and print_hrs_post_compaction() (the 
> iteration with PostCompactionPrinterClosure).
>
> * Introduced G1CollectedHeap::start_region_for_worker() which 
> calculates which region each parallel worker should start from during 
> parallel iterations.
>
> * In G1MarkSweep, we currently use a closure to find the first region 
> in the heap. This is a bit unnecessary. I replaced it with getting 
> region 0 directly.
>
> * Re-implemeneted the HeapRegion::next_compaction_space() not to use 
> an iterator. Iterators actually wrap around which is not appropriate 
> here.
>
> * Remove G1CollectedHeap::heap_region_iterate_from() as it's not used 
> any more.
>
> * I removed the FILTERINTOCSCLOSURE_DOHISTOGRAMCOUNT and 
> FILTEROUTOFREGIONCLOSURE_DOHISTOGRAMCOUNT switches, along with any 
> associated fields and variables, as they are not used any more.
>
> * verify_dirty_young_regions() does not call dirty_dirty_young_list() 
> on first_survivor_region() any more as it's unnecessary: the list 
> starting from first_region() actually includes the survivors.
>
> * Removed the G1RSCountHisto develop parameter and associate closures.
>
> * Updated the HS_FORMAT macros to also show whether a region is 
> humongous (HS / HC), old (O), or free (F) instead of only showing 
> whether it's eden or survivor (the abbreviations are the same as what 
> we use for our per-region output).
>
> * Introduced two convenience methods on HeapRegion:
> - region_num() returns the number of distinct regions that are covered 
> by the region (1 if non-humongous, >= 1 if humongous); I also updated 
> the HeapRegionSet classes to use this method instead of essentially 
> replicating the functionality
> - last_hc_index() returns the index+1 of the last continues humongous 
> region associated with the target starts humongous region
>
>
>
>