8160897: Concurrent mark mark stack memory allocation leaks memory
Summary: Fix and clean up concurrent mark mark stack memory allocation.
Reviewed-by: jmasa, sangheki
--- a/hotspot/src/share/vm/gc/g1/g1ConcurrentMark.cpp Tue Jul 12 15:06:18 2016 +0300
+++ b/hotspot/src/share/vm/gc/g1/g1ConcurrentMark.cpp Fri Jul 15 13:33:44 2016 +0200
@@ -132,109 +132,114 @@
heapWordToOffset(mr.end()), false);
}
-G1CMMarkStack::G1CMMarkStack(G1ConcurrentMark* cm) :
- _base(NULL), _cm(cm)
-{}
-
-bool G1CMMarkStack::allocate(size_t capacity) {
- // allocate a stack of the requisite depth
- ReservedSpace rs(ReservedSpace::allocation_align_size_up(capacity * sizeof(oop)));
+G1CMMarkStack::G1CMMarkStack() :
+ _reserved_space(),
+ _base(NULL),
+ _capacity(0),
+ _saved_index((size_t)AllBits),
+ _should_expand(false) {
+ set_empty();
+}
+
+bool G1CMMarkStack::resize(size_t new_capacity) {
+ assert(is_empty(), "Only resize when stack is empty.");
+ assert(new_capacity <= MarkStackSizeMax,
+ "Trying to resize stack to " SIZE_FORMAT " elements when the maximum is " SIZE_FORMAT, new_capacity, MarkStackSizeMax);
+
+ size_t reservation_size = ReservedSpace::allocation_align_size_up(new_capacity * sizeof(oop));
+
+ ReservedSpace rs(reservation_size);
if (!rs.is_reserved()) {
- log_warning(gc)("ConcurrentMark MarkStack allocation failure");
+ log_warning(gc)("Failed to reserve memory for new overflow mark stack with " SIZE_FORMAT " elements and size " SIZE_FORMAT "B.", new_capacity, reservation_size);
return false;
}
- MemTracker::record_virtual_memory_type((address)rs.base(), mtGC);
- if (!_virtual_space.initialize(rs, rs.size())) {
- log_warning(gc)("ConcurrentMark MarkStack backing store failure");
- // Release the virtual memory reserved for the marking stack
+
+ VirtualSpace vs;
+
+ if (!vs.initialize(rs, rs.size())) {
rs.release();
+ log_warning(gc)("Failed to commit memory for new overflow mark stack of size " SIZE_FORMAT "B.", rs.size());
return false;
}
- assert(_virtual_space.committed_size() == rs.size(),
- "Didn't reserve backing store for all of G1ConcurrentMark stack?");
- _base = (oop*) _virtual_space.low();
- setEmpty();
- _capacity = (jint) capacity;
- _saved_index = -1;
+
+ assert(vs.committed_size() == rs.size(), "Failed to commit all of the mark stack.");
+
+ // Release old mapping.
+ _reserved_space.release();
+
+ // Save new mapping for future unmapping.
+ _reserved_space = rs;
+
+ MemTracker::record_virtual_memory_type((address)_reserved_space.base(), mtGC);
+
+ _base = (oop*) vs.low();
+ _capacity = new_capacity;
+ set_empty();
_should_expand = false;
+
return true;
}
+bool G1CMMarkStack::allocate(size_t capacity) {
+ return resize(capacity);
+}
+
void G1CMMarkStack::expand() {
- // Called, during remark, if we've overflown the marking stack during marking.
- assert(isEmpty(), "stack should been emptied while handling overflow");
- assert(_capacity <= (jint) MarkStackSizeMax, "stack bigger than permitted");
// Clear expansion flag
_should_expand = false;
- if (_capacity == (jint) MarkStackSizeMax) {
- log_trace(gc)("(benign) Can't expand marking stack capacity, at max size limit");
+
+ if (_capacity == MarkStackSizeMax) {
+ log_debug(gc)("Can not expand overflow mark stack further, already at maximum capacity of " SIZE_FORMAT " elements.", _capacity);
return;
}
+ size_t old_capacity = _capacity;
// Double capacity if possible
- jint new_capacity = MIN2(_capacity*2, (jint) MarkStackSizeMax);
- // Do not give up existing stack until we have managed to
- // get the double capacity that we desired.
- ReservedSpace rs(ReservedSpace::allocation_align_size_up(new_capacity *
- sizeof(oop)));
- if (rs.is_reserved()) {
- // Release the backing store associated with old stack
- _virtual_space.release();
- // Reinitialize virtual space for new stack
- if (!_virtual_space.initialize(rs, rs.size())) {
- fatal("Not enough swap for expanded marking stack capacity");
- }
- _base = (oop*)(_virtual_space.low());
- _index = 0;
- _capacity = new_capacity;
+ size_t new_capacity = MIN2(old_capacity * 2, MarkStackSizeMax);
+
+ if (resize(new_capacity)) {
+ log_debug(gc)("Expanded marking stack capacity from " SIZE_FORMAT " to " SIZE_FORMAT " elements",
+ old_capacity, new_capacity);
} else {
- // Failed to double capacity, continue;
- log_trace(gc)("(benign) Failed to expand marking stack capacity from " SIZE_FORMAT "K to " SIZE_FORMAT "K",
- _capacity / K, new_capacity / K);
+ log_warning(gc)("Failed to expand marking stack capacity from " SIZE_FORMAT " to " SIZE_FORMAT " elements",
+ old_capacity, new_capacity);
}
}
-void G1CMMarkStack::set_should_expand() {
- // If we're resetting the marking state because of an
- // marking stack overflow, record that we should, if
- // possible, expand the stack.
- _should_expand = _cm->has_overflown();
-}
-
G1CMMarkStack::~G1CMMarkStack() {
if (_base != NULL) {
_base = NULL;
- _virtual_space.release();
+ _reserved_space.release();
}
}
-void G1CMMarkStack::par_push_arr(oop* ptr_arr, int n) {
+void G1CMMarkStack::par_push_arr(oop* buffer, size_t n) {
MutexLockerEx x(ParGCRareEvent_lock, Mutex::_no_safepoint_check_flag);
- jint start = _index;
- jint next_index = start + n;
+ size_t start = _index;
+ size_t next_index = start + n;
if (next_index > _capacity) {
_overflow = true;
return;
}
// Otherwise.
_index = next_index;
- for (int i = 0; i < n; i++) {
- int ind = start + i;
+ for (size_t i = 0; i < n; i++) {
+ size_t ind = start + i;
assert(ind < _capacity, "By overflow test above.");
- _base[ind] = ptr_arr[i];
+ _base[ind] = buffer[i];
}
}
-bool G1CMMarkStack::par_pop_arr(oop* ptr_arr, int max, int* n) {
+bool G1CMMarkStack::par_pop_arr(oop* buffer, size_t max, size_t* n) {
MutexLockerEx x(ParGCRareEvent_lock, Mutex::_no_safepoint_check_flag);
- jint index = _index;
+ size_t index = _index;
if (index == 0) {
*n = 0;
return false;
} else {
- int k = MIN2(max, index);
- jint new_ind = index - k;
- for (int j = 0; j < k; j++) {
- ptr_arr[j] = _base[new_ind + j];
+ size_t k = MIN2(max, index);
+ size_t new_ind = index - k;
+ for (size_t j = 0; j < k; j++) {
+ buffer[j] = _base[new_ind + j];
}
_index = new_ind;
*n = k;
@@ -243,20 +248,14 @@
}
void G1CMMarkStack::note_start_of_gc() {
- assert(_saved_index == -1,
- "note_start_of_gc()/end_of_gc() bracketed incorrectly");
+ assert(_saved_index == (size_t)AllBits, "note_start_of_gc()/end_of_gc() calls bracketed incorrectly");
_saved_index = _index;
}
void G1CMMarkStack::note_end_of_gc() {
- // This is intentionally a guarantee, instead of an assert. If we
- // accidentally add something to the mark stack during GC, it
- // will be a correctness issue so it's better if we crash. we'll
- // only check this once per GC anyway, so it won't be a performance
- // issue in any way.
- guarantee(_saved_index == _index,
- "saved index: %d index: %d", _saved_index, _index);
- _saved_index = -1;
+ guarantee(!stack_modified(), "Saved index " SIZE_FORMAT " must be the same as " SIZE_FORMAT, _saved_index, _index);
+
+ _saved_index = (size_t)AllBits;
}
G1CMRootRegions::G1CMRootRegions() :
@@ -351,7 +350,7 @@
_prevMarkBitMap(&_markBitMap1),
_nextMarkBitMap(&_markBitMap2),
- _markStack(this),
+ _global_mark_stack(),
// _finger set in set_non_marking_state
_max_worker_id(ParallelGCThreads),
@@ -485,8 +484,8 @@
}
}
- if (!_markStack.allocate(MarkStackSize)) {
- log_warning(gc)("Failed to allocate CM marking stack");
+ if (!_global_mark_stack.allocate(MarkStackSize)) {
+ vm_exit_during_initialization("Failed to allocate initial concurrent mark overflow mark stack.");
return;
}
@@ -541,8 +540,8 @@
void G1ConcurrentMark::reset_marking_state(bool clear_overflow) {
- _markStack.set_should_expand();
- _markStack.setEmpty(); // Also clears the _markStack overflow flag
+ _global_mark_stack.set_should_expand(has_overflown());
+ _global_mark_stack.set_empty(); // Also clears the overflow stack's overflow flag
if (clear_overflow) {
clear_has_overflown();
} else {
@@ -1076,7 +1075,7 @@
weakRefsWork(clear_all_soft_refs);
if (has_overflown()) {
- // Oops. We overflowed. Restart concurrent marking.
+ // We overflowed. Restart concurrent marking.
_restart_for_overflow = true;
// Verify the heap w.r.t. the previous marking bitmap.
@@ -1109,8 +1108,8 @@
}
// Expand the marking stack, if we have to and if we can.
- if (_markStack.should_expand()) {
- _markStack.expand();
+ if (_global_mark_stack.should_expand()) {
+ _global_mark_stack.expand();
}
// Statistics
@@ -1637,7 +1636,7 @@
// Set the soft reference policy
rp->setup_policy(clear_all_soft_refs);
- assert(_markStack.isEmpty(), "mark stack should be empty");
+ assert(_global_mark_stack.is_empty(), "mark stack should be empty");
// Instances of the 'Keep Alive' and 'Complete GC' closures used
// in serial reference processing. Note these closures are also
@@ -1692,10 +1691,10 @@
// oop closures will set the has_overflown flag if we overflow the
// global marking stack.
- assert(_markStack.overflow() || _markStack.isEmpty(),
+ assert(_global_mark_stack.overflow() || _global_mark_stack.is_empty(),
"mark stack should be empty (unless it overflowed)");
- if (_markStack.overflow()) {
+ if (_global_mark_stack.overflow()) {
// This should have been done already when we tried to push an
// entry on to the global mark stack. But let's do it again.
set_has_overflown();
@@ -1714,7 +1713,7 @@
return;
}
- assert(_markStack.isEmpty(), "Marking should have completed");
+ assert(_global_mark_stack.is_empty(), "Marking should have completed");
// Unload Klasses, String, Symbols, Code Cache, etc.
if (ClassUnloadingWithConcurrentMark) {
@@ -1967,7 +1966,7 @@
}
// Verify entries on the global mark stack
- _markStack.iterate(VerifyNoCSetOops("Stack"));
+ _global_mark_stack.iterate(VerifyNoCSetOops("Stack"));
// Verify entries on the task queues
for (uint i = 0; i < _max_worker_id; ++i) {
@@ -2366,13 +2365,13 @@
// local array where we'll store the entries that will be popped
// from the global stack.
oop buffer[global_stack_transfer_size];
- int n;
+ size_t n;
_cm->mark_stack_pop(buffer, global_stack_transfer_size, &n);
assert(n <= global_stack_transfer_size,
"we should not pop more than the given limit");
if (n > 0) {
// yes, we did actually pop at least one entry
- for (int i = 0; i < n; ++i) {
+ for (size_t i = 0; i < n; ++i) {
bool success = _task_queue->push(buffer[i]);
// We only call this when the local queue is empty or under a
// given target limit. So, we do not expect this push to fail.
--- a/hotspot/src/share/vm/gc/g1/g1ConcurrentMark.hpp Tue Jul 12 15:06:18 2016 +0300
+++ b/hotspot/src/share/vm/gc/g1/g1ConcurrentMark.hpp Fri Jul 15 13:33:44 2016 +0200
@@ -145,53 +145,56 @@
void clear_range(MemRegion mr);
};
-// Represents a marking stack used by ConcurrentMarking in the G1 collector.
+// Represents the overflow mark stack used by concurrent marking.
+//
+// Stores oops in a huge buffer in virtual memory that is always fully committed.
+// Resizing may only happen during a STW pause when the stack is empty.
class G1CMMarkStack VALUE_OBJ_CLASS_SPEC {
- VirtualSpace _virtual_space; // Underlying backing store for actual stack
- G1ConcurrentMark* _cm;
- oop* _base; // bottom of stack
- jint _index; // one more than last occupied index
- jint _capacity; // max #elements
- jint _saved_index; // value of _index saved at start of GC
+ ReservedSpace _reserved_space; // Space currently reserved for the mark stack.
+
+ oop* _base; // Bottom address of allocated memory area.
+ size_t _capacity; // Maximum number of elements.
+ size_t _index; // One more than last occupied index.
+
+ size_t _saved_index; // Value of _index saved at start of GC to detect mark stack modifications during that time.
bool _overflow;
bool _should_expand;
+ // Resizes the mark stack to the given new capacity. Releases any previous
+ // memory if successful.
+ bool resize(size_t new_capacity);
+
+ bool stack_modified() const { return _index != _saved_index; }
public:
- G1CMMarkStack(G1ConcurrentMark* cm);
+ G1CMMarkStack();
~G1CMMarkStack();
bool allocate(size_t capacity);
- // Pushes the first "n" elements of "ptr_arr" on the stack.
- // Locking impl: concurrency is allowed only with
- // "par_push_arr" and/or "par_pop_arr" operations, which use the same
- // locking strategy.
- void par_push_arr(oop* ptr_arr, int n);
+ // Pushes the first "n" elements of the given buffer on the stack.
+ void par_push_arr(oop* buffer, size_t n);
- // If returns false, the array was empty. Otherwise, removes up to "max"
- // elements from the stack, and transfers them to "ptr_arr" in an
- // unspecified order. The actual number transferred is given in "n" ("n
- // == 0" is deliberately redundant with the return value.) Locking impl:
- // concurrency is allowed only with "par_push_arr" and/or "par_pop_arr"
- // operations, which use the same locking strategy.
- bool par_pop_arr(oop* ptr_arr, int max, int* n);
+ // Moves up to max elements from the stack into the given buffer. Returns
+ // the number of elements pushed, and false if the array has been empty.
+ // Returns true if the buffer contains at least one element.
+ bool par_pop_arr(oop* buffer, size_t max, size_t* n);
- bool isEmpty() { return _index == 0; }
- int maxElems() { return _capacity; }
+ bool is_empty() const { return _index == 0; }
+ size_t capacity() const { return _capacity; }
- bool overflow() { return _overflow; }
+ bool overflow() const { return _overflow; }
void clear_overflow() { _overflow = false; }
bool should_expand() const { return _should_expand; }
- void set_should_expand();
+ void set_should_expand(bool value) { _should_expand = value; }
// Expand the stack, typically in response to an overflow condition
void expand();
- int size() { return _index; }
+ size_t size() const { return _index; }
- void setEmpty() { _index = 0; clear_overflow(); }
+ void set_empty() { _index = 0; clear_overflow(); }
// Record the current index.
void note_start_of_gc();
@@ -308,7 +311,7 @@
G1CMRootRegions _root_regions;
// For gray objects
- G1CMMarkStack _markStack; // Grey objects behind global finger
+ G1CMMarkStack _global_mark_stack; // Grey objects behind global finger
HeapWord* volatile _finger; // The global finger, region aligned,
// always points to the end of the
// last claimed region
@@ -478,21 +481,21 @@
// The push and pop operations are used by tasks for transfers
// between task-local queues and the global mark stack, and use
// locking for concurrency safety.
- bool mark_stack_push(oop* arr, int n) {
- _markStack.par_push_arr(arr, n);
- if (_markStack.overflow()) {
+ bool mark_stack_push(oop* arr, size_t n) {
+ _global_mark_stack.par_push_arr(arr, n);
+ if (_global_mark_stack.overflow()) {
set_has_overflown();
return false;
}
return true;
}
- void mark_stack_pop(oop* arr, int max, int* n) {
- _markStack.par_pop_arr(arr, max, n);
+ void mark_stack_pop(oop* arr, size_t max, size_t* n) {
+ _global_mark_stack.par_pop_arr(arr, max, n);
}
- size_t mark_stack_size() { return _markStack.size(); }
- size_t partial_mark_stack_size_target() { return _markStack.maxElems()/3; }
- bool mark_stack_overflow() { return _markStack.overflow(); }
- bool mark_stack_empty() { return _markStack.isEmpty(); }
+ size_t mark_stack_size() { return _global_mark_stack.size(); }
+ size_t partial_mark_stack_size_target() { return _global_mark_stack.capacity()/3; }
+ bool mark_stack_overflow() { return _global_mark_stack.overflow(); }
+ bool mark_stack_empty() { return _global_mark_stack.is_empty(); }
G1CMRootRegions* root_regions() { return &_root_regions; }
@@ -598,12 +601,12 @@
// Notify data structures that a GC has started.
void note_start_of_gc() {
- _markStack.note_start_of_gc();
+ _global_mark_stack.note_start_of_gc();
}
// Notify data structures that a GC is finished.
void note_end_of_gc() {
- _markStack.note_end_of_gc();
+ _global_mark_stack.note_end_of_gc();
}
// Verify that there are no CSet oops on the stacks (taskqueues /
@@ -660,17 +663,17 @@
class G1CMTask : public TerminatorTerminator {
private:
enum PrivateConstants {
- // the regular clock call is called once the scanned words reaches
+ // The regular clock call is called once the scanned words reaches
// this limit
words_scanned_period = 12*1024,
- // the regular clock call is called once the number of visited
+ // The regular clock call is called once the number of visited
// references reaches this limit
refs_reached_period = 384,
- // initial value for the hash seed, used in the work stealing code
+ // Initial value for the hash seed, used in the work stealing code
init_hash_seed = 17,
- // how many entries will be transferred between global stack and
- // local queues
- global_stack_transfer_size = 16
+ // How many entries will be transferred between global stack and
+ // local queues at once.
+ global_stack_transfer_size = 1024
};
uint _worker_id;
--- a/hotspot/src/share/vm/gc/g1/g1ConcurrentMark.inline.hpp Tue Jul 12 15:06:18 2016 +0300
+++ b/hotspot/src/share/vm/gc/g1/g1ConcurrentMark.inline.hpp Fri Jul 15 13:33:44 2016 +0200
@@ -91,8 +91,9 @@
template<typename Fn>
inline void G1CMMarkStack::iterate(Fn fn) {
- assert(_saved_index == _index, "saved index: %d index: %d", _saved_index, _index);
- for (int i = 0; i < _index; ++i) {
+ assert_at_safepoint(true);
+ assert(!stack_modified(), "Saved index " SIZE_FORMAT " must be the same as " SIZE_FORMAT, _saved_index, _index);
+ for (size_t i = 0; i < _index; ++i) {
fn(_base[i]);
}
}
--- a/hotspot/src/share/vm/memory/virtualspace.cpp Tue Jul 12 15:06:18 2016 +0300
+++ b/hotspot/src/share/vm/memory/virtualspace.cpp Fri Jul 15 13:33:44 2016 +0200
@@ -676,7 +676,7 @@
void VirtualSpace::release() {
- // This does not release memory it never reserved.
+ // This does not release memory it reserved.
// Caller must release via rs.release();
_low_boundary = NULL;
_high_boundary = NULL;