8370682: G1: Survivor regions not in young gen cset group

Reviewed-by: iwalulya, ayang

Author: Thomas Schatzl

src/hotspot/share/gc/g1/g1CollectedHeap.cpp

@@ -2563,10 +2563,12 @@ void G1CollectedHeap::verify_region_attr_is_remset_tracked() {
   public:
     virtual bool do_heap_region(G1HeapRegion* r) {
       G1CollectedHeap* g1h = G1CollectedHeap::heap();
-      const bool is_remset_tracked = g1h->region_attr(r->bottom()).is_remset_tracked();
-      assert(r->rem_set()->is_tracked() == is_remset_tracked,
-             "Region %u remset tracking status (%s) different to region attribute (%s)",
-             r->hrm_index(), BOOL_TO_STR(r->rem_set()->is_tracked()), BOOL_TO_STR(is_remset_tracked));
+      G1HeapRegionAttr attr = g1h->region_attr(r->bottom());
+      bool const is_remset_tracked = attr.is_remset_tracked();
+      assert((r->rem_set()->is_tracked() == is_remset_tracked) ||
+             (attr.is_new_survivor() && is_remset_tracked),
+             "Region %u (%s) remset tracking status (%s) different to region attribute (%s)",
+             r->hrm_index(), r->get_type_str(), BOOL_TO_STR(r->rem_set()->is_tracked()), BOOL_TO_STR(is_remset_tracked));
       return false;
     }
   } cl;
@@ -3105,9 +3107,8 @@ G1HeapRegion* G1CollectedHeap::new_mutator_alloc_region(size_t word_size,
       new_alloc_region->set_eden();
       _eden.add(new_alloc_region);
       _policy->set_region_eden(new_alloc_region);
-      _policy->remset_tracker()->update_at_allocate(new_alloc_region);
-      // Install the group cardset.
-      young_regions_cset_group()->add(new_alloc_region);
+
+      collection_set()->add_eden_region(new_alloc_region);
       G1HeapRegionPrinter::alloc(new_alloc_region);
       return new_alloc_region;
     }
