JDK-7045330 : G1: Simplify/fix the HeapRegionSeq class
  • Type: Enhancement
  • Component: hotspot
  • Sub-Component: gc
  • Affected Version: hs21
  • Priority: P4
  • Status: Closed
  • Resolution: Fixed
  • OS: generic
  • CPU: generic
  • Submitted: 2011-05-16
  • Updated: 2013-09-18
  • Resolved: 2011-09-30
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 7 JDK 8 Other
7u2Fixed 8Fixed hs22Fixed
Related Reports
Relates :  
Relates :  
Relates :  
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 main CR
12-09-2011

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

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

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

EVALUATION See Description.
16-05-2011