6423256: GC stacks should use a better data structure

6942771: SEGV in ParScanThreadState::take_from_overflow_stack

Reviewed-by: apetrusenko, ysr, pbk

hotspot/src/share/vm/gc_implementation/parallelScavenge/pcTasks.cpp

@@ -59,8 +59,6 @@ void MarkFromRootsTask::do_it(GCTaskManager* manager, uint which) {
     PrintGCDetails && TraceParallelOldGCTasks, true, gclog_or_tty));
   ParCompactionManager* cm =
     ParCompactionManager::gc_thread_compaction_manager(which);
-  assert(cm->stacks_have_been_allocated(),
-         "Stack space has not been allocated");
   PSParallelCompact::MarkAndPushClosure mark_and_push_closure(cm);
 
   switch (_root_type) {
@@ -119,7 +117,6 @@ void MarkFromRootsTask::do_it(GCTaskManager* manager, uint which) {
 
   // Do the real work
   cm->follow_marking_stacks();
-  // cm->deallocate_stacks();
 }
 
 
@@ -135,8 +132,6 @@ void RefProcTaskProxy::do_it(GCTaskManager* manager, uint which)
     PrintGCDetails && TraceParallelOldGCTasks, true, gclog_or_tty));
   ParCompactionManager* cm =
     ParCompactionManager::gc_thread_compaction_manager(which);
-  assert(cm->stacks_have_been_allocated(),
-         "Stack space has not been allocated");
   PSParallelCompact::MarkAndPushClosure mark_and_push_closure(cm);
   PSParallelCompact::FollowStackClosure follow_stack_closure(cm);
   _rp_task.work(_work_id, *PSParallelCompact::is_alive_closure(),

hotspot/src/share/vm/gc_implementation/parallelScavenge/psCompactionManager.cpp

@@ -46,23 +46,6 @@ ParCompactionManager::ParCompactionManager() :
   marking_stack()->initialize();
   _objarray_stack.initialize();
   region_stack()->initialize();
-
-  // Note that _revisit_klass_stack is allocated out of the
-  // C heap (as opposed to out of ResourceArena).
-  int size =
-    (SystemDictionary::number_of_classes() * 2) * 2 / ParallelGCThreads;
-  _revisit_klass_stack = new (ResourceObj::C_HEAP) GrowableArray<Klass*>(size, true);
-  // From some experiments (#klass/k)^2 for k = 10 seems a better fit, but this will
-  // have to do for now until we are able to investigate a more optimal setting.
-  _revisit_mdo_stack = new (ResourceObj::C_HEAP) GrowableArray<DataLayout*>(size*2, true);
-}
-
-ParCompactionManager::~ParCompactionManager() {
-  delete _revisit_klass_stack;
-  delete _revisit_mdo_stack;
-  // _manager_array and _stack_array are statics
-  // shared with all instances of ParCompactionManager
-  // should not be deallocated.
 }
 
 void ParCompactionManager::initialize(ParMarkBitMap* mbm) {
@@ -134,9 +117,9 @@ ParCompactionManager::gc_thread_compaction_manager(int index) {
 }
 
 void ParCompactionManager::reset() {
-  for(uint i=0; i<ParallelGCThreads+1; i++) {
-    manager_array(i)->revisit_klass_stack()->clear();
-    manager_array(i)->revisit_mdo_stack()->clear();
+  for(uint i = 0; i < ParallelGCThreads + 1; i++) {
+    assert(manager_array(i)->revisit_klass_stack()->is_empty(), "sanity");
+    assert(manager_array(i)->revisit_mdo_stack()->is_empty(), "sanity");
   }
 }
 
@@ -178,10 +161,3 @@ void ParCompactionManager::drain_region_stacks() {
     }
   } while (!region_stack()->is_empty());
 }
-
-#ifdef ASSERT
-bool ParCompactionManager::stacks_have_been_allocated() {
-  return (revisit_klass_stack()->data_addr() != NULL &&
-          revisit_mdo_stack()->data_addr() != NULL);
-}
-#endif

hotspot/src/share/vm/gc_implementation/parallelScavenge/psCompactionManager.hpp

@@ -80,10 +80,9 @@ private:
   // type of TaskQueue.
   RegionTaskQueue               _region_stack;
 
-#if 1  // does this happen enough to need a per thread stack?
-  GrowableArray<Klass*>*        _revisit_klass_stack;
-  GrowableArray<DataLayout*>*   _revisit_mdo_stack;
-#endif
+  Stack<Klass*>                 _revisit_klass_stack;
+  Stack<DataLayout*>            _revisit_mdo_stack;
+
   static ParMarkBitMap* _mark_bitmap;
 
   Action _action;
@@ -113,10 +112,7 @@ private:
   inline static ParCompactionManager* manager_array(int index);
 
   ParCompactionManager();
