JDK-7095236 : G1: _markedRegions never contains NULL regions
  • Type: Enhancement
  • Component: hotspot
  • Sub-Component: gc
  • Affected Version: hs23
  • Priority: P5
  • Status: Closed
  • Resolution: Fixed
  • OS: generic
  • CPU: generic
  • Submitted: 2011-09-26
  • Updated: 2013-09-18
  • Resolved: 2012-01-23
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
7u4Fixed 8Fixed hs23Fixed
Description
We statically established the statement in the synopsis line above.
This allowed for some simplification in the cache filling code:
the code to skip over NULL regions could be dropped entirely
and replaced by an assertion that a NULL was never encountered
in the _markedRegionsArray. This allowed some further simplifications,
see Suggested Fix section. We also deleted some unused methods (dead code).

The code was tested with JPRT and refworkload.
Comments
EVALUATION See main CR
22-10-2011
EVALUATION http://hg.openjdk.java.net/hsx/hotspot-gc/hotspot/rev/b9390528617c
10-10-2011
EVALUATION Yes.
26-09-2011
SUGGESTED FIX Preliminary diffs subject to code review and more testing. Firther dead code deletion under consideration. diff --git a/src/share/vm/gc_implementation/g1/collectionSetChooser.cpp b/src/share/vm/gc_implementation/g1/collectionSetChooser.cpp --- a/src/share/vm/gc_implementation/g1/collectionSetChooser.cpp +++ b/src/share/vm/gc_implementation/g1/collectionSetChooser.cpp @@ -197,43 +197,33 @@ HeapRegion *prev = NULL; while (index < _numMarkedRegions) { HeapRegion *curr = _markedRegions.at(index++); - if (curr != NULL) { - int si = curr->sort_index(); - guarantee(!curr->is_young(), "should not be young!"); - guarantee(si > -1 && si == (index-1), "sort index invariant"); - if (prev != NULL) { - guarantee(orderRegions(prev, curr) != 1, "regions should be sorted"); - } - prev = curr; + assert(curr != NULL, "Regions in _markedRegions array cannot be NULL"); + int si = curr->sort_index(); + guarantee(!curr->is_young(), "should not be young!"); + guarantee(si > -1 && si == (index-1), "sort index invariant"); + if (prev != NULL) { + guarantee(orderRegions(prev, curr) != 1, "regions should be sorted"); } + prev = curr; } return _cache.verify(); } #endif -bool -CollectionSetChooser::addRegionToCache() { - assert(!_cache.is_full(), "cache should not be full"); - - HeapRegion *hr = NULL; - while (hr == NULL && _curMarkedIndex < _numMarkedRegions) { - hr = _markedRegions.at(_curMarkedIndex++); - } - if (hr == NULL) - return false; - assert(!hr->is_young(), "should not be young!"); - assert(hr->sort_index() == _curMarkedIndex-1, "sort_index invariant"); - _markedRegions.at_put(hr->sort_index(), NULL); - _cache.insert(hr); - assert(!_cache.is_empty(), "cache should not be empty"); - assert(verify(), "cache should be consistent"); - return false; -} - void CollectionSetChooser::fillCache() { - while (!_cache.is_full() && addRegionToCache()) { + while (!_cache.is_full() && (_curMarkedIndex < _numMarkedRegions)) { + HeapRegion* hr = _markedRegions.at(_curMarkedIndex++); + assert(hr != NULL, + err_msg("Unexpected NULL hr in _markedRegions at index %d", + _curMarkedIndex - 1)); + assert(!hr->is_young(), "should not be young!"); + assert(hr->sort_index() == _curMarkedIndex-1, "sort_index invariant"); + _markedRegions.at_put(hr->sort_index(), NULL); + _cache.insert(hr); + assert(!_cache.is_empty(), "cache should not be empty"); } + assert(verify(), "cache should be consistent"); } void @@ -334,20 +324,6 @@ clearMarkedHeapRegions(); } -void CollectionSetChooser::removeRegion(HeapRegion *hr) { - int si = hr->sort_index(); - assert(si == -1 || hr->is_marked(), "Sort index not valid."); - if (si > -1) { - assert(_markedRegions.at(si) == hr, "Sort index not valid." ); - _markedRegions.at_put(si, NULL); - } else if (si < -1) { - assert(_cache.region_in_cache(hr), "should be in the cache"); - _cache.remove(hr); - assert(hr->sort_index() == -1, "sort index invariant"); - } - hr->set_sort_index(-1); -} - // if time_remaining < 0.0, then this method should try to return // a region, whether it fits within the remaining time or not HeapRegion* diff --git a/src/share/vm/gc_implementation/g1/collectionSetChooser.hpp b/src/share/vm/gc_implementation/g1/collectionSetChooser.hpp --- a/src/share/vm/gc_implementation/g1/collectionSetChooser.hpp +++ b/src/share/vm/gc_implementation/g1/collectionSetChooser.hpp @@ -102,7 +102,6 @@ void sortMarkedHeapRegions(); void fillCache(); - bool addRegionToCache(void); void addMarkedHeapRegion(HeapRegion *hr); // Must be called before calls to getParMarkedHeapRegionChunk. @@ -122,9 +121,6 @@ void updateAfterFullCollection(); - // Ensure that "hr" is not a member of the marked region array or the cache - void removeRegion(HeapRegion* hr); - bool unmarked_age_1_returned_as_new() { return _unmarked_age_1_returned_as_new; } // Returns true if the used portion of "_markedRegions" is properly
26-09-2011