changeset 50441:523c2a73a3dc

8204081: Mismatch in rebuild policy and collection set chooser causes remembered sets to be kept errorneously Summary: Due to mismatch in which region's remembered sets should be rebuilt and the ones that are looked at in the collection set chooser superfluous remembered sets might be built and kept alive until the next marking. Reviewed-by: sjohanss, kbarrett
author tschatzl
date Thu, 07 Jun 2018 11:20:30 +0200
parents cfdd37095f66
children ab967988f850
files src/hotspot/share/gc/g1/collectionSetChooser.cpp src/hotspot/share/gc/g1/g1Policy.cpp src/hotspot/share/gc/g1/heapRegion.hpp
diffstat 3 files changed, 22 insertions(+), 26 deletions(-) [+]
line wrap: on
line diff
--- a/src/hotspot/share/gc/g1/collectionSetChooser.cpp	Thu Jun 07 11:20:18 2018 +0200
+++ b/src/hotspot/share/gc/g1/collectionSetChooser.cpp	Thu Jun 07 11:20:30 2018 +0200
@@ -147,7 +147,7 @@
 void CollectionSetChooser::add_region(HeapRegion* hr) {
   assert(!hr->is_pinned(),
          "Pinned region shouldn't be added to the collection set (index %u)", hr->hrm_index());
-  assert(!hr->is_young(), "should not be young!");
+  assert(hr->is_old(), "should be old but is %s", hr->get_type_str());
   assert(hr->rem_set()->is_complete(),
          "Trying to add region %u to the collection set with incomplete remembered set", hr->hrm_index());
   _regions.append(hr);
@@ -185,7 +185,7 @@
 
 void CollectionSetChooser::set_region(uint index, HeapRegion* hr) {
   assert(regions_at(index) == NULL, "precondition");
-  assert(!hr->is_young(), "should not be young!");
+  assert(hr->is_old(), "should be old but is %s", hr->get_type_str());
   regions_at_put(index, hr);
   hr->calc_gc_efficiency();
 }
@@ -233,18 +233,19 @@
     _cset_updater(hrSorted, true /* parallel */, chunk_size) { }
 
   bool do_heap_region(HeapRegion* r) {
-    // Do we have any marking information for this region?
-    if (r->is_marked()) {
-      // We will skip any region that's currently used as an old GC
-      // alloc region (we should not consider those for collection
-      // before we fill them up).
-      if (_cset_updater.should_add(r) && !_g1h->is_old_gc_alloc_region(r)) {
-        _cset_updater.add_region(r);
-      } else if (r->is_old()) {
-        // Can clean out the remembered sets of all regions that we did not choose but
-        // we created the remembered set for.
-        r->rem_set()->clear(true);
-      }
+    // We will skip any region that's currently used as an old GC
+    // alloc region (we should not consider those for collection
+    // before we fill them up).
+    if (_cset_updater.should_add(r) && !_g1h->is_old_gc_alloc_region(r)) {
+      _cset_updater.add_region(r);
+    } else if (r->is_old()) {
+      // Keep remembered sets for humongous regions, otherwise clean out remembered
+      // sets for old regions.
+      r->rem_set()->clear(true /* only_cardset */);
+    } else {
+      assert(!r->is_old() || !r->rem_set()->is_tracked(),
+             "Missed to clear unused remembered set of region %u (%s) that is %s",
+             r->hrm_index(), r->get_type_str(), r->rem_set()->get_state_str());
     }
     return false;
   }
@@ -280,11 +281,10 @@
 }
 
 bool CollectionSetChooser::should_add(HeapRegion* hr) const {
-  assert(hr->is_marked(), "pre-condition");
-  assert(!hr->is_young(), "should never consider young regions");
-  return !hr->is_pinned() &&
-          region_occupancy_low_enough_for_evac(hr->live_bytes()) &&
-          hr->rem_set()->is_complete();
+  return !hr->is_young() &&
+         !hr->is_pinned() &&
+         region_occupancy_low_enough_for_evac(hr->live_bytes()) &&
+         hr->rem_set()->is_complete();
 }
 
 void CollectionSetChooser::rebuild(WorkGang* workers, uint n_regions) {
--- a/src/hotspot/share/gc/g1/g1Policy.cpp	Thu Jun 07 11:20:18 2018 +0200
+++ b/src/hotspot/share/gc/g1/g1Policy.cpp	Thu Jun 07 11:20:30 2018 +0200
@@ -825,10 +825,10 @@
 
 size_t G1Policy::predict_bytes_to_copy(HeapRegion* hr) const {
   size_t bytes_to_copy;
-  if (hr->is_marked())
+  if (!hr->is_young()) {
     bytes_to_copy = hr->max_live_bytes();
-  else {
-    assert(hr->is_young() && hr->age_in_surv_rate_group() != -1, "invariant");
+  } else {
+    assert(hr->age_in_surv_rate_group() != -1, "invariant");
     int age = hr->age_in_surv_rate_group();
     double yg_surv_rate = predict_yg_surv_rate(age, hr->surv_rate_group());
     bytes_to_copy = (size_t) (hr->used() * yg_surv_rate);
--- a/src/hotspot/share/gc/g1/heapRegion.hpp	Thu Jun 07 11:20:18 2018 +0200
+++ b/src/hotspot/share/gc/g1/heapRegion.hpp	Thu Jun 07 11:20:30 2018 +0200
@@ -541,10 +541,6 @@
   // objects during evac failure handling.
   void note_self_forwarding_removal_end(size_t marked_bytes);
 
-  // Returns "false" iff no object in the region was allocated when the
-  // last mark phase ended.
-  bool is_marked() { return _prev_top_at_mark_start != bottom(); }
-
   void reset_during_compaction() {
     assert(is_humongous(),
            "should only be called for humongous regions");