8276696: ParallelObjectIterator freed at the wrong time in VM_HeapDumper

fisk · fisk · commit f4dc03ea6de3 · 2021-11-23T14:34:21.000Z
Reviewed-by: pliden, stefank
diff --git a/src/hotspot/share/gc/g1/g1CollectedHeap.cpp b/src/hotspot/share/gc/g1/g1CollectedHeap.cpp
@@ -2260,7 +2260,7 @@ void G1CollectedHeap::object_iterate(ObjectClosure* cl) {
   heap_region_iterate(&blk);
 }
 
-class G1ParallelObjectIterator : public ParallelObjectIterator {
+class G1ParallelObjectIterator : public ParallelObjectIteratorImpl {
 private:
   G1CollectedHeap*  _heap;
   HeapRegionClaimer _claimer;
@@ -2275,7 +2275,7 @@ class G1ParallelObjectIterator : public ParallelObjectIterator {
   }
 };
 
-ParallelObjectIterator* G1CollectedHeap::parallel_object_iterator(uint thread_num) {
+ParallelObjectIteratorImpl* G1CollectedHeap::parallel_object_iterator(uint thread_num) {
   return new G1ParallelObjectIterator(thread_num);
 }
 
diff --git a/src/hotspot/share/gc/g1/g1CollectedHeap.hpp b/src/hotspot/share/gc/g1/g1CollectedHeap.hpp
@@ -1077,7 +1077,7 @@ class G1CollectedHeap : public CollectedHeap {
   // Iterate over all objects, calling "cl.do_object" on each.
   void object_iterate(ObjectClosure* cl) override;
 
-  ParallelObjectIterator* parallel_object_iterator(uint thread_num) override;
+  ParallelObjectIteratorImpl* parallel_object_iterator(uint thread_num) override;
 
   // Keep alive an object that was loaded with AS_NO_KEEPALIVE.
   void keep_alive(oop obj) override;
diff --git a/src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp b/src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp
@@ -600,7 +600,7 @@ void ParallelScavengeHeap::object_iterate_parallel(ObjectClosure* cl,
   }
 }
 
-class PSScavengeParallelObjectIterator : public ParallelObjectIterator {
+class PSScavengeParallelObjectIterator : public ParallelObjectIteratorImpl {
 private:
   ParallelScavengeHeap*  _heap;
   HeapBlockClaimer      _claimer;
@@ -615,7 +615,7 @@ class PSScavengeParallelObjectIterator : public ParallelObjectIterator {
   }
 };
 
-ParallelObjectIterator* ParallelScavengeHeap::parallel_object_iterator(uint thread_num) {
+ParallelObjectIteratorImpl* ParallelScavengeHeap::parallel_object_iterator(uint thread_num) {
   return new PSScavengeParallelObjectIterator();
 }
 
diff --git a/src/hotspot/share/gc/parallel/parallelScavengeHeap.hpp b/src/hotspot/share/gc/parallel/parallelScavengeHeap.hpp
@@ -231,7 +231,7 @@ class ParallelScavengeHeap : public CollectedHeap {
 
   void object_iterate(ObjectClosure* cl);
   void object_iterate_parallel(ObjectClosure* cl, HeapBlockClaimer* claimer);
-  virtual ParallelObjectIterator* parallel_object_iterator(uint thread_num);
+  virtual ParallelObjectIteratorImpl* parallel_object_iterator(uint thread_num);
 
   HeapWord* block_start(const void* addr) const;
   bool block_is_obj(const HeapWord* addr) const;
diff --git a/src/hotspot/share/gc/shared/collectedHeap.cpp b/src/hotspot/share/gc/shared/collectedHeap.cpp
@@ -110,6 +110,18 @@ void GCHeapLog::log_heap(CollectedHeap* heap, bool before) {
   st.print_cr("}");
 }
 
+ParallelObjectIterator::ParallelObjectIterator(uint thread_num) :
+  _impl(Universe::heap()->parallel_object_iterator(thread_num))
+{}
+
+ParallelObjectIterator::~ParallelObjectIterator() {
+  delete _impl;
+}
+
+void ParallelObjectIterator::object_iterate(ObjectClosure* cl, uint worker_id) {
+  _impl->object_iterate(cl, worker_id);
+}
+
 size_t CollectedHeap::unused() const {
   MutexLocker ml(Heap_lock);
   return capacity() - used();
diff --git a/src/hotspot/share/gc/shared/collectedHeap.hpp b/src/hotspot/share/gc/shared/collectedHeap.hpp
@@ -62,10 +62,23 @@ class VirtualSpaceSummary;
 class WorkerThreads;
 class nmethod;
 
-class ParallelObjectIterator : public CHeapObj<mtGC> {
+class ParallelObjectIteratorImpl : public CHeapObj<mtGC> {
 public:
+  virtual ~ParallelObjectIteratorImpl() {}
   virtual void object_iterate(ObjectClosure* cl, uint worker_id) = 0;
-  virtual ~ParallelObjectIterator() {}
+};
+
+// User facing parallel object iterator. This is a StackObj, which ensures that
+// the _impl is allocated and deleted in the scope of this object. This ensures
+// the life cycle of the implementation is as required by ThreadsListHandle,
+// which is sometimes used by the root iterators.
+class ParallelObjectIterator : public StackObj {
+  ParallelObjectIteratorImpl* _impl;
+
+public:
+  ParallelObjectIterator(uint thread_num);
+  ~ParallelObjectIterator();
+  void object_iterate(ObjectClosure* cl, uint worker_id);
 };
 
 //
@@ -82,6 +95,7 @@ class CollectedHeap : public CHeapObj<mtInternal> {
   friend class JVMCIVMStructs;
   friend class IsGCActiveMark; // Block structured external access to _is_gc_active
   friend class MemAllocator;
+  friend class ParallelObjectIterator;
 
  private:
   GCHeapLog* _gc_heap_log;
@@ -384,10 +398,12 @@ class CollectedHeap : public CHeapObj<mtInternal> {
   // Iterate over all objects, calling "cl.do_object" on each.
   virtual void object_iterate(ObjectClosure* cl) = 0;
 
-  virtual ParallelObjectIterator* parallel_object_iterator(uint thread_num) {
+ protected:
+  virtual ParallelObjectIteratorImpl* parallel_object_iterator(uint thread_num) {
     return NULL;
   }
 
+ public:
   // Keep alive an object that was loaded with AS_NO_KEEPALIVE.
   virtual void keep_alive(oop obj) {}
 
diff --git a/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp b/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp
@@ -1366,7 +1366,7 @@ class ShenandoahObjectIterateParScanClosure : public BasicOopIterateClosure {
 // parallel marking queues.
 // Every worker processes it's own marking queue. work-stealing is used
 // to balance workload.
-class ShenandoahParallelObjectIterator : public ParallelObjectIterator {
+class ShenandoahParallelObjectIterator : public ParallelObjectIteratorImpl {
 private:
   uint                         _num_workers;
   bool                         _init_ready;
@@ -1465,7 +1465,7 @@ class ShenandoahParallelObjectIterator : public ParallelObjectIterator {
   }
 };
 
-ParallelObjectIterator* ShenandoahHeap::parallel_object_iterator(uint workers) {
+ParallelObjectIteratorImpl* ShenandoahHeap::parallel_object_iterator(uint workers) {
   return new ShenandoahParallelObjectIterator(workers, &_aux_bit_map);
 }
 
diff --git a/src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp b/src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp
@@ -484,7 +484,7 @@ class ShenandoahHeap : public CollectedHeap {
   // Used for native heap walkers: heap dumpers, mostly
   void object_iterate(ObjectClosure* cl);
   // Parallel heap iteration support
-  virtual ParallelObjectIterator* parallel_object_iterator(uint workers);
+  virtual ParallelObjectIteratorImpl* parallel_object_iterator(uint workers);
 
   // Keep alive an object that was loaded with AS_NO_KEEPALIVE.
   void keep_alive(oop obj);
diff --git a/src/hotspot/share/gc/z/zCollectedHeap.cpp b/src/hotspot/share/gc/z/zCollectedHeap.cpp
@@ -226,7 +226,7 @@ void ZCollectedHeap::object_iterate(ObjectClosure* cl) {
   _heap.object_iterate(cl, true /* visit_weaks */);
 }
 
-ParallelObjectIterator* ZCollectedHeap::parallel_object_iterator(uint nworkers) {
+ParallelObjectIteratorImpl* ZCollectedHeap::parallel_object_iterator(uint nworkers) {
   return _heap.parallel_object_iterator(nworkers, true /* visit_weaks */);
 }
 
diff --git a/src/hotspot/share/gc/z/zCollectedHeap.hpp b/src/hotspot/share/gc/z/zCollectedHeap.hpp
@@ -95,7 +95,7 @@ class ZCollectedHeap : public CollectedHeap {
   virtual GrowableArray<MemoryPool*> memory_pools();
 
   virtual void object_iterate(ObjectClosure* cl);
-  virtual ParallelObjectIterator* parallel_object_iterator(uint nworkers);
+  virtual ParallelObjectIteratorImpl* parallel_object_iterator(uint nworkers);
 
   virtual void keep_alive(oop obj);
 
diff --git a/src/hotspot/share/gc/z/zHeap.cpp b/src/hotspot/share/gc/z/zHeap.cpp
@@ -439,7 +439,7 @@ void ZHeap::object_iterate(ObjectClosure* cl, bool visit_weaks) {
   iter.object_iterate(cl, 0 /* worker_id */);
 }
 
-ParallelObjectIterator* ZHeap::parallel_object_iterator(uint nworkers, bool visit_weaks) {
+ParallelObjectIteratorImpl* ZHeap::parallel_object_iterator(uint nworkers, bool visit_weaks) {
   assert(SafepointSynchronize::is_at_safepoint(), "Should be at safepoint");
   return new ZHeapIterator(nworkers, visit_weaks);
 }
diff --git a/src/hotspot/share/gc/z/zHeap.hpp b/src/hotspot/share/gc/z/zHeap.hpp
@@ -141,7 +141,7 @@ class ZHeap {
 
   // Iteration
   void object_iterate(ObjectClosure* cl, bool visit_weaks);
-  ParallelObjectIterator* parallel_object_iterator(uint nworkers, bool visit_weaks);
+  ParallelObjectIteratorImpl* parallel_object_iterator(uint nworkers, bool visit_weaks);
   void pages_do(ZPageClosure* cl);
 
   // Serviceability
diff --git a/src/hotspot/share/gc/z/zHeapIterator.hpp b/src/hotspot/share/gc/z/zHeapIterator.hpp
@@ -42,7 +42,7 @@ using ZHeapIteratorQueues = GenericTaskQueueSet<ZHeapIteratorQueue, mtGC>;
 using ZHeapIteratorArrayQueue = OverflowTaskQueue<ObjArrayTask, mtGC>;
 using ZHeapIteratorArrayQueues = GenericTaskQueueSet<ZHeapIteratorArrayQueue, mtGC>;
 
-class ZHeapIterator : public ParallelObjectIterator {
+class ZHeapIterator : public ParallelObjectIteratorImpl {
   friend class ZHeapIteratorContext;
 
 private:
diff --git a/src/hotspot/share/memory/heapInspection.cpp b/src/hotspot/share/memory/heapInspection.cpp
@@ -578,18 +578,12 @@ uintx HeapInspection::populate_table(KlassInfoTable* cit, BoolObjectClosure *fil
       const uint capped_parallel_thread_num = MIN2(parallel_thread_num, workers->max_workers());
       WithActiveWorkers with_active_workers(workers, capped_parallel_thread_num);
 
-      ParallelObjectIterator* poi = Universe::heap()->parallel_object_iterator(workers->active_workers());
-      if (poi != NULL) {
-        // The GC supports parallel object iteration.
-
-        ParHeapInspectTask task(poi, cit, filter);
-        // Run task with the active workers.
-        workers->run_task(&task);
-
-        delete poi;
-        if (task.success()) {
-          return task.missed_count();
-        }
+      ParallelObjectIterator poi(workers->active_workers());
+      ParHeapInspectTask task(&poi, cit, filter);
+      // Run task with the active workers.
+      workers->run_task(&task);
+      if (task.success()) {
+        return task.missed_count();
       }
     }
   }
diff --git a/src/hotspot/share/services/heapDumper.cpp b/src/hotspot/share/services/heapDumper.cpp
@@ -1910,7 +1910,6 @@ class VM_HeapDumper : public VM_GC_Operation, public WorkerTask {
       // Number of dumper threads that only iterate heap.
       uint _heap_only_dumper_threads = _num_dumper_threads - 1 /* VMDumper thread */;
       _dumper_controller = new (std::nothrow) DumperController(_heap_only_dumper_threads);
-      _poi = Universe::heap()->parallel_object_iterator(_num_dumper_threads);
     }
   }
 
@@ -1999,10 +1998,6 @@ class VM_HeapDumper : public VM_GC_Operation, public WorkerTask {
       }
       FREE_C_HEAP_ARRAY(ThreadStackTrace*, _stack_traces);
     }
-    if (_poi != NULL) {
-      delete _poi;
-      _poi = NULL;
-    }
     if (_dumper_controller != NULL) {
       delete _dumper_controller;
       _dumper_controller = NULL;
@@ -2252,7 +2247,14 @@ void VM_HeapDumper::doit() {
     work(0);
   } else {
     prepare_parallel_dump(workers->active_workers());
-    workers->run_task(this);
+    if (_num_dumper_threads > 1) {
+      ParallelObjectIterator poi(_num_dumper_threads);
+      _poi = &poi;
+      workers->run_task(this);
+      _poi = NULL;
+    } else {
+      workers->run_task(this);
+    }
     finish_parallel_dump();
   }
 

Original file line number	Diff line number	Diff line change
`@@ -2260,7 +2260,7 @@ void G1CollectedHeap::object_iterate(ObjectClosure* cl) {`
`2260`	`2260`	`heap_region_iterate(&blk);`
`2261`	`2261`	`}`
`2262`	`2262`
`2263`		`-class G1ParallelObjectIterator : public ParallelObjectIterator {`
	`2263`	`+class G1ParallelObjectIterator : public ParallelObjectIteratorImpl {`
`2264`	`2264`	`private:`
`2265`	`2265`	`G1CollectedHeap* _heap;`
`2266`	`2266`	`HeapRegionClaimer _claimer;`
`@@ -2275,7 +2275,7 @@ class G1ParallelObjectIterator : public ParallelObjectIterator {`
`2275`	`2275`	`}`
`2276`	`2276`	`};`
`2277`	`2277`
`2278`		`-ParallelObjectIterator* G1CollectedHeap::parallel_object_iterator(uint thread_num) {`
	`2278`	`+ParallelObjectIteratorImpl* G1CollectedHeap::parallel_object_iterator(uint thread_num) {`
`2279`	`2279`	`return new G1ParallelObjectIterator(thread_num);`
`2280`	`2280`	`}`
`2281`	`2281`
Original file line number	Diff line number	Diff line change
`@@ -600,7 +600,7 @@ void ParallelScavengeHeap::object_iterate_parallel(ObjectClosure* cl,`
`600`	`600`	`}`
`601`	`601`	`}`
`602`	`602`
`603`		`-class PSScavengeParallelObjectIterator : public ParallelObjectIterator {`
	`603`	`+class PSScavengeParallelObjectIterator : public ParallelObjectIteratorImpl {`
`604`	`604`	`private:`
`605`	`605`	`ParallelScavengeHeap* _heap;`
`606`	`606`	`HeapBlockClaimer _claimer;`
`@@ -615,7 +615,7 @@ class PSScavengeParallelObjectIterator : public ParallelObjectIterator {`
`615`	`615`	`}`
`616`	`616`	`};`
`617`	`617`
`618`		`-ParallelObjectIterator* ParallelScavengeHeap::parallel_object_iterator(uint thread_num) {`
	`618`	`+ParallelObjectIteratorImpl* ParallelScavengeHeap::parallel_object_iterator(uint thread_num) {`
`619`	`619`	`return new PSScavengeParallelObjectIterator();`
`620`	`620`	`}`
`621`	`621`
Original file line number	Diff line number	Diff line change
`@@ -226,7 +226,7 @@ void ZCollectedHeap::object_iterate(ObjectClosure* cl) {`
`226`	`226`	`_heap.object_iterate(cl, true /* visit_weaks */);`
`227`	`227`	`}`
`228`	`228`
`229`		`-ParallelObjectIterator* ZCollectedHeap::parallel_object_iterator(uint nworkers) {`
	`229`	`+ParallelObjectIteratorImpl* ZCollectedHeap::parallel_object_iterator(uint nworkers) {`
`230`	`230`	`return _heap.parallel_object_iterator(nworkers, true /* visit_weaks */);`
`231`	`231`	`}`
`232`	`232`