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

Tony Printezis tony.printezis at oracle.com
Tue Nov 22 15:49:47 UTC 2011


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







More information about the hotspot-gc-dev mailing list