8209700: Remove HeapRegionSetBase::RegionSetKind for a more flexible approach
Reviewed-by: kbarrett, phh
--- a/src/hotspot/share/gc/g1/g1CollectedHeap.cpp Wed Aug 22 20:37:07 2018 +0200
+++ b/src/hotspot/share/gc/g1/g1CollectedHeap.cpp Wed Aug 22 20:37:07 2018 +0200
@@ -1405,6 +1405,68 @@
_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 @@
_eden_pool(NULL),
_survivor_pool(NULL),
_old_pool(NULL),
- _old_set("Old Region Set", HeapRegionSetBase::ForOldRegions, new OldRegionSetMtSafeChecker()),
- _archive_set("Archive Region Set", HeapRegionSetBase::ForArchiveRegions, new ArchiveRegionSetMtSafeChecker()),
- _humongous_set("Humongous Region Set", HeapRegionSetBase::ForHumongousRegions, new HumongousRegionSetMtSafeChecker()),
+ _old_set("Old Region Set", new OldRegionSetChecker()),
+ _archive_set("Archive Region Set", new ArchiveRegionSetChecker()),
+ _humongous_set("Humongous Region Set", new HumongousRegionSetChecker()),
_bot(NULL),
_listener(),
_hrm(),
--- a/src/hotspot/share/gc/g1/heapRegionManager.cpp Wed Aug 22 20:37:07 2018 +0200
+++ b/src/hotspot/share/gc/g1/heapRegionManager.cpp Wed Aug 22 20:37:07 2018 +0200
@@ -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,
--- a/src/hotspot/share/gc/g1/heapRegionManager.hpp Wed Aug 22 20:37:07 2018 +0200
+++ b/src/hotspot/share/gc/g1/heapRegionManager.hpp Wed Aug 22 20:37:07 2018 +0200
@@ -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:
// Empty constructor, we'll initialize it with the initialize() method.
- 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)
- { }
+ HeapRegionManager();
void initialize(G1RegionToSpaceMapper* heap_storage,
G1RegionToSpaceMapper* prev_bitmap,
--- a/src/hotspot/share/gc/g1/heapRegionSet.cpp Wed Aug 22 20:37:07 2018 +0200
+++ b/src/hotspot/share/gc/g1/heapRegionSet.cpp Wed Aug 22 20:37:07 2018 +0200
@@ -33,8 +33,8 @@
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(hr->is_free() == regions_free(), "Wrong free 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",
+ 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::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(" humongous : %s", BOOL_TO_STR(regions_humongous()));
- 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());
+ out->print_cr(" Region Type : %s", _checker->get_description());
+ out->print_cr(" Length : %14u", length());
}
-HeapRegionSetBase::HeapRegionSetBase(const char* name, RegionSetKind kind, HRSMtSafeChecker* mt_safety_checker)
- : _region_kind(kind), _mt_safety_checker(mt_safety_checker), _length(0), _name(name), _verify_in_progress(false)
+HeapRegionSetBase::HeapRegionSetBase(const char* name, HeapRegionSetChecker* checker)
+ : _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 @@
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.");
-}
--- a/src/hotspot/share/gc/g1/heapRegionSet.hpp Wed Aug 22 20:37:07 2018 +0200
+++ b/src/hotspot/share/gc/g1/heapRegionSet.hpp Wed Aug 22 20:37:07 2018 +0200
@@ -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 HeapRegionSetBase {
friend class VMStructs;
-public:
- enum RegionSetKind {
- ForOldRegions,
- ForHumongousRegions,
- ForArchiveRegions,
- ForFreeRegions
- };
-private:
- RegionSetKind _region_kind;
- HRSMtSafeChecker* _mt_safety_checker;
+ HeapRegionSetChecker* _checker;
protected:
// The number of regions in to the set.
@@ -88,19 +81,13 @@
// 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) {
- _mt_safety_checker->check();
+ if (_checker != NULL) {
+ _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 @@
class HeapRegionSet : public HeapRegionSetBase {
public:
- HeapRegionSet(const char* name, RegionSetKind kind, HRSMtSafeChecker* mt_safety_checker):
- HeapRegionSetBase(name, kind, mt_safety_checker) {
- assert(kind != ForFreeRegions, "Must not call this constructor for Free regions.");
+ HeapRegionSet(const char* name, HeapRegionSetChecker* checker):
+ HeapRegionSetBase(name, checker) {
}
void bulk_remove(const uint removed) {
@@ -172,8 +158,8 @@
virtual void clear();
public:
- FreeRegionList(const char* name, HRSMtSafeChecker* mt_safety_checker = NULL):
- HeapRegionSetBase(name, ForFreeRegions, mt_safety_checker) {
+ FreeRegionList(const char* name, HeapRegionSetChecker* checker = NULL):
+ HeapRegionSetBase(name, checker) {
clear();
}