RFR: 8236981: Remove ShenandoahTraversalUpdateRefsClosure

[Aditya Mandaleeka]

Hi all,

This change removes the ShenandoahTraversalUpdateRefsClosure and replaces its use in traversal mode
STW weak root processing with the regular ShenandoahUpdateRefsClosure.

The only non self-explanatory part of this change is the ordering swap of fixup_roots and
parallel_cleaning in ShenandoahTraversalGC::final_traversal_collection. This was necessary to
address the case where a previous traversal GC which ran out of memory could leave objects forwarded
with their cset bits dropped (and therefore those forwarded objects would not get resolved in the
normal update closure which only updates objects in the cset). Moving fixup_roots before
parallel_cleaning solves this because fixup_roots will use the ShenandoahTraversalFixRootsClosure to
update all the roots.

Thanks once again to Roman for his support. I verified that hotspot_gc_shenandoah (which includes
traversal mode tests) is clean with this change.

Bug: https://bugs.openjdk.java.net/browse/JDK-8236981
Patch: As I don't yet have access to cr.openjdk.java.net, I am pasting the patch below.

Thanks,
Aditya Mandaleeka

======
diff -r 92cf8efd381d src/hotspot/share/gc/shenandoah/shenandoahClosures.hpp
--- a/src/hotspot/share/gc/shenandoah/shenandoahClosures.hpp    Fri Mar 06 10:27:24 2020 +0530
+++ b/src/hotspot/share/gc/shenandoah/shenandoahClosures.hpp    Thu Mar 05 22:30:55 2020 -0800
@@ -68,20 +68,6 @@
   inline void do_oop_work(T* p);
 };

-class ShenandoahTraversalUpdateRefsClosure: public OopClosure {
-private:
-  ShenandoahHeap* const           _heap;
-  ShenandoahHeapRegionSet* const  _traversal_set;
-
-public:
-  inline ShenandoahTraversalUpdateRefsClosure();
-  inline void do_oop(oop* p);
-  inline void do_oop(narrowOop* p);
-private:
-  template <class T>
-  inline void do_oop_work(T* p);
-};
-
 template <DecoratorSet MO = MO_UNORDERED>
 class ShenandoahEvacuateUpdateRootsClosure: public BasicOopIterateClosure {
 private:
diff -r 92cf8efd381d src/hotspot/share/gc/shenandoah/shenandoahClosures.inline.hpp
--- a/src/hotspot/share/gc/shenandoah/shenandoahClosures.inline.hpp     Fri Mar 06 10:27:24 2020 +0530
+++ b/src/hotspot/share/gc/shenandoah/shenandoahClosures.inline.hpp     Thu Mar 05 22:30:55 2020 -0800
@@ -83,29 +83,6 @@
 void ShenandoahUpdateRefsClosure::do_oop(oop* p)       { do_oop_work(p); }
 void ShenandoahUpdateRefsClosure::do_oop(narrowOop* p) { do_oop_work(p); }

-ShenandoahTraversalUpdateRefsClosure::ShenandoahTraversalUpdateRefsClosure() :
-  _heap(ShenandoahHeap::heap()),
-  _traversal_set(ShenandoahHeap::heap()->traversal_gc()->traversal_set()) {
-  assert(_heap->is_traversal_mode(), "Why we here?");
-}
-
-template <class T>
-void ShenandoahTraversalUpdateRefsClosure::do_oop_work(T* p) {
-  T o = RawAccess<>::oop_load(p);
-  if (!CompressedOops::is_null(o)) {
-    oop obj = CompressedOops::decode_not_null(o);
-    if (_heap->in_collection_set(obj) || _traversal_set->is_in(obj)) {
-      obj = ShenandoahBarrierSet::resolve_forwarded_not_null(obj);
-      RawAccess<IS_NOT_NULL>::oop_store(p, obj);
-    } else {
-      shenandoah_assert_not_forwarded(p, obj);
-    }
-  }
-}
-
-void ShenandoahTraversalUpdateRefsClosure::do_oop(oop* p)       { do_oop_work(p); }
-void ShenandoahTraversalUpdateRefsClosure::do_oop(narrowOop* p) { do_oop_work(p); }
-
 template <DecoratorSet MO>
 ShenandoahEvacuateUpdateRootsClosure<MO>::ShenandoahEvacuateUpdateRootsClosure() :
   _heap(ShenandoahHeap::heap()), _thread(Thread::current()) {
diff -r 92cf8efd381d src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp
--- a/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp        Fri Mar 06 10:27:24 2020 +0530
+++ b/src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp        Thu Mar 05 22:30:55 2020 -0800
@@ -2180,19 +2180,11 @@
   ShenandoahGCPhase phase(timing_phase);
   phase_timings()->record_workers_start(timing_phase);
   if (has_forwarded_objects()) {
-    if (is_traversal_mode()) {
-      ShenandoahForwardedIsAliveClosure is_alive;
-      ShenandoahTraversalUpdateRefsClosure keep_alive;
-      ShenandoahParallelWeakRootsCleaningTask<ShenandoahForwardedIsAliveClosure, ShenandoahTraversalUpdateRefsClosure>
-        cleaning_task(&is_alive, &keep_alive, num_workers, !ShenandoahConcurrentRoots::should_do_concurrent_class_unloading());
-      _workers->run_task(&cleaning_task);
-    } else {
-      ShenandoahForwardedIsAliveClosure is_alive;
-      ShenandoahUpdateRefsClosure keep_alive;
-      ShenandoahParallelWeakRootsCleaningTask<ShenandoahForwardedIsAliveClosure, ShenandoahUpdateRefsClosure>
-        cleaning_task(&is_alive, &keep_alive, num_workers, !ShenandoahConcurrentRoots::should_do_concurrent_class_unloading());
-      _workers->run_task(&cleaning_task);
-    }
+    ShenandoahForwardedIsAliveClosure is_alive;
+    ShenandoahUpdateRefsClosure keep_alive;
+    ShenandoahParallelWeakRootsCleaningTask<ShenandoahForwardedIsAliveClosure, ShenandoahUpdateRefsClosure>
+      cleaning_task(&is_alive, &keep_alive, num_workers, !ShenandoahConcurrentRoots::should_do_concurrent_class_unloading());
+    _workers->run_task(&cleaning_task);
   } else {
     ShenandoahIsAliveClosure is_alive;
 #ifdef ASSERT
diff -r 92cf8efd381d src/hotspot/share/gc/shenandoah/shenandoahTraversalGC.cpp
--- a/src/hotspot/share/gc/shenandoah/shenandoahTraversalGC.cpp Fri Mar 06 10:27:24 2020 +0530
+++ b/src/hotspot/share/gc/shenandoah/shenandoahTraversalGC.cpp Thu Mar 05 22:30:55 2020 -0800
@@ -605,8 +605,11 @@
     // that results the TLAB/GCLAB not usable. Retire them here.
     _heap->make_parsable(true);

+    // Do this fixup before the call to parallel_cleaning to ensure that all
+    // forwarded objects (including those that are no longer in the cset) are
+    // updated by the time we do weak root processing.
+    fixup_roots();
     _heap->parallel_cleaning(false);
-    fixup_roots();

     _heap->set_has_forwarded_objects(false);