8160897: Concurrent mark mark stack memory allocation leaks memory
authortschatzl
Fri, 15 Jul 2016 13:33:44 +0200
changeset 39966 43c7f35b800e
parent 39965 5955109add86
child 39968 e02d57bd3e01
8160897: Concurrent mark mark stack memory allocation leaks memory Summary: Fix and clean up concurrent mark mark stack memory allocation. Reviewed-by: jmasa, sangheki
hotspot/src/share/vm/gc/g1/g1ConcurrentMark.cpp
hotspot/src/share/vm/gc/g1/g1ConcurrentMark.hpp
hotspot/src/share/vm/gc/g1/g1ConcurrentMark.inline.hpp
hotspot/src/share/vm/memory/virtualspace.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.
--- 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;