CRR (M): 7114678: G1: various small fixes, code cleanup, and refactoring
tony.printezis at oracle.com
Tue Nov 22 07:49:47 PST 2011
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
The webrev is here:
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:
And here's the rest:
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.
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
* Introduced G1CollectedHeap::start_region_for_worker() which calculates
which region each parallel worker should start from during parallel
* 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
* 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
* 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