United StatesChange Country, Oracle Worldwide Web Sites Communities I am a... I want to...
Bug ID: JDK-7095236 G1: _markedRegions never contains NULL regions
JDK-7095236 : G1: _markedRegions never contains NULL regions

Details
Type:
Enhancement
Submit Date:
2011-09-26
Status:
Closed
Updated Date:
2012-01-23
Project Name:
JDK
Resolved Date:
2012-01-23
Component:
hotspot
OS:
generic
Sub-Component:
gc
CPU:
generic
Priority:
P5
Resolution:
Fixed
Affected Versions:
hs23
Fixed Versions:
hs23 (b02)

Related Reports
Backport:
Backport:

Sub Tasks

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
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
                                     
2011-09-26
EVALUATION

Yes.
                                     
2011-09-26
EVALUATION

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

See main CR
                                     
2011-10-22



Hardware and Software, Engineered to Work Together