8267257: Shenandoah: Always deduplicate strings when it is enabled du…

…ring full gc

Reviewed-by: rkennke

src/hotspot/share/gc/shenandoah/shenandoahConcurrentMark.cpp

@@ -87,8 +87,8 @@ class ShenandoahConcurrentMarkingTask : public AbstractGangTask {
     ShenandoahReferenceProcessor* rp = heap->ref_processor();
     assert(rp != NULL, "need reference processor");
     _cm->mark_loop(worker_id, _terminator, rp,
-                   true, // cancellable
-                   ShenandoahStringDedup::is_enabled()); // perform string dedup
+                   true /*cancellable*/,
+                   ShenandoahStringDedup::is_enabled() ? ENQUEUE_DEDUP : NO_DEDUP);
   }
 };
 
@@ -149,11 +149,9 @@ class ShenandoahFinalMarkingTask : public AbstractGangTask {
                                                ShenandoahIUBarrier ? &mark_cl : NULL);
       Threads::threads_do(&tc);
     }
-
     _cm->mark_loop(worker_id, _terminator, rp,
-                   false, // not cancellable
-                   _dedup_string);
-
+                   false /*not cancellable*/,
+                   _dedup_string ? ENQUEUE_DEDUP : NO_DEDUP);
     assert(_cm->task_queues()->is_empty(), "Should be empty");
   }
 };

src/hotspot/share/gc/shenandoah/shenandoahMark.cpp

@@ -55,9 +55,8 @@ void ShenandoahMark::clear() {
   ShenandoahBarrierSet::satb_mark_queue_set().abandon_partial_marking();
 }
 
