8209700: Remove HeapRegionSetBase::RegionSetKind for a more flexible approach

Reviewed-by: kbarrett, phh

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

@@ -1405,6 +1405,68 @@ void G1CollectedHeap::shrink(size_t shrink_bytes) {
   _verifier->verify_region_sets_optional();
 }
 
+class OldRegionSetChecker : public HeapRegionSetChecker {
+public:
+  void check_mt_safety() {
+    // Master Old Set MT safety protocol:
+    // (a) If we're at a safepoint, operations on the master old set
+    // should be invoked:
+    // - by the VM thread (which will serialize them), or
+    // - by the GC workers while holding the FreeList_lock, if we're
+    //   at a safepoint for an evacuation pause (this lock is taken
+    //   anyway when an GC alloc region is retired so that a new one
+    //   is allocated from the free list), or
+    // - by the GC workers while holding the OldSets_lock, if we're at a
+    //   safepoint for a cleanup pause.
+    // (b) If we're not at a safepoint, operations on the master old set
+    // should be invoked while holding the Heap_lock.
+
+    if (SafepointSynchronize::is_at_safepoint()) {
+      guarantee(Thread::current()->is_VM_thread() ||
+                FreeList_lock->owned_by_self() || OldSets_lock->owned_by_self(),
+                "master old set MT safety protocol at a safepoint");
+    } else {
+      guarantee(Heap_lock->owned_by_self(), "master old set MT safety protocol outside a safepoint");
+    }
+  }
+  bool is_correct_type(HeapRegion* hr) { return hr->is_old(); }
+  const char* get_description() { return "Old Regions"; }
+};
+
+class ArchiveRegionSetChecker : public HeapRegionSetChecker {
+public:
+  void check_mt_safety() {
+    guarantee(!Universe::is_fully_initialized() || SafepointSynchronize::is_at_safepoint(),
+              "May only change archive regions during initialization or safepoint.");
+  }
+  bool is_correct_type(HeapRegion* hr) { return hr->is_archive(); }
+  const char* get_description() { return "Archive Regions"; }
+};
+
+class HumongousRegionSetChecker : public HeapRegionSetChecker {
+public:
+  void check_mt_safety() {
+    // Humongous Set MT safety protocol:
+    // (a) If we're at a safepoint, operations on the master humongous
+    // set should be invoked by either the VM thread (which will
+    // serialize them) or by the GC workers while holding the
+    // OldSets_lock.
+    // (b) If we're not at a safepoint, operations on the master
+    // humongous set should be invoked while holding the Heap_lock.
+
+    if (SafepointSynchronize::is_at_safepoint()) {
+      guarantee(Thread::current()->is_VM_thread() ||
+                OldSets_lock->owned_by_self(),
+                "master humongous set MT safety protocol at a safepoint");
+    } else {
+      guarantee(Heap_lock->owned_by_self(),
+                "master humongous set MT safety protocol outside a safepoint");
+    }
+  }
+  bool is_correct_type(HeapRegion* hr) { return hr->is_humongous(); }
+  const char* get_description() { return "Humongous Regions"; }
+};
+
 G1CollectedHeap::G1CollectedHeap(G1CollectorPolicy* collector_policy) :
   CollectedHeap(),
   _young_gen_sampling_thread(NULL),
@@ -1417,9 +1479,9 @@ G1CollectedHeap::G1CollectedHeap(G1CollectorPolicy* collector_policy) :
   _eden_pool(NULL),
   _survivor_pool(NULL),
   _old_pool(NULL),
-  _old_set("Old Region Set", HeapRegionSetBase::ForOldRegions, new OldRegionSetMtSafeChecker()),
+  _old_set("Old Region Set", new OldRegionSetChecker()),
-  _archive_set("Archive Region Set", HeapRegionSetBase::ForArchiveRegions, new ArchiveRegionSetMtSafeChecker()),
+  _archive_set("Archive Region Set", new ArchiveRegionSetChecker()),
-  _humongous_set("Humongous Region Set", HeapRegionSetBase::ForHumongousRegions, new HumongousRegionSetMtSafeChecker()),
+  _humongous_set("Humongous Region Set", new HumongousRegionSetChecker()),
   _bot(NULL),
   _listener(),
   _hrm(),

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

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2001, 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2001, 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
@@ -30,6 +30,41 @@
 #include "gc/g1/heapRegionSet.inline.hpp"
 #include "memory/allocation.hpp"
 
