8221435: Shenandoah should not mark through weak roots
authorzgu
Tue, 26 Mar 2019 12:12:49 -0400
changeset 54338 7a34a3270270
parent 54337 5a9d780eb9dd
child 54339 f69a2f675f19
8221435: Shenandoah should not mark through weak roots Reviewed-by: rkennke, shade
src/hotspot/share/gc/shenandoah/shenandoahConcurrentMark.cpp
src/hotspot/share/gc/shenandoah/shenandoahConcurrentMark.hpp
src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp
src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp
src/hotspot/share/gc/shenandoah/shenandoahMarkCompact.cpp
src/hotspot/share/gc/shenandoah/shenandoahRootProcessor.cpp
src/hotspot/share/gc/shenandoah/shenandoahRootProcessor.hpp
src/hotspot/share/gc/shenandoah/shenandoahStringDedup.cpp
src/hotspot/share/gc/shenandoah/shenandoahStringDedup.hpp
src/hotspot/share/gc/shenandoah/shenandoahTraversalGC.cpp
--- a/src/hotspot/share/gc/shenandoah/shenandoahConcurrentMark.cpp	Fri Mar 29 15:59:28 2019 +0100
+++ b/src/hotspot/share/gc/shenandoah/shenandoahConcurrentMark.cpp	Tue Mar 26 12:12:49 2019 -0400
@@ -120,11 +120,10 @@
 
     CLDToOopClosure clds_cl(oops, ClassLoaderData::_claim_strong);
     MarkingCodeBlobClosure blobs_cl(oops, ! CodeBlobToOopClosure::FixRelocations);
-    OopClosure* weak_oops = _process_refs ? NULL : oops;
 
     ResourceMark m;
     if (heap->unload_classes()) {
-      _rp->process_strong_roots(oops, weak_oops, &clds_cl, NULL, &blobs_cl, NULL, worker_id);
+      _rp->process_strong_roots(oops, &clds_cl, NULL, &blobs_cl, NULL, worker_id);
     } else {
       if (ShenandoahConcurrentScanCodeRoots) {
         CodeBlobClosure* code_blobs = NULL;
@@ -137,9 +136,9 @@
           code_blobs = &assert_to_space;
         }
 #endif
-        _rp->process_all_roots(oops, weak_oops, &clds_cl, code_blobs, NULL, worker_id);
+        _rp->process_all_roots(oops, &clds_cl, code_blobs, NULL, worker_id);
       } else {
-        _rp->process_all_roots(oops, weak_oops, &clds_cl, &blobs_cl, NULL, worker_id);
+        _rp->process_all_roots(oops, &clds_cl, &blobs_cl, NULL, worker_id);
       }
     }
   }
@@ -177,7 +176,7 @@
         DEBUG_ONLY(&assert_to_space)
         NOT_DEBUG(NULL);
     }
-    _rp->process_all_roots(&cl, &cl, &cldCl, code_blobs, NULL, worker_id);
+    _rp->update_all_roots(&cl, &cldCl, code_blobs, NULL, worker_id);
   }
 };
 
@@ -446,11 +445,16 @@
     weak_refs_work(full_gc);
   }
 
+  weak_roots_work();
+
   // And finally finish class unloading
   if (_heap->unload_classes()) {
     _heap->unload_classes_and_cleanup_tables(full_gc);
+  } else if (ShenandoahStringDedup::is_enabled()) {
+    ShenandoahIsAliveSelector alive;
+    BoolObjectClosure* is_alive = alive.is_alive_closure();
+    ShenandoahStringDedup::unlink_or_oops_do(is_alive, NULL, false);
   }
-
   assert(task_queues()->is_empty(), "Should be empty");
   TASKQUEUE_STATS_ONLY(task_queues()->print_taskqueue_stats());
   TASKQUEUE_STATS_ONLY(task_queues()->reset_taskqueue_stats());
@@ -555,11 +559,13 @@
 private:
   template <class T>
   inline void do_oop_work(T* p) {
+#ifdef ASSERT
     T o = RawAccess<>::oop_load(p);
     if (!CompressedOops::is_null(o)) {
       oop obj = CompressedOops::decode_not_null(o);
       shenandoah_assert_not_forwarded(p, obj);
     }
+#endif
   }
 
 public:
