EVALUATION

See main CR
                                     

EVALUATION

http://hg.openjdk.java.net/hsx/hotspot-gc/hotspot/rev/b9390528617c
                                     

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
                                     

EVALUATION

Yes.