@@ -3120,7 +3121,6 @@ void G1CollectedHeap::retire_mutator_alloc_region(G1HeapRegion* alloc_region,
   assert_heap_locked_or_at_safepoint(true /* should_be_vm_thread */);
   assert(alloc_region->is_eden(), "all mutator alloc regions should be eden");
 
-  collection_set()->add_eden_region(alloc_region);
   increase_used(allocated_bytes);
   _eden.add_used_bytes(allocated_bytes);
   G1HeapRegionPrinter::retire(alloc_region);
@@ -3164,14 +3164,20 @@ G1HeapRegion* G1CollectedHeap::new_gc_alloc_region(size_t word_size, G1HeapRegio
     if (type.is_survivor()) {
       new_alloc_region->set_survivor();
       _survivor.add(new_alloc_region);
+      // The remembered set/group cardset for this region will be installed at the
+      // end of GC. Cannot do that right now because we still need the current young
+      // gen cardset group.
+      // However, register with the attribute table to collect remembered set entries
+      // immediately as it is the only source for determining the need for remembered
+      // set tracking during GC.
       register_new_survivor_region_with_region_attr(new_alloc_region);
-      // Install the group cardset.
-      young_regions_cset_group()->add(new_alloc_region);
     } else {
       new_alloc_region->set_old();
+      // Update remembered set/cardset.
+      _policy->remset_tracker()->update_at_allocate(new_alloc_region);
+      // Synchronize with region attribute table.
       update_region_attr(new_alloc_region);
     }
-    _policy->remset_tracker()->update_at_allocate(new_alloc_region);
     G1HeapRegionPrinter::alloc(new_alloc_region);
     return new_alloc_region;
   }

src/hotspot/share/gc/g1/g1CollectionSet.cpp

@@ -163,6 +163,7 @@ void G1CollectionSet::clear() {
   assert_at_safepoint_on_vm_thread();
   _regions_cur_length = 0;
   _groups.clear();
+  assert(_optional_groups.length() == 0, "must be");
 }
 
 void G1CollectionSet::iterate(G1HeapRegionClosure* cl) const {
@@ -216,7 +217,11 @@ void G1CollectionSet::add_young_region_common(G1HeapRegion* hr) {
   assert(hr->is_young(), "invariant");
   assert(_inc_build_state == CSetBuildType::Active, "Precondition");
 
-  assert(!hr->in_collection_set(), "invariant");
+  // Add to remembered set/cardset group.
+  _g1h->policy()->remset_tracker()->update_at_allocate(hr);
+  _g1h->young_regions_cset_group()->add(hr);
+
+  // Synchronize with the region attribute table.
   _g1h->register_young_region_with_region_attr(hr);
 
   // We use UINT_MAX as "invalid" marker in verification.
@@ -233,11 +238,13 @@ void G1CollectionSet::add_young_region_common(G1HeapRegion* hr) {
 }
 
 void G1CollectionSet::add_survivor_regions(G1HeapRegion* hr) {
+  assert_at_safepoint_on_vm_thread();
   assert(hr->is_survivor(), "Must only add survivor regions, but is %s", hr->get_type_str());
   add_young_region_common(hr);
 }
 
 void G1CollectionSet::add_eden_region(G1HeapRegion* hr) {
+  assert_heap_locked_or_at_safepoint(true /* should_be_vm_thread */);
   assert(hr->is_eden(), "Must only add eden regions, but is %s", hr->get_type_str());
   add_young_region_common(hr);
 }

src/hotspot/share/gc/g1/g1FullGCResetMetadataTask.cpp

@@ -31,16 +31,13 @@ G1FullGCResetMetadataTask::G1ResetMetadataClosure::G1ResetMetadataClosure(G1Full
   _collector(collector) { }
 
 void G1FullGCResetMetadataTask::G1ResetMetadataClosure::reset_region_metadata(G1HeapRegion* hr) {
+  assert(hr->is_humongous() || !hr->rem_set()->has_cset_group(),
+         "Non-humongous regions must not have cset group");
   hr->rem_set()->clear();
   hr->clear_both_card_tables();
 }
 
 bool G1FullGCResetMetadataTask::G1ResetMetadataClosure::do_heap_region(G1HeapRegion* hr) {
-  if (!hr->is_humongous()) {
-    hr->uninstall_cset_group();
-  }
-
-
   uint const region_idx = hr->hrm_index();
   if (!_collector->is_compaction_target(region_idx)) {
     assert(!hr->is_free(), "all free regions should be compaction targets");

src/hotspot/share/gc/g1/g1ParScanThreadState.inline.hpp

@@ -151,11 +151,13 @@ template <class T> void G1ParScanThreadState::mark_card_if_tracked(G1HeapRegionA
 
 #ifdef ASSERT
   G1HeapRegion* const hr_obj = _g1h->heap_region_containing(o);
-  assert(region_attr.is_remset_tracked() == hr_obj->rem_set()->is_tracked(),
-         "State flag indicating remset tracking disagrees (%s) with actual remembered set (%s) for region %u",
+  assert((region_attr.is_remset_tracked() == hr_obj->rem_set()->is_tracked()) ||
+         (region_attr.is_new_survivor() && region_attr.is_remset_tracked()),
+         "State flag indicating remset tracking disagrees (%s) with actual remembered set (%s) for region %u (%s)",
          BOOL_TO_STR(region_attr.is_remset_tracked()),
          BOOL_TO_STR(hr_obj->rem_set()->is_tracked()),
-         hr_obj->hrm_index());
+         hr_obj->hrm_index(),
+         hr_obj->get_type_str());
 #endif
   if (!region_attr.is_remset_tracked()) {
     return;