+class MasterFreeRegionListChecker : public HeapRegionSetChecker {
+public:
+  void check_mt_safety() {
+    // Master Free List MT safety protocol:
+    // (a) If we're at a safepoint, operations on the master free list
+    // should be invoked by either the VM thread (which will serialize
+    // them) or by the GC workers while holding the
+    // FreeList_lock.
+    // (b) If we're not at a safepoint, operations on the master free
+    // list should be invoked while holding the Heap_lock.
+
+    if (SafepointSynchronize::is_at_safepoint()) {
+      guarantee(Thread::current()->is_VM_thread() ||
+                FreeList_lock->owned_by_self(), "master free list MT safety protocol at a safepoint");
+    } else {
+      guarantee(Heap_lock->owned_by_self(), "master free list MT safety protocol outside a safepoint");
+    }
+  }
+  bool is_correct_type(HeapRegion* hr) { return hr->is_free(); }
+  const char* get_description() { return "Free Regions"; }
+};
+
+HeapRegionManager::HeapRegionManager() :
+  _regions(), _heap_mapper(NULL),
+  _prev_bitmap_mapper(NULL),
+  _next_bitmap_mapper(NULL),
+  _bot_mapper(NULL),
+  _cardtable_mapper(NULL),
+  _card_counts_mapper(NULL),
+  _free_list("Free list", new MasterFreeRegionListChecker()),
+  _available_map(mtGC),
+  _num_committed(0),
+  _allocated_heapregions_length(0)
+{ }
+
 void HeapRegionManager::initialize(G1RegionToSpaceMapper* heap_storage,
                                G1RegionToSpaceMapper* prev_bitmap,
                                G1RegionToSpaceMapper* next_bitmap,

src/hotspot/share/gc/g1/heapRegionManager.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2001, 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2001, 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
@@ -128,14 +128,7 @@ public:
 
  public:
   // Empty constructor, we'll initialize it with the initialize() method.
-  HeapRegionManager() :
+  HeapRegionManager();
-   _regions(), _heap_mapper(NULL),
-   _prev_bitmap_mapper(NULL), _next_bitmap_mapper(NULL), _bot_mapper(NULL),
-   _cardtable_mapper(NULL), _card_counts_mapper(NULL),
-   _free_list("Free list", new MasterFreeRegionListMtSafeChecker()),
-   _available_map(mtGC), _num_committed(0),
-   _allocated_heapregions_length(0)
-  { }
 
   void initialize(G1RegionToSpaceMapper* heap_storage,
                   G1RegionToSpaceMapper* prev_bitmap,

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

@@ -33,8 +33,8 @@ uint FreeRegionList::_unrealistically_long_length = 0;
 void HeapRegionSetBase::verify_region(HeapRegion* hr) {
   assert(hr->containing_set() == this, "Inconsistent containing set for %u", hr->hrm_index());
   assert(!hr->is_young(), "Adding young region %u", hr->hrm_index()); // currently we don't use these sets for young regions
-  assert(hr->is_humongous() == regions_humongous(), "Wrong humongous state for region %u and set %s", hr->hrm_index(), name());
+  assert(_checker == NULL || _checker->is_correct_type(hr), "Wrong type of region %u (%s) and set %s",
-  assert(hr->is_free() == regions_free(), "Wrong free state for region %u and set %s", hr->hrm_index(), name());
+         hr->hrm_index(), hr->get_type_str(), name());
   assert(!hr->is_free() || hr->is_empty(), "Free region %u is not empty for set %s", hr->hrm_index(), name());
   assert(!hr->is_empty() || hr->is_free() || hr->is_archive(),
          "Empty region %u is not free or archive for set %s", hr->hrm_index(), name());
@@ -75,18 +75,13 @@ void HeapRegionSetBase::verify_end() {
 void HeapRegionSetBase::print_on(outputStream* out, bool print_contents) {
   out->cr();
   out->print_cr("Set: %s (" PTR_FORMAT ")", name(), p2i(this));
-  out->print_cr("  Region Assumptions");
+  out->print_cr("  Region Type         : %s", _checker->get_description());
-  out->print_cr("    humongous         : %s", BOOL_TO_STR(regions_humongous()));
+  out->print_cr("  Length              : %14u", length());
-  out->print_cr("    archive           : %s", BOOL_TO_STR(regions_archive()));
-  out->print_cr("    free              : %s", BOOL_TO_STR(regions_free()));
-  out->print_cr("  Attributes");
-  out->print_cr("    length            : %14u", length());
 }
 
-HeapRegionSetBase::HeapRegionSetBase(const char* name, RegionSetKind kind, HRSMtSafeChecker* mt_safety_checker)
+HeapRegionSetBase::HeapRegionSetBase(const char* name, HeapRegionSetChecker* checker)
-  : _region_kind(kind), _mt_safety_checker(mt_safety_checker), _length(0), _name(name), _verify_in_progress(false)
+  : _checker(checker), _length(0), _name(name), _verify_in_progress(false)
 {
-  assert(kind >= ForOldRegions && kind <= ForFreeRegions, "Invalid heap region set kind %d.", kind);
 }
 
 void FreeRegionList::set_unrealistically_long_length(uint len) {
@@ -293,78 +288,3 @@ void FreeRegionList::verify_list() {
   guarantee(_tail == NULL || _tail->next() == NULL, "_tail should not have a next");
   guarantee(length() == count, "%s count mismatch. Expected %u, actual %u.", name(), length(), count);
 }
-
-// Note on the check_mt_safety() methods below:
-//
-// Verification of the "master" heap region sets / lists that are
-// maintained by G1CollectedHeap is always done during a STW pause and
-// by the VM thread at the start / end of the pause. The standard
-// verification methods all assert check_mt_safety(). This is
-// important as it ensures that verification is done without
-// concurrent updates taking place at the same time. It follows, that,
-// for the "master" heap region sets / lists, the check_mt_safety()
-// method should include the VM thread / STW case.
-
-void MasterFreeRegionListMtSafeChecker::check() {
-  // Master Free List MT safety protocol:
-  // (a) If we're at a safepoint, operations on the master free list
-  // should be invoked by either the VM thread (which will serialize
-  // them) or by the GC workers while holding the
-  // FreeList_lock.
-  // (b) If we're not at a safepoint, operations on the master free
-  // list should be invoked while holding the Heap_lock.
-
-  if (SafepointSynchronize::is_at_safepoint()) {
-    guarantee(Thread::current()->is_VM_thread() ||
-              FreeList_lock->owned_by_self(), "master free list MT safety protocol at a safepoint");
-  } else {
-    guarantee(Heap_lock->owned_by_self(), "master free list MT safety protocol outside a safepoint");
-  }
-}
-
-void OldRegionSetMtSafeChecker::check() {
-  // Master Old Set MT safety protocol:
-  // (a) If we're at a safepoint, operations on the master old set
-  // should be invoked:
-  // - by the VM thread (which will serialize them), or
-  // - by the GC workers while holding the FreeList_lock, if we're
-  //   at a safepoint for an evacuation pause (this lock is taken
-  //   anyway when an GC alloc region is retired so that a new one
-  //   is allocated from the free list), or
-  // - by the GC workers while holding the OldSets_lock, if we're at a
-  //   safepoint for a cleanup pause.
-  // (b) If we're not at a safepoint, operations on the master old set
-  // should be invoked while holding the Heap_lock.
-
-  if (SafepointSynchronize::is_at_safepoint()) {
-    guarantee(Thread::current()->is_VM_thread()
-        || FreeList_lock->owned_by_self() || OldSets_lock->owned_by_self(),
-        "master old set MT safety protocol at a safepoint");
-  } else {
-    guarantee(Heap_lock->owned_by_self(), "master old set MT safety protocol outside a safepoint");
-  }
-}
-
-void HumongousRegionSetMtSafeChecker::check() {
-  // Humongous Set MT safety protocol:
-  // (a) If we're at a safepoint, operations on the master humongous
-  // set should be invoked by either the VM thread (which will
-  // serialize them) or by the GC workers while holding the
-  // OldSets_lock.
-  // (b) If we're not at a safepoint, operations on the master
-  // humongous set should be invoked while holding the Heap_lock.
-
-  if (SafepointSynchronize::is_at_safepoint()) {
-    guarantee(Thread::current()->is_VM_thread() ||
-              OldSets_lock->owned_by_self(),
-              "master humongous set MT safety protocol at a safepoint");
-  } else {
-    guarantee(Heap_lock->owned_by_self(),
-              "master humongous set MT safety protocol outside a safepoint");
-  }
-}
-
-void ArchiveRegionSetMtSafeChecker::check() {
-  guarantee(!Universe::is_fully_initialized() || SafepointSynchronize::is_at_safepoint(),
-            "May only change archive regions during initialization or safepoint.");
-}

src/hotspot/share/gc/g1/heapRegionSet.hpp

@@ -47,16 +47,18 @@
   } while (0)
 
 
-class HRSMtSafeChecker : public CHeapObj<mtGC> {
+// Interface collecting various instance specific verification methods of
+// HeapRegionSets.
+class HeapRegionSetChecker : public CHeapObj<mtGC> {
 public:
-  virtual void check() = 0;
+  // Verify MT safety for this HeapRegionSet.
+  virtual void check_mt_safety() = 0;
+  // Returns true if the given HeapRegion is of the correct type for this HeapRegionSet.
+  virtual bool is_correct_type(HeapRegion* hr) = 0;
+  // Return a description of the type of regions this HeapRegionSet contains.
+  virtual const char* get_description() = 0;
 };
 
-class MasterFreeRegionListMtSafeChecker    : public HRSMtSafeChecker { public: void check(); };
-class HumongousRegionSetMtSafeChecker      : public HRSMtSafeChecker { public: void check(); };
-class OldRegionSetMtSafeChecker            : public HRSMtSafeChecker { public: void check(); };
-class ArchiveRegionSetMtSafeChecker        : public HRSMtSafeChecker { public: void check(); };
-
 // Base class for all the classes that represent heap region sets. It
 // contains the basic attributes that each set needs to maintain
 // (e.g., length, region num, used bytes sum) plus any shared
@@ -64,17 +66,8 @@ class ArchiveRegionSetMtSafeChecker        : public HRSMtSafeChecker { public: v
 
 class HeapRegionSetBase {
   friend class VMStructs;
-public:
-  enum RegionSetKind {
-    ForOldRegions,
-    ForHumongousRegions,
-    ForArchiveRegions,
-    ForFreeRegions
-  };
 
-private:
+  HeapRegionSetChecker* _checker;
-  RegionSetKind _region_kind;
-  HRSMtSafeChecker* _mt_safety_checker;
 
 protected:
   // The number of regions in to the set.
@@ -88,19 +81,13 @@ protected:
   // added to / removed from a set are consistent.
   void verify_region(HeapRegion* hr) PRODUCT_RETURN;
 
-  // Indicates whether all regions in the set should be of a given particular type.
-  // Only used for verification.
-  bool regions_humongous() const { return _region_kind == ForHumongousRegions; }
-  bool regions_archive() const { return _region_kind == ForArchiveRegions; }
-  bool regions_free() const { return _region_kind == ForFreeRegions; }
-
   void check_mt_safety() {
-    if (_mt_safety_checker != NULL) {
+    if (_checker != NULL) {
-      _mt_safety_checker->check();
+      _checker->check_mt_safety();
     }
   }
 
-  HeapRegionSetBase(const char* name, RegionSetKind kind, HRSMtSafeChecker* mt_safety_checker);
+  HeapRegionSetBase(const char* name, HeapRegionSetChecker* verifier);
 
 public:
   const char* name() { return _name; }
@@ -134,9 +121,8 @@ public:
 
 class HeapRegionSet : public HeapRegionSetBase {
 public:
-  HeapRegionSet(const char* name, RegionSetKind kind, HRSMtSafeChecker* mt_safety_checker):
+  HeapRegionSet(const char* name, HeapRegionSetChecker* checker):
-    HeapRegionSetBase(name, kind, mt_safety_checker) {
+    HeapRegionSetBase(name, checker) {
-    assert(kind != ForFreeRegions, "Must not call this constructor for Free regions.");
   }
 
   void bulk_remove(const uint removed) {
@@ -172,8 +158,8 @@ protected:
   virtual void clear();
 
 public:
-  FreeRegionList(const char* name, HRSMtSafeChecker* mt_safety_checker = NULL):
+  FreeRegionList(const char* name, HeapRegionSetChecker* checker = NULL):
-    HeapRegionSetBase(name, ForFreeRegions, mt_safety_checker) {
+    HeapRegionSetBase(name, checker) {
     clear();
   }