@@ -648,6 +654,23 @@
 
 }
 
+// Process leftover weak oops: update them, if needed or assert they do not
+// need updating otherwise.
+// Weak processor API requires us to visit the oops, even if we are not doing
+// anything to them.
+void ShenandoahConcurrentMark::weak_roots_work() {
+  WorkGang* workers = _heap->workers();
+  ShenandoahIsAliveSelector is_alive;
+
+  if (_heap->has_forwarded_objects()) {
+    ShenandoahWeakUpdateClosure cl;
+    WeakProcessor::weak_oops_do(workers, is_alive.is_alive_closure(), &cl, 1);
+  } else {
+    ShenandoahWeakAssertNotForwardedClosure cl;
+    WeakProcessor::weak_oops_do(workers, is_alive.is_alive_closure(), &cl, 1);
+  }
+}
+
 void ShenandoahConcurrentMark::weak_refs_work_doit(bool full_gc) {
   ReferenceProcessor* rp = _heap->ref_processor();
 
@@ -689,26 +712,18 @@
     ShenandoahGCPhase phase(phase_process);
     ShenandoahTerminationTracker phase_term(phase_process_termination);
 
-    // Process leftover weak oops: update them, if needed (using parallel version),
-    // or assert they do not need updating (using serial version) otherwise.
-    // Weak processor API requires us to visit the oops, even if we are not doing
-    // anything to them.
     if (_heap->has_forwarded_objects()) {
       ShenandoahCMKeepAliveUpdateClosure keep_alive(get_queue(serial_worker_id));
       rp->process_discovered_references(is_alive.is_alive_closure(), &keep_alive,
                                         &complete_gc, &executor,
                                         &pt);
 
-      ShenandoahWeakUpdateClosure cl;
-      WeakProcessor::weak_oops_do(workers, is_alive.is_alive_closure(), &cl, 1);
     } else {
       ShenandoahCMKeepAliveClosure keep_alive(get_queue(serial_worker_id));
       rp->process_discovered_references(is_alive.is_alive_closure(), &keep_alive,
                                         &complete_gc, &executor,
                                         &pt);
 
-      ShenandoahWeakAssertNotForwardedClosure cl;
-      WeakProcessor::weak_oops_do(is_alive.is_alive_closure(), &cl);
     }
 
     pt.print_all_references();
--- a/src/hotspot/share/gc/shenandoah/shenandoahConcurrentMark.hpp	Fri Mar 29 15:59:28 2019 +0100
+++ b/src/hotspot/share/gc/shenandoah/shenandoahConcurrentMark.hpp	Tue Mar 26 12:12:49 2019 -0400
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2013, 2018, Red Hat, Inc. All rights reserved.
+ * Copyright (c) 2013, 2019, Red Hat, Inc. All rights reserved.
  *
  * This code is free software; you can redistribute it and/or modify it
  * under the terms of the GNU General Public License version 2 only, as
@@ -86,6 +86,8 @@
   void weak_refs_work(bool full_gc);
   void weak_refs_work_doit(bool full_gc);
 
+  void weak_roots_work();
+
 public:
   void preclean_weak_refs();
 
--- a/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp	Fri Mar 29 15:59:28 2019 +0100
+++ b/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp	Tue Mar 26 12:12:49 2019 -0400
@@ -1127,16 +1127,6 @@
 #endif
 }
 
