8205426: Humongous continues remembered set does not match humongous start region one after Remark
authortschatzl
Mon, 09 Jul 2018 10:19:51 +0200
changeset 51007 fc9dd181d70e
parent 51006 3982b9671e71
child 51008 8df91a1b549b
8205426: Humongous continues remembered set does not match humongous start region one after Remark Summary: Remembered set states for humongous objects crossing an internal per-thread processing threshold could synchronized if the humongous continues regions were processed first. Reviewed-by: ehelin, kbarrett
src/hotspot/share/gc/g1/g1ConcurrentMark.cpp
src/hotspot/share/gc/g1/g1RemSetTrackingPolicy.cpp
src/hotspot/share/gc/g1/g1RemSetTrackingPolicy.hpp
test/hotspot/jtreg/gc/g1/TestHumongousRemsetsMatch.java
--- a/src/hotspot/share/gc/g1/g1ConcurrentMark.cpp	Mon Jul 09 12:20:56 2018 +0800
+++ b/src/hotspot/share/gc/g1/g1ConcurrentMark.cpp	Mon Jul 09 10:19:51 2018 +0200
@@ -1024,11 +1024,17 @@
 
     uint _num_regions_selected_for_rebuild;  // The number of regions actually selected for rebuild.
 
-    void update_remset_before_rebuild(HeapRegion * hr) {
+    void update_remset_before_rebuild(HeapRegion* hr) {
       G1RemSetTrackingPolicy* tracking_policy = _g1h->g1_policy()->remset_tracker();
 
-      size_t const live_bytes = _cm->liveness(hr->hrm_index()) * HeapWordSize;
-      bool selected_for_rebuild = tracking_policy->update_before_rebuild(hr, live_bytes);
+      bool selected_for_rebuild;
+      if (hr->is_humongous()) {
+        bool const is_live = _cm->liveness(hr->humongous_start_region()->hrm_index()) > 0;
+        selected_for_rebuild = tracking_policy->update_humongous_before_rebuild(hr, is_live);
+      } else {
+        size_t const live_bytes = _cm->liveness(hr->hrm_index());
+        selected_for_rebuild = tracking_policy->update_before_rebuild(hr, live_bytes);
+      }
       if (selected_for_rebuild) {
         _num_regions_selected_for_rebuild++;
       }
--- a/src/hotspot/share/gc/g1/g1RemSetTrackingPolicy.cpp	Mon Jul 09 12:20:56 2018 +0800
+++ b/src/hotspot/share/gc/g1/g1RemSetTrackingPolicy.cpp	Mon Jul 09 10:19:51 2018 +0200
@@ -29,10 +29,6 @@
 #include "gc/g1/heapRegionRemSet.hpp"
 #include "runtime/safepoint.hpp"
 
-bool G1RemSetTrackingPolicy::is_interesting_humongous_region(HeapRegion* r) const {
-  return r->is_humongous() && oop(r->humongous_start_region()->bottom())->is_typeArray();
-}
-
 bool G1RemSetTrackingPolicy::needs_scan_for_rebuild(HeapRegion* r) const {
   // All non-free, non-young, non-closed archive regions need to be scanned for references;
   // At every gc we gather references to other regions in young, and closed archive
@@ -64,51 +60,81 @@
   /* nothing to do */
 }
 
+static void print_before_rebuild(HeapRegion* r, bool selected_for_rebuild, size_t total_live_bytes, size_t live_bytes) {
+  log_trace(gc, remset, tracking)("Before rebuild region %u "
+                                  "(ntams: " PTR_FORMAT ") "
+                                  "total_live_bytes " SIZE_FORMAT " "
+                                  "selected %s "
+                                  "(live_bytes " SIZE_FORMAT " "
+                                  "next_marked " SIZE_FORMAT " "
+                                  "marked " SIZE_FORMAT " "
+                                  "type %s)",
+                                  r->hrm_index(),
+                                  p2i(r->next_top_at_mark_start()),
+                                  total_live_bytes,
+                                  BOOL_TO_STR(selected_for_rebuild),
+                                  live_bytes,
+                                  r->next_marked_bytes(),
+                                  r->marked_bytes(),
+                                  r->get_type_str());
+}
+
+bool G1RemSetTrackingPolicy::update_humongous_before_rebuild(HeapRegion* r, bool is_live) {
+  assert(SafepointSynchronize::is_at_safepoint(), "should be at safepoint");
+  assert(r->is_humongous(), "Region %u should be humongous", r->hrm_index());
+
+  if (r->is_archive()) {
+    return false;
+  }
+
+  assert(!r->rem_set()->is_updating(), "Remembered set of region %u is updating before rebuild", r->hrm_index());
+
+  bool selected_for_rebuild = false;
+  // For humongous regions, to be of interest for rebuilding the remembered set the following must apply:
+  // - We always try to update the remembered sets of humongous regions containing
+  // type arrays as they might have been reset after full gc.
+  if (is_live && oop(r->humongous_start_region()->bottom())->is_typeArray() && !r->rem_set()->is_tracked()) {
+    r->rem_set()->set_state_updating();
+    selected_for_rebuild = true;
+  }
+
+  size_t const live_bytes = is_live ? HeapRegion::GrainBytes : 0;
+  print_before_rebuild(r, selected_for_rebuild, live_bytes, live_bytes);
+
+  return selected_for_rebuild;
+}
+
 bool G1RemSetTrackingPolicy::update_before_rebuild(HeapRegion* r, size_t live_bytes) {
   assert(SafepointSynchronize::is_at_safepoint(), "should be at safepoint");
-
-  bool selected_for_rebuild = false;
+  assert(!r->is_humongous(), "Region %u is humongous", r->hrm_index());
 
   // Only consider updating the remembered set for old gen regions - excluding archive regions
   // which never move (but are "Old" regions).
-  if (r->is_old_or_humongous() && !r->is_archive()) {
-    size_t between_ntams_and_top = (r->top() - r->next_top_at_mark_start()) * HeapWordSize;
-    size_t total_live_bytes = live_bytes + between_ntams_and_top;
-    // Completely free regions after rebuild are of no interest wrt rebuilding the
-    // remembered set.
-    assert(!r->rem_set()->is_updating(), "Remembered set of region %u is updating before rebuild", r->hrm_index());
-    // To be of interest for rebuilding the remembered set the following must apply:
-    // - They must contain some live data in them.
-    // - We always try to update the remembered sets of humongous regions containing
-    // type arrays if they are empty as they might have been reset after full gc.
-    // - Only need to rebuild non-complete remembered sets.
-    // - Otherwise only add those old gen regions which occupancy is low enough that there
-    // is a chance that we will ever evacuate them in the mixed gcs.
-    if ((total_live_bytes > 0) &&
-        (is_interesting_humongous_region(r) || CollectionSetChooser::region_occupancy_low_enough_for_evac(total_live_bytes)) &&
-        !r->rem_set()->is_tracked()) {
+  if (!r->is_old() || r->is_archive()) {
+    return false;
+  }
+
+  assert(!r->rem_set()->is_updating(), "Remembered set of region %u is updating before rebuild", r->hrm_index());
+
+  size_t between_ntams_and_top = (r->top() - r->next_top_at_mark_start()) * HeapWordSize;
+  size_t total_live_bytes = live_bytes + between_ntams_and_top;
 
-      r->rem_set()->set_state_updating();
-      selected_for_rebuild = true;
-    }
-    log_trace(gc, remset, tracking)("Before rebuild region %u "
-                                    "(ntams: " PTR_FORMAT ") "
-                                    "total_live_bytes " SIZE_FORMAT " "
-                                    "selected %s "
-                                    "(live_bytes " SIZE_FORMAT " "
-                                    "next_marked " SIZE_FORMAT " "
-                                    "marked " SIZE_FORMAT " "
-                                    "type %s)",
-                                    r->hrm_index(),
-                                    p2i(r->next_top_at_mark_start()),
-                                    total_live_bytes,
-                                    BOOL_TO_STR(selected_for_rebuild),
-                                    live_bytes,
-                                    r->next_marked_bytes(),
-                                    r->marked_bytes(),
-                                    r->get_type_str());
+  bool selected_for_rebuild = false;
+  // For old regions, to be of interest for rebuilding the remembered set the following must apply:
+  // - They must contain some live data in them.
+  // - Only need to rebuild non-complete remembered sets.
+  // - Otherwise only add those old gen regions which occupancy is low enough that there
+  // is a chance that we will ever evacuate them in the mixed gcs.
+  if ((total_live_bytes > 0) &&
+      CollectionSetChooser::region_occupancy_low_enough_for_evac(total_live_bytes) &&
+      !r->rem_set()->is_tracked()) {
+
+    r->rem_set()->set_state_updating();
+    selected_for_rebuild = true;
   }
 
+  print_before_rebuild(r, selected_for_rebuild, total_live_bytes, live_bytes);
+
   return selected_for_rebuild;
 }
 
@@ -149,4 +175,3 @@
                                     r->rem_set()->mem_size());
   }
 }