-template <bool CANCELLABLE>
-void ShenandoahMark::mark_loop_prework(uint w, TaskTerminator *t, ShenandoahReferenceProcessor *rp,
-                                       bool strdedup) {
+template <bool CANCELLABLE, StringDedupMode STRING_DEDUP>
+void ShenandoahMark::mark_loop_prework(uint w, TaskTerminator *t, ShenandoahReferenceProcessor *rp) {
   ShenandoahObjToScanQueue* q = get_queue(w);
 
   ShenandoahHeap* const heap = ShenandoahHeap::heap();
@@ -67,53 +66,58 @@ void ShenandoahMark::mark_loop_prework(uint w, TaskTerminator *t, ShenandoahRefe
   // play nice with specialized_oop_iterators.
   if (heap->unload_classes()) {
     if (heap->has_forwarded_objects()) {
-      if (strdedup) {
-        using Closure = ShenandoahMarkUpdateRefsMetadataClosure<ENQUEUE_DEDUP>;
-        Closure cl(q, rp);
-        mark_loop_work<Closure, CANCELLABLE>(&cl, ld, w, t);
-      } else {
-        using Closure = ShenandoahMarkUpdateRefsMetadataClosure<NO_DEDUP>;
-        Closure cl(q, rp);
-        mark_loop_work<Closure, CANCELLABLE>(&cl, ld, w, t);
-      }
+      using Closure = ShenandoahMarkUpdateRefsMetadataClosure<STRING_DEDUP>;
+      Closure cl(q, rp);
+      mark_loop_work<Closure, CANCELLABLE>(&cl, ld, w, t);
     } else {
-      if (strdedup) {
-        using Closure = ShenandoahMarkRefsMetadataClosure<ENQUEUE_DEDUP>;
-        Closure cl(q, rp);
-        mark_loop_work<Closure, CANCELLABLE>(&cl, ld, w, t);
-      } else {
-        using Closure = ShenandoahMarkRefsMetadataClosure<NO_DEDUP>;
-        Closure cl(q, rp);
-        mark_loop_work<Closure, CANCELLABLE>(&cl, ld, w, t);
-      }
+      using Closure = ShenandoahMarkRefsMetadataClosure<STRING_DEDUP>;
+      Closure cl(q, rp);
+      mark_loop_work<Closure, CANCELLABLE>(&cl, ld, w, t);
     }
   } else {
     if (heap->has_forwarded_objects()) {
-      if (strdedup) {
-        using Closure = ShenandoahMarkUpdateRefsClosure<ENQUEUE_DEDUP>;
-        Closure cl(q, rp);
-        mark_loop_work<Closure, CANCELLABLE>(&cl, ld, w, t);
-      } else {
-        using Closure = ShenandoahMarkUpdateRefsClosure<NO_DEDUP>;
-        Closure cl(q, rp);
-        mark_loop_work<Closure, CANCELLABLE>(&cl, ld, w, t);
-      }
+      using Closure = ShenandoahMarkUpdateRefsClosure<STRING_DEDUP>;
+      Closure cl(q, rp);
+      mark_loop_work<Closure, CANCELLABLE>(&cl, ld, w, t);
     } else {
-      if (strdedup) {
-        using Closure = ShenandoahMarkRefsClosure<ENQUEUE_DEDUP>;
-        Closure cl(q, rp);
-        mark_loop_work<Closure, CANCELLABLE>(&cl, ld, w, t);
-      } else {
-        using Closure = ShenandoahMarkRefsClosure<NO_DEDUP>;
-        Closure cl(q, rp);
-        mark_loop_work<Closure, CANCELLABLE>(&cl, ld, w, t);
-      }
+      using Closure = ShenandoahMarkRefsClosure<STRING_DEDUP>;
+      Closure cl(q, rp);
+      mark_loop_work<Closure, CANCELLABLE>(&cl, ld, w, t);
     }
   }
 
   heap->flush_liveness_cache(w);
 }
 
+void ShenandoahMark::mark_loop(uint worker_id, TaskTerminator* terminator, ShenandoahReferenceProcessor *rp,
+               bool cancellable, StringDedupMode dedup_mode) {
+  if (cancellable) {
+    switch(dedup_mode) {
+      case NO_DEDUP:
+        mark_loop_prework<true, NO_DEDUP>(worker_id, terminator, rp);
+        break;
+      case ENQUEUE_DEDUP:
+        mark_loop_prework<true, ENQUEUE_DEDUP>(worker_id, terminator, rp);
+        break;
+      case ALWAYS_DEDUP:
+        mark_loop_prework<true, ALWAYS_DEDUP>(worker_id, terminator, rp);
+        break;
+    }
+  } else {
+    switch(dedup_mode) {
+      case NO_DEDUP:
+        mark_loop_prework<false, NO_DEDUP>(worker_id, terminator, rp);
+        break;
+      case ENQUEUE_DEDUP:
+        mark_loop_prework<false, ENQUEUE_DEDUP>(worker_id, terminator, rp);
+        break;
+      case ALWAYS_DEDUP:
+        mark_loop_prework<false, ALWAYS_DEDUP>(worker_id, terminator, rp);
+        break;
+    }
+  }
+}
+
 template <class T, bool CANCELLABLE>
 void ShenandoahMark::mark_loop_work(T* cl, ShenandoahLiveData* live_data, uint worker_id, TaskTerminator *terminator) {
   uintx stride = ShenandoahMarkLoopStride;
@@ -188,12 +192,3 @@ void ShenandoahMark::mark_loop_work(T* cl, ShenandoahLiveData* live_data, uint w
     }
   }
 }
-
-void ShenandoahMark::mark_loop(uint worker_id, TaskTerminator* terminator, ShenandoahReferenceProcessor *rp,
-               bool cancellable, bool strdedup) {
-  if (cancellable) {
-    mark_loop_prework<true>(worker_id, terminator, rp, strdedup);
-  } else {
-    mark_loop_prework<false>(worker_id, terminator, rp, strdedup);
-  }
-}

src/hotspot/share/gc/shenandoah/shenandoahMark.hpp

@@ -70,12 +70,12 @@ class ShenandoahMark: public StackObj {
   template <class T, bool CANCELLABLE>
   void mark_loop_work(T* cl, ShenandoahLiveData* live_data, uint worker_id, TaskTerminator *t);
 
-  template <bool CANCELLABLE>
-  void mark_loop_prework(uint worker_id, TaskTerminator *terminator, ShenandoahReferenceProcessor *rp, bool strdedup);
+  template <bool CANCELLABLE, StringDedupMode STRING_DEDUP>
+  void mark_loop_prework(uint worker_id, TaskTerminator *terminator, ShenandoahReferenceProcessor *rp);
 
 protected:
   void mark_loop(uint worker_id, TaskTerminator* terminator, ShenandoahReferenceProcessor *rp,
-                 bool cancellable, bool strdedup);
+                 bool cancellable, StringDedupMode dedup_mode);
 };
 
 #endif // SHARE_GC_SHENANDOAH_SHENANDOAHMARK_HPP

