United StatesChange Country, Oracle Worldwide Web Sites Communities I am a... I want to...
Bug ID: JDK-7045330 G1: Simplify/fix the HeapRegionSeq class
JDK-7045330 : G1: Simplify/fix the HeapRegionSeq class

Details
Type:
Enhancement
Submit Date:
2011-05-16
Status:
Closed
Updated Date:
2011-11-25
Project Name:
JDK
Resolved Date:
2011-09-30
Component:
hotspot
OS:
generic
Sub-Component:
gc
CPU:
generic
Priority:
P4
Resolution:
Fixed
Affected Versions:
hs21
Fixed Versions:
hs22 (b02)

Related Reports
Backport:
Backport:
Relates:
Relates:
Relates:

Sub Tasks

Description
This CR comprises a number of fixes that relate to the HeapRegionSeq class and its uses. Here's a list:

* The array that keeps track of the HeapRegions was implemented using the growable array data structure. However, a correctness issue forced us to fix the array size to the maximum allowed (the array could grow during a GC, that operation was not thread-safe, but all the GC threads were accessing the array concurrently). So, using the growable array data structure here is a bit pointless. So, we'll use a simple array instead to keep the code simpler.

* To avoid de-allocating / re-allocating the HeapRegion instances when the heap is shrunk / expanded we should keep track of three types of indexes (this is a change that fixes also fixes the memory leak in 7042285 : G1: native memory leak during humongous object allocation):

active : [0, _length) : we have HeapRegions for them that correspond to committed parts of the heap. These HeapRegions are currently in use.
inactive : [_length, _allocated_length) : we have HeapRegions for them that correspond to uncommitted parts of the heap. These HeapRegions are not currently in use, but will be if we grow the heap.
not allocated : [_allocated_length, _max_length) : we have never allocated HeapRegions for them as we have not so far committed those parts of the heap.

* For times when we know that a given address is in the heap, introduce a method that returns the corresponding region via a "biased" version of the HeapRegion array (to avoid subtracting the bottom during the calculation). This is made easier by not having the growable array data structure (as we have access to the array itself).

* Reaching the HeapRegion array from the G1CollectedHeap requires an (unnecessary) level of indirection, given that the G1CollectedHeap::_hrs field is a HeapRegionSeq*. We should eliminate the indirection and embed the HeapRegionSeq object in G1CollectedHeap by making the _hrs field of type HeapRegionSeq.

* Some code in HeapRegionSeq was written assuming that the HeapRegions added to it might not always be contiguous. This is currently not the case and the G1 heap is assumed to be contiguous, so we should simplify that code.

* In a lot of places int's are used for indexes. We should use size_t's (this is part of 6804436 : G1: heap region indices should be size_t; we'll do the HeapRegionSeq part of 6804436 in this CR and do the rest separately).

* There were several methods / fields that are not used. Let's remove them. Here's a list:

int HeapRegionSeq::_next_rr_candidate
int HeapRegionSeq::find(HeapRegion* hr)
void HeapRegionSeq::iterate_from(int idx, HeapRegionClosure* blk);
void HeapRegionSeq::print()
void HeapRegionSeq::print_empty_runs()

int G1CollectedHeap::addr_to_arena_id(void* addr)
void G1CollectedHeap::heap_region_iterate_from(int idx, HeapRegionClosure* blk);

* We should rename a couple of fields to give them more sensible names:

_alloc_search_start -> _next_search_index
_seq_bottom -> _bottom

* The code for HeapRegionSeq::iterate_from(HeapRegion* r, HeapRegionClosure* blk) is wrong. It might have been OK before parts of it were commented out, but now it seems to always start the iteration from index 0 which is not what it's supposed to do. This should be fixed to do the right thing.

* Add a verify_optional() method to check the assumptions of the HeapRegionSeq class.

* Even though we have wrappers in G1CollectedHeap for some of the HeapRegionSeq methods, we are actually not consistent in calling one or the other. We should ensure that we always call the wrappers. Here are some examples

heap_region_iterate() instead of _hrs.iterate()
n_regions() instead of _hrs.length()
region_at() instead of _hrs.at()

* Unify the code that expands the sequence by either re-using an existing HeapRegion or needing to create a new one into a single method on HeapRegionSeq class, instead of spreading it across G1CollectedHeap and HeapRegionSeq. Also make it robust wrt to a HeapRegion allocation failing.

                                    

Comments
EVALUATION

See Description.
                                     
2011-05-16
EVALUATION

http://hg.openjdk.java.net/hsx/hotspot-gc/hotspot/rev/c3f1170908be
                                     
2011-06-10
EVALUATION

http://hg.openjdk.java.net/hsx/hotspot-comp/hotspot/rev/c3f1170908be
                                     
2011-07-08
EVALUATION

http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/c3f1170908be
                                     
2011-07-08
EVALUATION

See main CR
                                     
2011-09-12



Hardware and Software, Engineered to Work Together