-void ShenandoahHeap::roots_iterate(OopClosure* cl) {
-  assert(ShenandoahSafepoint::is_at_shenandoah_safepoint(), "Only iterate roots while world is stopped");
-
-  CodeBlobToOopClosure blobsCl(cl, false);
-  CLDToOopClosure cldCl(cl, ClassLoaderData::_claim_strong);
-
-  ShenandoahRootProcessor rp(this, 1, ShenandoahPhaseTimings::_num_phases);
-  rp.process_all_roots(cl, NULL, &cldCl, &blobsCl, NULL, 0);
-}
-
 // Returns size in bytes
 size_t ShenandoahHeap::unsafe_max_tlab_alloc(Thread *thread) const {
   if (ShenandoahElasticTLAB) {
@@ -1337,7 +1327,7 @@
   ObjectIterateScanRootClosure oops(&_aux_bit_map, &oop_stack);
   CLDToOopClosure clds(&oops, ClassLoaderData::_claim_none);
   CodeBlobToOopClosure blobs(&oops, false);
-  rp.process_all_roots(&oops, &oops, &clds, &blobs, NULL, 0);
+  rp.process_all_roots(&oops, &clds, &blobs, NULL, 0);
 
   // Work through the oop stack to traverse heap.
   while (! oop_stack.is_empty()) {
--- a/src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp	Fri Mar 29 15:59:28 2019 +0100
+++ b/src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp	Tue Mar 26 12:12:49 2019 -0400
@@ -742,8 +742,6 @@
 
   void stop_concurrent_marking();
 
-  void roots_iterate(OopClosure* cl);
-
 private:
   void trash_cset_regions();
   void update_heap_references(bool concurrent);
--- a/src/hotspot/share/gc/shenandoah/shenandoahMarkCompact.cpp	Fri Mar 29 15:59:28 2019 +0100
+++ b/src/hotspot/share/gc/shenandoah/shenandoahMarkCompact.cpp	Tue Mar 26 12:12:49 2019 -0400
@@ -572,9 +572,9 @@
     MarkingCodeBlobClosure adjust_code_closure(&cl,
                                              CodeBlobToOopClosure::FixRelocations);
 
-    _rp->process_all_roots(&cl, &cl,
-                           &adjust_cld_closure,
-                           &adjust_code_closure, NULL, worker_id);
+    _rp->update_all_roots(&cl,
+                          &adjust_cld_closure,
+                          &adjust_code_closure, NULL, worker_id);
   }
 };
 
--- a/src/hotspot/share/gc/shenandoah/shenandoahRootProcessor.cpp	Fri Mar 29 15:59:28 2019 +0100
+++ b/src/hotspot/share/gc/shenandoah/shenandoahRootProcessor.cpp	Tue Mar 26 12:12:49 2019 -0400
@@ -119,14 +119,9 @@
   Management::oops_do(oops);
   JvmtiExport::oops_do(oops);
   JNIHandles::oops_do(oops);
-  WeakProcessor::oops_do(oops);
   ObjectSynchronizer::oops_do(oops);
   SystemDictionary::oops_do(oops);
 
-  if (ShenandoahStringDedup::is_enabled()) {
-    ShenandoahStringDedup::oops_do_slow(oops);
-  }
-
   // Do thread roots the last. This allows verification code to find
   // any broken objects from those special roots first, not the accidental
   // dangling reference from the thread root.
@@ -134,7 +129,6 @@
 }
 
 void ShenandoahRootProcessor::process_strong_roots(OopClosure* oops,
-                                                   OopClosure* weak_oops,
                                                    CLDClosure* clds,
                                                    CLDClosure* weak_clds,
                                                    CodeBlobClosure* blobs,
@@ -142,13 +136,12 @@
                                                    uint worker_id) {
 
   process_java_roots(oops, clds, weak_clds, blobs, thread_cl, worker_id);
-  process_vm_roots(oops, NULL, weak_oops, worker_id);
+  process_vm_roots(oops, worker_id);
 
   _process_strong_tasks->all_tasks_completed(n_workers());
 }
 
 void ShenandoahRootProcessor::process_all_roots(OopClosure* oops,
-                                                OopClosure* weak_oops,
                                                 CLDClosure* clds,
                                                 CodeBlobClosure* blobs,
                                                 ThreadClosure* thread_cl,
@@ -156,7 +149,7 @@
 
   ShenandoahWorkerTimings* worker_times = ShenandoahHeap::heap()->phase_timings()->worker_times();
   process_java_roots(oops, clds, clds, blobs, thread_cl, worker_id);
-  process_vm_roots(oops, oops, weak_oops, worker_id);
+  process_vm_roots(oops, worker_id);
 
   if (blobs != NULL) {
     ShenandoahWorkerTimingsTracker timer(worker_times, ShenandoahPhaseTimings::CodeCacheRoots, worker_id);
@@ -164,6 +157,40 @@
   }
 
   _process_strong_tasks->all_tasks_completed(n_workers());
+
+}
+
+void ShenandoahRootProcessor::update_all_roots(OopClosure* oops,
+                                               CLDClosure* clds,
+                                               CodeBlobClosure* blobs,
+                                               ThreadClosure* thread_cl,
+                                               uint worker_id) {
+  process_all_roots(oops, clds, blobs, thread_cl, worker_id);
+
+  AlwaysTrueClosure always_true;
+  _weak_processor_task.work<AlwaysTrueClosure, OopClosure>(worker_id, &always_true, oops);
+  _processed_weak_roots = true;
+
+  if (ShenandoahStringDedup::is_enabled()) {
+    ShenandoahStringDedup::parallel_oops_do(&always_true, oops, worker_id);
+  }
+}
+
+void ShenandoahRootProcessor::traversal_update_all_roots(OopClosure* oops,
+                                                         CLDClosure* clds,
+                                                         CodeBlobClosure* blobs,
+                                                         ThreadClosure* thread_cl,
+                                                         uint worker_id) {
+  process_all_roots(oops, clds, blobs, thread_cl, worker_id);
+
+  ShenandoahIsAliveSelector is_alive;
+  BoolObjectClosure* is_alive_closure = is_alive.is_alive_closure();
+  _weak_processor_task.work<BoolObjectClosure, OopClosure>(worker_id, is_alive_closure, oops);
+  _processed_weak_roots = true;
+
+  if (ShenandoahStringDedup::is_enabled()) {
+    ShenandoahStringDedup::parallel_oops_do(is_alive_closure, oops, worker_id);
+  }
 }
 
 class ShenandoahParallelOopsDoThreadClosure : public ThreadClosure {
@@ -209,10 +236,7 @@
 }
 
 void ShenandoahRootProcessor::process_vm_roots(OopClosure* strong_roots,
-                                               OopClosure* weak_roots,
-                                               OopClosure* jni_weak_roots,
-                                               uint worker_id)
-{
+                                               uint worker_id) {
   ShenandoahWorkerTimings* worker_times = ShenandoahHeap::heap()->phase_timings()->worker_times();
   if (_process_strong_tasks->try_claim_task(SHENANDOAH_RP_PS_Universe_oops_do)) {
     ShenandoahWorkerTimingsTracker timer(worker_times, ShenandoahPhaseTimings::UniverseRoots, worker_id);
@@ -235,15 +259,6 @@
     ShenandoahWorkerTimingsTracker timer(worker_times, ShenandoahPhaseTimings::SystemDictionaryRoots, worker_id);
     SystemDictionary::oops_do(strong_roots);
   }
-  if (jni_weak_roots != NULL) {
-    AlwaysTrueClosure always_true;
-    _weak_processor_task.work<AlwaysTrueClosure, OopClosure>(worker_id, &always_true, jni_weak_roots);
-    _processed_weak_roots = true;
-  }
-
-  if (ShenandoahStringDedup::is_enabled() && weak_roots != NULL) {
-    ShenandoahStringDedup::parallel_oops_do(weak_roots, worker_id);
-  }
 
   {
     ShenandoahWorkerTimingsTracker timer(worker_times, ShenandoahPhaseTimings::ObjectSynchronizerRoots, worker_id);
--- a/src/hotspot/share/gc/shenandoah/shenandoahRootProcessor.hpp	Fri Mar 29 15:59:28 2019 +0100
+++ b/src/hotspot/share/gc/shenandoah/shenandoahRootProcessor.hpp	Tue Mar 26 12:12:49 2019 -0400
@@ -73,8 +73,6 @@
                           uint worker_i);
 
   void process_vm_roots(OopClosure* scan_non_heap_roots,
-                        OopClosure* scan_non_heap_weak_roots,
-                        OopClosure* weak_jni_roots,
                         uint worker_i);
 
   void weak_processor_timing_to_shenandoah_timing(const WeakProcessorPhases::Phase wpp,
@@ -86,21 +84,40 @@
                           ShenandoahPhaseTimings::Phase phase);
   ~ShenandoahRootProcessor();
 
-  // Apply oops, clds and blobs to all strongly reachable roots in the system
-  void process_strong_roots(OopClosure* oops, OopClosure* weak_oops,
+  // Apply oops, clds and blobs to all strongly reachable roots in the system.
+  // Optionally, apply class loader closure to weak clds, depending on class unloading
+  // for the particular GC cycles.
+  void process_strong_roots(OopClosure* oops,
                             CLDClosure* clds,
                             CLDClosure* weak_clds,
                             CodeBlobClosure* blobs,
                             ThreadClosure* thread_cl,
                             uint worker_id);
 
-  // Apply oops, clds and blobs to strongly and weakly reachable roots in the system
-  void process_all_roots(OopClosure* oops, OopClosure* weak_oops,
+  // Apply oops, clds and blobs to strongly reachable roots in the system
+  void process_all_roots(OopClosure* oops,
                          CLDClosure* clds,
                          CodeBlobClosure* blobs,
                          ThreadClosure* thread_cl,
                          uint worker_id);
 
+  // Apply oops, clds and blobs to strongly and weakly reachable roots in the system
+  void update_all_roots(OopClosure* oops,
+                        CLDClosure* clds,
+                        CodeBlobClosure* blobs,
+                        ThreadClosure* thread_cl,
+                        uint worker_id);
+
+
+  // Apply oops, clds and blobs to strongly and weakly reachable roots in the system
+  // during traversal GC.
+  // It cleans up and updates weak roots in one iteration.
+  void traversal_update_all_roots(OopClosure* oops,
+                                  CLDClosure* clds,
+                                  CodeBlobClosure* blobs,
+                                  ThreadClosure* thread_cl,
+                                  uint worker_id);
+
   // For slow debug/verification code
   void process_all_roots_slow(OopClosure* oops);
 
--- a/src/hotspot/share/gc/shenandoah/shenandoahStringDedup.cpp	Fri Mar 29 15:59:28 2019 +0100
+++ b/src/hotspot/share/gc/shenandoah/shenandoahStringDedup.cpp	Tue Mar 26 12:12:49 2019 -0400
@@ -71,13 +71,13 @@
   StringDedupTable::deduplicate(java_string, &dummy);
 }
 
-void ShenandoahStringDedup::parallel_oops_do(OopClosure* cl, uint worker_id) {
+void ShenandoahStringDedup::parallel_oops_do(BoolObjectClosure* is_alive, OopClosure* cl, uint worker_id) {
   assert(SafepointSynchronize::is_at_safepoint(), "Must be at a safepoint");
   assert(is_enabled(), "String deduplication not enabled");
 
   ShenandoahWorkerTimings* worker_times = ShenandoahHeap::heap()->phase_timings()->worker_times();
 
-  StringDedupUnlinkOrOopsDoClosure sd_cl(NULL, cl);
+  StringDedupUnlinkOrOopsDoClosure sd_cl(is_alive, cl);
 
   {
     ShenandoahWorkerTimingsTracker x(worker_times, ShenandoahPhaseTimings::StringDedupQueueRoots, worker_id);
--- a/src/hotspot/share/gc/shenandoah/shenandoahStringDedup.hpp	Fri Mar 29 15:59:28 2019 +0100
+++ b/src/hotspot/share/gc/shenandoah/shenandoahStringDedup.hpp	Tue Mar 26 12:12:49 2019 -0400
@@ -38,11 +38,11 @@
   // Deduplicate a string, the call is lock-free
   static void deduplicate(oop java_string);
 
-  static void parallel_oops_do(OopClosure* cl, uint worker_id);
+  static void parallel_oops_do(BoolObjectClosure* is_alive, OopClosure* cl, uint worker_id);
   static void oops_do_slow(OopClosure* cl);
 
   static inline bool is_candidate(oop obj);
-private:
+
   static void unlink_or_oops_do(BoolObjectClosure* is_alive,
                                 OopClosure* keep_alive,
                                 bool allow_resize_and_rehash);
--- a/src/hotspot/share/gc/shenandoah/shenandoahTraversalGC.cpp	Fri Mar 29 15:59:28 2019 +0100
+++ b/src/hotspot/share/gc/shenandoah/shenandoahTraversalGC.cpp	Tue Mar 26 12:12:49 2019 -0400
@@ -188,13 +188,13 @@
       ShenandoahMarkCLDClosure cld_cl(&roots_cl);
       MarkingCodeBlobClosure code_cl(&roots_cl, CodeBlobToOopClosure::FixRelocations);
       if (unload_classes) {
-        _rp->process_strong_roots(&roots_cl, process_refs ? NULL : &roots_cl, &cld_cl, NULL, NULL, NULL, worker_id);
+        _rp->process_strong_roots(&roots_cl, &cld_cl, NULL, NULL, NULL, worker_id);
         // Need to pre-evac code roots here. Otherwise we might see from-space constants.
         ShenandoahWorkerTimings* worker_times = _heap->phase_timings()->worker_times();
         ShenandoahWorkerTimingsTracker timer(worker_times, ShenandoahPhaseTimings::CodeCacheRoots, worker_id);
         _cset_coderoots->possibly_parallel_blobs_do(&code_cl);
       } else {
-        _rp->process_all_roots(&roots_cl, process_refs ? NULL : &roots_cl, &cld_cl, &code_cl, NULL, worker_id);
+        _rp->process_all_roots(&roots_cl, &cld_cl, &code_cl, NULL, worker_id);
       }
     }
   }
@@ -270,9 +270,9 @@
       ShenandoahTraversalSATBThreadsClosure tc(&satb_cl);
       if (unload_classes) {
         ShenandoahRemarkCLDClosure weak_cld_cl(&roots_cl);
-        _rp->process_strong_roots(&roots_cl, process_refs ? NULL : &roots_cl, &cld_cl, &weak_cld_cl, NULL, &tc, worker_id);
+        _rp->process_strong_roots(&roots_cl, &cld_cl, &weak_cld_cl, NULL, &tc, worker_id);
       } else {
-        _rp->process_all_roots(&roots_cl, process_refs ? NULL : &roots_cl, &cld_cl, NULL, &tc, worker_id);
+        _rp->process_all_roots(&roots_cl, &cld_cl, NULL, &tc, worker_id);
       }
     } else {
       ShenandoahTraversalDegenClosure roots_cl(q, rp);
@@ -280,9 +280,9 @@
       ShenandoahTraversalSATBThreadsClosure tc(&satb_cl);
       if (unload_classes) {
         ShenandoahRemarkCLDClosure weak_cld_cl(&roots_cl);
-        _rp->process_strong_roots(&roots_cl, process_refs ? NULL : &roots_cl, &cld_cl, &weak_cld_cl, NULL, &tc, worker_id);
+        _rp->process_strong_roots(&roots_cl, &cld_cl, &weak_cld_cl, NULL, &tc, worker_id);
       } else {
-        _rp->process_all_roots(&roots_cl, process_refs ? NULL : &roots_cl, &cld_cl, NULL, &tc, worker_id);
+        _rp->process_all_roots(&roots_cl, &cld_cl, NULL, &tc, worker_id);
       }
     }
 
@@ -594,8 +594,11 @@
     weak_refs_work();
   }
 
-  if (!_heap->cancelled_gc() && _heap->unload_classes()) {
-    _heap->unload_classes_and_cleanup_tables(false);
+  if (!_heap->cancelled_gc()) {
+    if (_heap->unload_classes()) {
+      _heap->unload_classes_and_cleanup_tables(false);
+    }
+
     fixup_roots();
   }
 
@@ -698,7 +701,7 @@
     ShenandoahTraversalFixRootsClosure cl;
     MarkingCodeBlobClosure blobsCl(&cl, CodeBlobToOopClosure::FixRelocations);
     CLDToOopClosure cldCl(&cl, ClassLoaderData::_claim_strong);
-    _rp->process_all_roots(&cl, &cl, &cldCl, &blobsCl, NULL, worker_id);
+    _rp->traversal_update_all_roots(&cl, &cldCl, &blobsCl, NULL, worker_id);
   }
 };