src/hotspot/share/gc/shenandoah/shenandoahMark.inline.hpp

@@ -260,6 +260,9 @@ inline void ShenandoahMark::mark_through_ref(T* p, ShenandoahObjToScanQueue* q,
       if ((STRING_DEDUP == ENQUEUE_DEDUP) && ShenandoahStringDedup::is_candidate(obj)) {
         assert(ShenandoahStringDedup::is_enabled(), "Must be enabled");
         req->add(obj);
+      } else if ((STRING_DEDUP == ALWAYS_DEDUP) && ShenandoahStringDedup::is_string_candidate(obj)) {
+        assert(ShenandoahStringDedup::is_enabled(), "Must be enabled");
+        req->add(obj);
       }
     }
 

src/hotspot/share/gc/shenandoah/shenandoahOopClosures.hpp

@@ -34,7 +34,8 @@
 
 enum StringDedupMode {
   NO_DEDUP,      // Do not do anything for String deduplication
-  ENQUEUE_DEDUP  // Enqueue candidate Strings for deduplication
+  ENQUEUE_DEDUP, // Enqueue candidate Strings for deduplication, if meet age threshold
+  ALWAYS_DEDUP   // Enqueue Strings for deduplication
 };
 
 class ShenandoahMarkRefsSuperClosure : public MetadataVisitingOopIterateClosure {

src/hotspot/share/gc/shenandoah/shenandoahSTWMark.cpp

@@ -133,7 +133,7 @@ void ShenandoahSTWMark::finish_mark(uint worker_id) {
   ShenandoahReferenceProcessor* rp = ShenandoahHeap::heap()->ref_processor();
 
   mark_loop(worker_id, &_terminator, rp,
-            false, // not cancellable
-            ShenandoahStringDedup::is_enabled());
+            false /* not cancellable */,
+            ShenandoahStringDedup::is_enabled() ? ALWAYS_DEDUP : NO_DEDUP);
 }
 

src/hotspot/share/gc/shenandoah/shenandoahStringDedup.hpp

@@ -29,6 +29,7 @@
 
 class ShenandoahStringDedup : public StringDedup {
 public:
+  static inline bool is_string_candidate(oop obj);
   static inline bool is_candidate(oop obj);
 };
 

src/hotspot/share/gc/shenandoah/shenandoahStringDedup.inline.hpp

@@ -28,13 +28,18 @@
 #include "classfile/javaClasses.inline.hpp"
 #include "gc/shenandoah/shenandoahStringDedup.hpp"
 