-
--- a/src/hotspot/share/gc/g1/g1RemSetTrackingPolicy.hpp	Mon Jul 09 12:20:56 2018 +0800
+++ b/src/hotspot/share/gc/g1/g1RemSetTrackingPolicy.hpp	Mon Jul 09 10:19:51 2018 +0200
@@ -33,10 +33,6 @@
 // the remembered set, ie. when it should be tracked, and if/when the remembered
 // set is complete.
 class G1RemSetTrackingPolicy : public CHeapObj<mtGC> {
-private:
-  // Is the given region an interesting humongous region to start remembered set tracking
-  // for?
-  bool is_interesting_humongous_region(HeapRegion* r) const;
 public:
   // Do we need to scan the given region to get all outgoing references for remembered
   // set rebuild?
@@ -45,6 +41,9 @@
   // called at any time. The caller makes sure that the changes to the remembered
   // set state are visible to other threads.
   void update_at_allocate(HeapRegion* r);
+  // Update remembered set tracking state for humongous regions before we are going to
+  // rebuild remembered sets. Called at safepoint in the remark pause.
+  bool update_humongous_before_rebuild(HeapRegion* r, bool is_live);
   // Update remembered set tracking state before we are going to rebuild remembered
   // sets. Called at safepoint in the remark pause.
   bool update_before_rebuild(HeapRegion* r, size_t live_bytes);
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/test/hotspot/jtreg/gc/g1/TestHumongousRemsetsMatch.java	Mon Jul 09 10:19:51 2018 +0200
@@ -0,0 +1,79 @@
+/*
+ * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test TestHumongousRemSetsMatch
+ * @bug 8205426
+ * @summary Test to make sure that humongous object remset states are in sync
+ * @key gc
+ * @requires vm.gc.G1
+ * @library /test/lib
+ * @build sun.hotspot.WhiteBox
+ * @run driver ClassFileInstaller sun.hotspot.WhiteBox
+ * @run main/othervm -Xbootclasspath/a:. -XX:+UnlockDiagnosticVMOptions -Xmx512M -Xms512M -Xmn10M -XX:ParallelGCThreads=2 -XX:-UseDynamicNumberOfGCThreads -XX:+UseG1GC -XX:+WhiteBoxAPI -XX:G1HeapRegionSize=1M -XX:+VerifyAfterGC -Xlog:gc,gc+remset+tracking=trace TestHumongousRemsetsMatch
+ */
+
+import sun.hotspot.WhiteBox;
+
+public class TestHumongousRemsetsMatch {
+
+    // G1 at the moment uses one thread every this amount of regions.
+    private static final int WorkerThreadBoundary = 384;
+
+    private static final int ObjSizeInRegions = 17;
+    private static final int M = 1024 * 1024;
+    private static final int TypeArrayObjSize = ObjSizeInRegions * M / 4 /* sizeof(int) */ - 1024 /* > header size */;
+
+    public static void main(String[] args) throws Exception {
+        WhiteBox wb = WhiteBox.getWhiteBox();
+
+        for (int j = 0; j < 3; j++) {
+            wb.fullGC(); // Start with a clean slate
+
+            // It may happen that our 7-region sized humongous objects may just be "misaligned"
+            // so that they do not cross the region 384 boundary. Try to counter this by offsetting
+            // the humongous objects just a little.
+            Object alignmentFudge = new int[(j + 1) * M / 4 /* sizeof(int) */ - 1024];
+
+            // Fill the heap so that more than WorkerThreadBoundary regions are occupied with humongous objects
+            // and hopefully one of these objects crosses the region WorkerThreadBoundary boundary.
+            Object[] lotsOfHumongousObjects = new Object[(WorkerThreadBoundary / ObjSizeInRegions) + 3];
+
+            for (int i = 0; i < lotsOfHumongousObjects.length; i++) {
+                lotsOfHumongousObjects[i] = new int[TypeArrayObjSize];
+            }
+
+            wb.fullGC();
+
+            // Trigger a concurrent cycle and wait until the Remark pause
+            wb.g1StartConcMarkCycle();
+            while (wb.g1InConcurrentMark()) {
+                Thread.sleep(200);
+            }
+            wb.youngGC(); // Trigger verification error.
+
+            System.out.println(lotsOfHumongousObjects + " " + alignmentFudge);
+        }
+    }
+}
+