# HG changeset patch # User tschatzl # Date 1468582424 -7200 # Node ID 43c7f35b800e420189bdaac5b618a3354e1c7b04 # Parent 5955109add86bb9cdea6c1665091ddbb1857079b 8160897: Concurrent mark mark stack memory allocation leaks memory Summary: Fix and clean up concurrent mark mark stack memory allocation. Reviewed-by: jmasa, sangheki diff -r 5955109add86 -r 43c7f35b800e hotspot/src/share/vm/gc/g1/g1ConcurrentMark.cpp --- 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. diff -r 5955109add86 -r 43c7f35b800e hotspot/src/share/vm/gc/g1/g1ConcurrentMark.hpp --- 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; diff -r 5955109add86 -r 43c7f35b800e hotspot/src/share/vm/gc/g1/g1ConcurrentMark.inline.hpp --- 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 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]); } } diff -r 5955109add86 -r 43c7f35b800e hotspot/src/share/vm/memory/virtualspace.cpp --- 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;