-bool ShenandoahStringDedup::is_candidate(oop obj) {
+bool ShenandoahStringDedup::is_string_candidate(oop obj) {
   assert(Thread::current()->is_Worker_thread(),
         "Only from a GC worker thread");
-  if (!java_lang_String::is_instance_inlined(obj) ||
-       java_lang_String::value(obj) == nullptr) {
+  return java_lang_String::is_instance_inlined(obj) &&
+         java_lang_String::value(obj) != nullptr;
+}
+
+bool ShenandoahStringDedup::is_candidate(oop obj) {
+  if (!is_string_candidate(obj)) {
     return false;
   }
+
   if (StringDedup::is_below_threshold_age(obj->age())) {
     const markWord mark = obj->mark();
     // Having/had displaced header, too risk to deal with them, skip

test/hotspot/jtreg/gc/shenandoah/TestStringDedup.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2017, 2018, Red Hat, Inc. All rights reserved.
+ * Copyright (c) 2017, 2021, Red Hat, Inc. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -34,12 +34,12 @@
  *
  * @run main/othervm -Xmx256m -Xlog:gc+stats -XX:+UnlockDiagnosticVMOptions -XX:+UnlockExperimentalVMOptions -XX:+UseStringDeduplication
  *      -XX:+UseShenandoahGC -XX:ShenandoahGCMode=passive
- *      -XX:+ShenandoahDegeneratedGC
+ *      -XX:+ShenandoahDegeneratedGC -DGCCount=1
  *      TestStringDedup
  *
  * @run main/othervm -Xmx256m -Xlog:gc+stats -XX:+UnlockDiagnosticVMOptions -XX:+UnlockExperimentalVMOptions -XX:+UseStringDeduplication
  *      -XX:+UseShenandoahGC -XX:ShenandoahGCMode=passive
- *      -XX:-ShenandoahDegeneratedGC
+ *      -XX:-ShenandoahDegeneratedGC -DGCCount=1
  *      TestStringDedup
  */
 
@@ -54,15 +54,15 @@
  *          java.management
  *
  * @run main/othervm -Xmx256m -Xlog:gc+stats -XX:+UnlockDiagnosticVMOptions -XX:+UnlockExperimentalVMOptions -XX:+UseStringDeduplication
- *      -XX:+UseShenandoahGC -XX:ShenandoahGCHeuristics=aggressive
+ *      -XX:+UseShenandoahGC -XX:ShenandoahGCHeuristics=aggressive -XX:StringDeduplicationAgeThreshold=3
  *      TestStringDedup
  *
  * @run main/othervm -Xmx256m -Xlog:gc+stats -XX:+UnlockDiagnosticVMOptions -XX:+UnlockExperimentalVMOptions -XX:+UseStringDeduplication
- *      -XX:+UseShenandoahGC
+ *      -XX:+UseShenandoahGC -XX:StringDeduplicationAgeThreshold=3
  *      TestStringDedup
  *
  * @run main/othervm -Xmx256m -Xlog:gc+stats -XX:+UnlockDiagnosticVMOptions -XX:+UnlockExperimentalVMOptions -XX:+UseStringDeduplication
- *      -XX:+UseShenandoahGC -XX:ShenandoahGCHeuristics=compact
+ *      -XX:+UseShenandoahGC -XX:ShenandoahGCHeuristics=compact -XX:StringDeduplicationAgeThreshold=3
  *      TestStringDedup
  */
 
@@ -77,11 +77,11 @@
  *          java.management
  *
  * @run main/othervm -Xmx256m -Xlog:gc+stats -XX:+UnlockDiagnosticVMOptions -XX:+UnlockExperimentalVMOptions -XX:+UseStringDeduplication
- *      -XX:+UseShenandoahGC -XX:ShenandoahGCMode=iu
+ *      -XX:+UseShenandoahGC -XX:ShenandoahGCMode=iu -XX:StringDeduplicationAgeThreshold=3
  *      TestStringDedup
  *
  * @run main/othervm -Xmx256m -Xlog:gc+stats -XX:+UnlockDiagnosticVMOptions -XX:+UnlockExperimentalVMOptions -XX:+UseStringDeduplication
- *      -XX:+UseShenandoahGC -XX:ShenandoahGCMode=iu -XX:ShenandoahGCHeuristics=aggressive
+ *      -XX:+UseShenandoahGC -XX:ShenandoahGCMode=iu -XX:ShenandoahGCHeuristics=aggressive -XX:StringDeduplicationAgeThreshold=3
  *      TestStringDedup
  */
 
@@ -96,6 +96,8 @@ public class TestStringDedup {
     private static Unsafe unsafe;
 
     private static final int UniqueStrings = 20;
+    // How many GC cycles are needed to complete deduplication.
+    private static final int GCCount = Integer.getInteger("GCCount", 3);
 
     static {
         try {
@@ -168,23 +170,31 @@ private static int verifyDedepString(ArrayList<StringAndId> strs) {
                 }
             }
         }
-        System.out.println("Dedup: " + dedup + "/" + total + " unique: " + (total - dedup));
         return (total - dedup);
     }
 
     public static void main(String[] args) {
         ArrayList<StringAndId> astrs = new ArrayList<>();
         generateStrings(astrs, UniqueStrings);
-        System.gc();
-        System.gc();
-        System.gc();
-        System.gc();
-        System.gc();
-
-        if (verifyDedepString(astrs) != UniqueStrings) {
-            // Can not guarantee all strings are deduplicated, there can
-            // still have pending items in queues.
-            System.out.println("Not all strings are deduplicated");
+        for (int count = 0; count < GCCount; count ++) {
+          System.gc();
         }
+
+        int unique_count = 0;
+        for (int waitCount = 0; waitCount < 3; waitCount ++) {
+            // Let concurrent string dedup thread to run
+            try {
+                Thread.sleep(1000);
+            } catch (InterruptedException e) {
+            }
+
+            // All deduplicated, done.
+            unique_count = verifyDedepString(astrs);
+            if ( unique_count == UniqueStrings) {
+                return;
+            }
+        }
+
+        throw new RuntimeException("Expecting " + UniqueStrings + " unique strings, but got " + unique_count);
     }
 }