Thu, 06 Oct 2011 18:56:47 -0700
7095236: G1: _markedRegions never contains NULL regions
Summary: Removed the code for skipping over NULL regions in _markedRegions, replacing it with an assertion that a NULL region is never encountered; removed dead methods, remove() and remove_region(), and inlined a simplified addRegion() directly into fillCache().
Reviewed-by: brutisso, tonyp
1.1 --- a/src/share/vm/gc_implementation/g1/collectionSetChooser.cpp Thu Oct 06 13:28:09 2011 -0400 1.2 +++ b/src/share/vm/gc_implementation/g1/collectionSetChooser.cpp Thu Oct 06 18:56:47 2011 -0700 1.3 @@ -118,30 +118,6 @@ 1.4 } 1.5 } 1.6 1.7 -// this is a bit expensive... but we expect that it should not be called 1.8 -// to often. 1.9 -void CSetChooserCache::remove(HeapRegion *hr) { 1.10 - assert(_occupancy > 0, "cache should not be empty"); 1.11 - assert(hr->sort_index() < -1, "should already be in the cache"); 1.12 - int index = get_index(hr->sort_index()); 1.13 - assert(_cache[index] == hr, "index should be correct"); 1.14 - int next_index = trim_index(index + 1); 1.15 - int last_index = trim_index(_first + _occupancy - 1); 1.16 - while (index != last_index) { 1.17 - assert(_cache[next_index] != NULL, "should not be null"); 1.18 - _cache[index] = _cache[next_index]; 1.19 - _cache[index]->set_sort_index(get_sort_index(index)); 1.20 - 1.21 - index = next_index; 1.22 - next_index = trim_index(next_index+1); 1.23 - } 1.24 - assert(index == last_index, "should have reached the last one"); 1.25 - _cache[index] = NULL; 1.26 - hr->set_sort_index(-1); 1.27 - --_occupancy; 1.28 - assert(verify(), "cache should be consistent"); 1.29 -} 1.30 - 1.31 static inline int orderRegions(HeapRegion* hr1, HeapRegion* hr2) { 1.32 if (hr1 == NULL) { 1.33 if (hr2 == NULL) return 0; 1.34 @@ -197,43 +173,34 @@ 1.35 HeapRegion *prev = NULL; 1.36 while (index < _numMarkedRegions) { 1.37 HeapRegion *curr = _markedRegions.at(index++); 1.38 - if (curr != NULL) { 1.39 - int si = curr->sort_index(); 1.40 - guarantee(!curr->is_young(), "should not be young!"); 1.41 - guarantee(si > -1 && si == (index-1), "sort index invariant"); 1.42 - if (prev != NULL) { 1.43 - guarantee(orderRegions(prev, curr) != 1, "regions should be sorted"); 1.44 - } 1.45 - prev = curr; 1.46 + guarantee(curr != NULL, "Regions in _markedRegions array cannot be NULL"); 1.47 + int si = curr->sort_index(); 1.48 + guarantee(!curr->is_young(), "should not be young!"); 1.49 + guarantee(si > -1 && si == (index-1), "sort index invariant"); 1.50 + if (prev != NULL) { 1.51 + guarantee(orderRegions(prev, curr) != 1, "regions should be sorted"); 1.52 } 1.53 + prev = curr; 1.54 } 1.55 return _cache.verify(); 1.56 } 1.57 #endif 1.58 1.59 -bool 1.60 -CollectionSetChooser::addRegionToCache() { 1.61 - assert(!_cache.is_full(), "cache should not be full"); 1.62 - 1.63 - HeapRegion *hr = NULL; 1.64 - while (hr == NULL && _curMarkedIndex < _numMarkedRegions) { 1.65 - hr = _markedRegions.at(_curMarkedIndex++); 1.66 - } 1.67 - if (hr == NULL) 1.68 - return false; 1.69 - assert(!hr->is_young(), "should not be young!"); 1.70 - assert(hr->sort_index() == _curMarkedIndex-1, "sort_index invariant"); 1.71 - _markedRegions.at_put(hr->sort_index(), NULL); 1.72 - _cache.insert(hr); 1.73 - assert(!_cache.is_empty(), "cache should not be empty"); 1.74 - assert(verify(), "cache should be consistent"); 1.75 - return false; 1.76 -} 1.77 - 1.78 void 1.79 CollectionSetChooser::fillCache() { 1.80 - while (!_cache.is_full() && addRegionToCache()) { 1.81 + while (!_cache.is_full() && (_curMarkedIndex < _numMarkedRegions)) { 1.82 + HeapRegion* hr = _markedRegions.at(_curMarkedIndex); 1.83 + assert(hr != NULL, 1.84 + err_msg("Unexpected NULL hr in _markedRegions at index %d", 1.85 + _curMarkedIndex)); 1.86 + _curMarkedIndex += 1; 1.87 + assert(!hr->is_young(), "should not be young!"); 1.88 + assert(hr->sort_index() == _curMarkedIndex-1, "sort_index invariant"); 1.89 + _markedRegions.at_put(hr->sort_index(), NULL); 1.90 + _cache.insert(hr); 1.91 + assert(!_cache.is_empty(), "cache should not be empty"); 1.92 } 1.93 + assert(verify(), "cache should be consistent"); 1.94 } 1.95 1.96 void 1.97 @@ -334,20 +301,6 @@ 1.98 clearMarkedHeapRegions(); 1.99 } 1.100 1.101 -void CollectionSetChooser::removeRegion(HeapRegion *hr) { 1.102 - int si = hr->sort_index(); 1.103 - assert(si == -1 || hr->is_marked(), "Sort index not valid."); 1.104 - if (si > -1) { 1.105 - assert(_markedRegions.at(si) == hr, "Sort index not valid." ); 1.106 - _markedRegions.at_put(si, NULL); 1.107 - } else if (si < -1) { 1.108 - assert(_cache.region_in_cache(hr), "should be in the cache"); 1.109 - _cache.remove(hr); 1.110 - assert(hr->sort_index() == -1, "sort index invariant"); 1.111 - } 1.112 - hr->set_sort_index(-1); 1.113 -} 1.114 - 1.115 // if time_remaining < 0.0, then this method should try to return 1.116 // a region, whether it fits within the remaining time or not 1.117 HeapRegion*
2.1 --- a/src/share/vm/gc_implementation/g1/collectionSetChooser.hpp Thu Oct 06 13:28:09 2011 -0400 2.2 +++ b/src/share/vm/gc_implementation/g1/collectionSetChooser.hpp Thu Oct 06 18:56:47 2011 -0700 2.3 @@ -1,5 +1,5 @@ 2.4 /* 2.5 - * Copyright (c) 2001, 2010, Oracle and/or its affiliates. All rights reserved. 2.6 + * Copyright (c) 2001, 2011, Oracle and/or its affiliates. All rights reserved. 2.7 * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. 2.8 * 2.9 * This code is free software; you can redistribute it and/or modify it 2.10 @@ -29,6 +29,26 @@ 2.11 #include "utilities/growableArray.hpp" 2.12 2.13 // We need to sort heap regions by collection desirability. 2.14 +// This sorting is currently done in two "stages". An initial sort is 2.15 +// done following a cleanup pause as soon as all of the marked but 2.16 +// non-empty regions have been identified and the completely empty 2.17 +// ones reclaimed. 2.18 +// This gives us a global sort on a GC efficiency metric 2.19 +// based on predictive data available at that time. However, 2.20 +// any of these regions that are collected will only be collected 2.21 +// during a future GC pause, by which time it is possible that newer 2.22 +// data might allow us to revise and/or refine the earlier 2.23 +// pause predictions, leading to changes in expected gc efficiency 2.24 +// order. To somewhat mitigate this obsolescence, more so in the 2.25 +// case of regions towards the end of the list, which will be 2.26 +// picked later, these pre-sorted regions from the _markedRegions 2.27 +// array are not used as is, but a small prefix thereof is 2.28 +// insertion-sorted again into a small cache, based on more 2.29 +// recent remembered set information. Regions are then drawn 2.30 +// from this cache to construct the collection set at each 2.31 +// incremental GC. 2.32 +// This scheme and/or its implementation may be subject to 2.33 +// revision in the future. 2.34 2.35 class CSetChooserCache VALUE_OBJ_CLASS_SPEC { 2.36 private: 2.37 @@ -37,8 +57,8 @@ 2.38 } PrivateConstants; 2.39 2.40 HeapRegion* _cache[CacheLength]; 2.41 - int _occupancy; // number of region in cache 2.42 - int _first; // "first" region in the cache 2.43 + int _occupancy; // number of regions in cache 2.44 + int _first; // (index of) "first" region in the cache 2.45 2.46 // adding CacheLength to deal with negative values 2.47 inline int trim_index(int index) { 2.48 @@ -62,7 +82,6 @@ 2.49 void clear(void); 2.50 void insert(HeapRegion *hr); 2.51 HeapRegion *remove_first(void); 2.52 - void remove (HeapRegion *hr); 2.53 inline HeapRegion *get_first(void) { 2.54 return _cache[_first]; 2.55 } 2.56 @@ -102,7 +121,6 @@ 2.57 2.58 void sortMarkedHeapRegions(); 2.59 void fillCache(); 2.60 - bool addRegionToCache(void); 2.61 void addMarkedHeapRegion(HeapRegion *hr); 2.62 2.63 // Must be called before calls to getParMarkedHeapRegionChunk. 2.64 @@ -122,9 +140,6 @@ 2.65 2.66 void updateAfterFullCollection(); 2.67 2.68 - // Ensure that "hr" is not a member of the marked region array or the cache 2.69 - void removeRegion(HeapRegion* hr); 2.70 - 2.71 bool unmarked_age_1_returned_as_new() { return _unmarked_age_1_returned_as_new; } 2.72 2.73 // Returns true if the used portion of "_markedRegions" is properly