-  ~ParCompactionManager();
 
-  void allocate_stacks();
-  void deallocate_stacks();
   ParMarkBitMap* mark_bitmap() { return _mark_bitmap; }
 
   // Take actions in preparation for a compaction.
@@ -129,11 +125,8 @@ private:
   bool should_verify_only();
   bool should_reset_only();
 
-#if 1
-  // Probably stays as a growable array
-  GrowableArray<Klass*>* revisit_klass_stack() { return _revisit_klass_stack; }
-  GrowableArray<DataLayout*>* revisit_mdo_stack() { return _revisit_mdo_stack; }
-#endif
+  Stack<Klass*>* revisit_klass_stack() { return &_revisit_klass_stack; }
+  Stack<DataLayout*>* revisit_mdo_stack() { return &_revisit_mdo_stack; }
 
   // Save for later processing.  Must not fail.
   inline void push(oop obj) { _marking_stack.push(obj); }
@@ -162,10 +155,6 @@ private:
   // Process tasks remaining on any stack
   void drain_region_stacks();
 
-  // Debugging support
-#ifdef ASSERT
-  bool stacks_have_been_allocated();
-#endif
 };
 
 inline ParCompactionManager* ParCompactionManager::manager_array(int index) {

hotspot/src/share/vm/gc_implementation/parallelScavenge/psMarkSweep.cpp

@@ -466,33 +466,16 @@ void PSMarkSweep::allocate_stacks() {
   _preserved_count_max  = pointer_delta(to_space->end(), to_space->top(), sizeof(jbyte));
   // Now divide by the size of a PreservedMark
   _preserved_count_max /= sizeof(PreservedMark);
-
-  _preserved_mark_stack = NULL;
-  _preserved_oop_stack = NULL;
-
-  _marking_stack = new (ResourceObj::C_HEAP) GrowableArray<oop>(4000, true);
-  _objarray_stack = new (ResourceObj::C_HEAP) GrowableArray<ObjArrayTask>(50, true);
-
-  int size = SystemDictionary::number_of_classes() * 2;
-  _revisit_klass_stack = new (ResourceObj::C_HEAP) GrowableArray<Klass*>(size, true);
-  // (#klass/k)^2, for k ~ 10 appears a better setting, but this will have to do for
-  // now until we investigate a more optimal setting.
-  _revisit_mdo_stack   = new (ResourceObj::C_HEAP) GrowableArray<DataLayout*>(size*2, true);
 }
 
 
 void PSMarkSweep::deallocate_stacks() {
-  if (_preserved_oop_stack) {
-    delete _preserved_mark_stack;
-    _preserved_mark_stack = NULL;
-    delete _preserved_oop_stack;
-    _preserved_oop_stack = NULL;
-  }
-
-  delete _marking_stack;
-  delete _objarray_stack;
-  delete _revisit_klass_stack;
-  delete _revisit_mdo_stack;
+  _preserved_mark_stack.clear(true);
+  _preserved_oop_stack.clear(true);
+  _marking_stack.clear();
+  _objarray_stack.clear(true);
+  _revisit_klass_stack.clear(true);
+  _revisit_mdo_stack.clear(true);
 }
 
 void PSMarkSweep::mark_sweep_phase1(bool clear_all_softrefs) {
@@ -542,17 +525,17 @@ void PSMarkSweep::mark_sweep_phase1(bool clear_all_softrefs) {
 
   // Update subklass/sibling/implementor links of live klasses
   follow_weak_klass_links();
-  assert(_marking_stack->is_empty(), "just drained");
+  assert(_marking_stack.is_empty(), "just drained");
 
   // Visit memoized mdo's and clear unmarked weak refs
   follow_mdo_weak_refs();
-  assert(_marking_stack->is_empty(), "just drained");
+  assert(_marking_stack.is_empty(), "just drained");
 
   // Visit symbol and interned string tables and delete unmarked oops
   SymbolTable::unlink(is_alive_closure());
   StringTable::unlink(is_alive_closure());
 
-  assert(_marking_stack->is_empty(), "stack should be empty by now");
+  assert(_marking_stack.is_empty(), "stack should be empty by now");
 }
 
 

hotspot/src/share/vm/gc_implementation/parallelScavenge/psParallelCompact.cpp

@@ -2170,6 +2170,16 @@ void PSParallelCompact::invoke_no_policy(bool maximum_heap_compaction) {
     heap->update_counters();
   }
 
+#ifdef ASSERT
+  for (size_t i = 0; i < ParallelGCThreads + 1; ++i) {
+    ParCompactionManager* const cm =
+      ParCompactionManager::manager_array(int(i));
+    assert(cm->marking_stack()->is_empty(),       "should be empty");
+    assert(cm->region_stack()->is_empty(),        "should be empty");
+    assert(cm->revisit_klass_stack()->is_empty(), "should be empty");
+  }
+#endif // ASSERT
+
   if (VerifyAfterGC && heap->total_collections() >= VerifyGCStartAt) {
     HandleMark hm;  // Discard invalid handles created during verification
     gclog_or_tty->print(" VerifyAfterGC:");
@@ -2711,21 +2721,22 @@ PSParallelCompact::follow_weak_klass_links() {
   // All klasses on the revisit stack are marked at this point.
   // Update and follow all subklass, sibling and implementor links.
   if (PrintRevisitStats) {
-    gclog_or_tty->print_cr("#classes in system dictionary = %d", SystemDictionary::number_of_classes());
+    gclog_or_tty->print_cr("#classes in system dictionary = %d",
+                           SystemDictionary::number_of_classes());
   }
   for (uint i = 0; i < ParallelGCThreads + 1; i++) {
     ParCompactionManager* cm = ParCompactionManager::manager_array(i);
     KeepAliveClosure keep_alive_closure(cm);
-    int length = cm->revisit_klass_stack()->length();
+    Stack<Klass*>* const rks = cm->revisit_klass_stack();
     if (PrintRevisitStats) {
-      gclog_or_tty->print_cr("Revisit klass stack[%d] length = %d", i, length);
+      gclog_or_tty->print_cr("Revisit klass stack[%u] length = " SIZE_FORMAT,
+                             i, rks->size());
     }
-    for (int j = 0; j < length; j++) {
-      cm->revisit_klass_stack()->at(j)->follow_weak_klass_links(
-        is_alive_closure(),
-        &keep_alive_closure);
+    while (!rks->is_empty()) {
+      Klass* const k = rks->pop();
+      k->follow_weak_klass_links(is_alive_closure(), &keep_alive_closure);
     }
-    // revisit_klass_stack is cleared in reset()
+
     cm->follow_marking_stacks();
   }
 }
@@ -2744,19 +2755,20 @@ void PSParallelCompact::follow_mdo_weak_refs() {
   // we can visit and clear any weak references from MDO's which
   // we memoized during the strong marking phase.
   if (PrintRevisitStats) {
-    gclog_or_tty->print_cr("#classes in system dictionary = %d", SystemDictionary::number_of_classes());
+    gclog_or_tty->print_cr("#classes in system dictionary = %d",
+                           SystemDictionary::number_of_classes());
   }
   for (uint i = 0; i < ParallelGCThreads + 1; i++) {
     ParCompactionManager* cm = ParCompactionManager::manager_array(i);
-    GrowableArray<DataLayout*>* rms = cm->revisit_mdo_stack();
-    int length = rms->length();
+    Stack<DataLayout*>* rms = cm->revisit_mdo_stack();
     if (PrintRevisitStats) {
-      gclog_or_tty->print_cr("Revisit MDO stack[%d] length = %d", i, length);
+      gclog_or_tty->print_cr("Revisit MDO stack[%u] size = " SIZE_FORMAT,
+                             i, rms->size());
     }
-    for (int j = 0; j < length; j++) {
-      rms->at(j)->follow_weak_refs(is_alive_closure());
+    while (!rms->is_empty()) {
+      rms->pop()->follow_weak_refs(is_alive_closure());
     }
-    // revisit_mdo_stack is cleared in reset()
+
     cm->follow_marking_stacks();
   }
 }

hotspot/src/share/vm/gc_implementation/parallelScavenge/psPromotionManager.cpp

@@ -185,7 +185,6 @@ void PSPromotionManager::reset() {
 
 
 void PSPromotionManager::drain_stacks_depth(bool totally_drain) {
-  assert(claimed_stack_depth()->overflow_stack() != NULL, "invariant");
   totally_drain = totally_drain || _totally_drain;
 
 #ifdef ASSERT

hotspot/src/share/vm/gc_implementation/parallelScavenge/psScavenge.cpp

@@ -34,9 +34,10 @@ bool                       PSScavenge::_survivor_overflow = false;
 int                        PSScavenge::_tenuring_threshold = 0;
 HeapWord*                  PSScavenge::_young_generation_boundary = NULL;
 elapsedTimer               PSScavenge::_accumulated_time;
-GrowableArray<markOop>*    PSScavenge::_preserved_mark_stack = NULL;
-GrowableArray<oop>*        PSScavenge::_preserved_oop_stack = NULL;
+Stack<markOop>             PSScavenge::_preserved_mark_stack;
+Stack<oop>                 PSScavenge::_preserved_oop_stack;
 CollectorCounters*         PSScavenge::_counters = NULL;
+bool                       PSScavenge::_promotion_failed = false;
 
 // Define before use
 class PSIsAliveClosure: public BoolObjectClosure {
@@ -223,6 +224,9 @@ bool PSScavenge::invoke_no_policy() {
   assert(SafepointSynchronize::is_at_safepoint(), "should be at safepoint");
   assert(Thread::current() == (Thread*)VMThread::vm_thread(), "should be in vm thread");
 
+  assert(_preserved_mark_stack.is_empty(), "should be empty");
+  assert(_preserved_oop_stack.is_empty(), "should be empty");
+
   TimeStamp scavenge_entry;
   TimeStamp scavenge_midpoint;
   TimeStamp scavenge_exit;
@@ -636,24 +640,20 @@ void PSScavenge::clean_up_failed_promotion() {
     young_gen->object_iterate(&unforward_closure);
 
     if (PrintGC && Verbose) {
-      gclog_or_tty->print_cr("Restoring %d marks",
-                              _preserved_oop_stack->length());
+      gclog_or_tty->print_cr("Restoring %d marks", _preserved_oop_stack.size());
     }
 
     // Restore any saved marks.
-    for (int i=0; i < _preserved_oop_stack->length(); i++) {
-      oop obj       = _preserved_oop_stack->at(i);
-      markOop mark  = _preserved_mark_stack->at(i);
+    while (!_preserved_oop_stack.is_empty()) {
+      oop obj      = _preserved_oop_stack.pop();
+      markOop mark = _preserved_mark_stack.pop();
       obj->set_mark(mark);
     }
 
-    // Deallocate the preserved mark and oop stacks.
-    // The stacks were allocated as CHeap objects, so
-    // we must call delete to prevent mem leaks.
-    delete _preserved_mark_stack;
-    _preserved_mark_stack = NULL;
-    delete _preserved_oop_stack;
-    _preserved_oop_stack = NULL;
+    // Clear the preserved mark and oop stack caches.
+    _preserved_mark_stack.clear(true);
+    _preserved_oop_stack.clear(true);
+    _promotion_failed = false;
   }
 
   // Reset the PromotionFailureALot counters.
@@ -661,27 +661,16 @@ void PSScavenge::clean_up_failed_promotion() {
 }
 
 // This method is called whenever an attempt to promote an object
-// fails. Some markOops will need preserving, some will not. Note
+// fails. Some markOops will need preservation, some will not. Note
 // that the entire eden is traversed after a failed promotion, with
 // all forwarded headers replaced by the default markOop. This means
 // it is not neccessary to preserve most markOops.
 void PSScavenge::oop_promotion_failed(oop obj, markOop obj_mark) {
-  if (_preserved_mark_stack == NULL) {
-    ThreadCritical tc; // Lock and retest
-    if (_preserved_mark_stack == NULL) {
-      assert(_preserved_oop_stack == NULL, "Sanity");
-      _preserved_mark_stack = new (ResourceObj::C_HEAP) GrowableArray<markOop>(40, true);
-      _preserved_oop_stack = new (ResourceObj::C_HEAP) GrowableArray<oop>(40, true);
-    }
-  }
-
-  // Because we must hold the ThreadCritical lock before using
-  // the stacks, we should be safe from observing partial allocations,
-  // which are also guarded by the ThreadCritical lock.
+  _promotion_failed = true;
   if (obj_mark->must_be_preserved_for_promotion_failure(obj)) {
     ThreadCritical tc;
-    _preserved_oop_stack->push(obj);
-    _preserved_mark_stack->push(obj_mark);
+    _preserved_oop_stack.push(obj);
+    _preserved_mark_stack.push(obj_mark);
   }
 }
 

hotspot/src/share/vm/gc_implementation/parallelScavenge/psScavenge.hpp

@@ -61,9 +61,10 @@ class PSScavenge: AllStatic {
   static HeapWord*           _young_generation_boundary; // The lowest address possible for the young_gen.
                                                          // This is used to decide if an oop should be scavenged,
                                                          // cards should be marked, etc.
-  static GrowableArray<markOop>* _preserved_mark_stack; // List of marks to be restored after failed promotion
-  static GrowableArray<oop>*     _preserved_oop_stack;  // List of oops that need their mark restored.
+  static Stack<markOop>          _preserved_mark_stack; // List of marks to be restored after failed promotion
+  static Stack<oop>              _preserved_oop_stack;  // List of oops that need their mark restored.
   static CollectorCounters*      _counters;         // collector performance counters
+  static bool                    _promotion_failed;
 
   static void clean_up_failed_promotion();
 
@@ -79,8 +80,7 @@ class PSScavenge: AllStatic {
   // Accessors
   static int              tenuring_threshold()  { return _tenuring_threshold; }
   static elapsedTimer*    accumulated_time()    { return &_accumulated_time; }
-  static bool             promotion_failed()
-    { return _preserved_mark_stack != NULL; }
+  static bool             promotion_failed()    { return _promotion_failed; }
   static int              consecutive_skipped_scavenges()
     { return _consecutive_skipped_scavenges; }