8204613: StringTable: Calculates wrong number of uncleaned items

Reviewed-by: pliden, coleenp

src/hotspot/share/classfile/stringTable.cpp

@@ -198,17 +198,16 @@ size_t StringTable::item_added() {
   return Atomic::add((size_t)1, &(the_table()->_items));
 }
 
-size_t StringTable::items_to_clean(size_t ncl) {
-  size_t total = Atomic::add((size_t)ncl, &(the_table()->_uncleaned_items));
+size_t StringTable::add_items_to_clean(size_t ndead) {
+  size_t total = Atomic::add((size_t)ndead, &(the_table()->_uncleaned_items));
   log_trace(stringtable)(
      "Uncleaned items:" SIZE_FORMAT " added: " SIZE_FORMAT " total:" SIZE_FORMAT,
-     the_table()->_uncleaned_items, ncl, total);
+     the_table()->_uncleaned_items, ndead, total);
   return total;
 }
 
 void StringTable::item_removed() {
   Atomic::add((size_t)-1, &(the_table()->_items));
-  Atomic::add((size_t)-1, &(the_table()->_uncleaned_items));
 }
 
 double StringTable::get_load_factor() {
@@ -405,8 +404,11 @@ void StringTable::unlink_or_oops_do(BoolObjectClosure* is_alive, OopClosure* f,
 
   StringTable::the_table()->_weak_handles->weak_oops_do(&stiac, tmp);
 
-  StringTable::the_table()->items_to_clean(stiac._count);
+  // This is the serial case without ParState.
+  // Just set the correct number and check for a cleaning phase.
+  the_table()->_uncleaned_items = stiac._count;
   StringTable::the_table()->check_concurrent_work();
+
   if (processed != NULL) {
     *processed = (int) stiac._count_total;
   }
@@ -430,8 +432,9 @@ void StringTable::possibly_parallel_unlink(
 
   _par_state_string->weak_oops_do(&stiac, &dnc);
 
-  StringTable::the_table()->items_to_clean(stiac._count);
-  StringTable::the_table()->check_concurrent_work();
+  // Accumulate the dead strings.
+  the_table()->add_items_to_clean(stiac._count);
+
   *processed = (int) stiac._count_total;
   *removed = (int) stiac._count;
 }
@@ -467,10 +470,8 @@ void StringTable::grow(JavaThread* jt) {
 }
 
 struct StringTableDoDelete : StackObj {
-  long _count;
-  StringTableDoDelete() : _count(0) {}
   void operator()(WeakHandle<vm_string_table_data>* val) {
-    ++_count;
+    /* do nothing */
   }
 };
 
@@ -524,6 +525,7 @@ void StringTable::check_concurrent_work() {
   if (_has_work) {
     return;
   }
+
   double load_factor = StringTable::get_load_factor();
   double dead_factor = StringTable::get_dead_factor();
   // We should clean/resize if we have more dead than alive,

src/hotspot/share/classfile/stringTable.hpp

@@ -83,7 +83,7 @@ private:
 
   static uintx item_added();
   static void item_removed();
-  static size_t items_to_clean(size_t ncl);
+  size_t add_items_to_clean(size_t ndead);
 
   StringTable();
 
@@ -113,6 +113,23 @@ private:
   static bool has_work() { return the_table()->_has_work; }
 
   // GC support
+
+  // Must be called before a parallel walk where strings might die.
+  static void reset_dead_counter() {
+    the_table()->_uncleaned_items = 0;
+  }
+  // After the parallel walk this method must be called to trigger
+  // cleaning. Note it might trigger a resize instead.
+  static void finish_dead_counter() {
+    the_table()->check_concurrent_work();
+  }
+
+  // If GC uses ParState directly it should add the number of cleared
+  // strings to this method.
+  static void inc_dead_counter(size_t ndead) {
+    the_table()->add_items_to_clean(ndead);
+  }
+
   //   Delete pointers to otherwise-unreachable objects.
   static void unlink(BoolObjectClosure* cl) {
     unlink_or_oops_do(cl);
@@ -150,9 +167,9 @@ private:
   oop lookup_shared(jchar* name, int len, unsigned int hash) NOT_CDS_JAVA_HEAP_RETURN_(NULL);
   static void copy_shared_string_table(CompactStringTableWriter* ch_table) NOT_CDS_JAVA_HEAP_RETURN;
  public:
-  static oop create_archived_string(oop s, Thread* THREAD);
+  static oop create_archived_string(oop s, Thread* THREAD) NOT_CDS_JAVA_HEAP_RETURN_(NULL);
   static void set_shared_string_mapped() { _shared_string_mapped = true; }
-  static bool shared_string_mapped()       { return _shared_string_mapped; }
+  static bool shared_string_mapped()     { return _shared_string_mapped; }
   static void shared_oops_do(OopClosure* f) NOT_CDS_JAVA_HEAP_RETURN;
   static void write_to_archive() NOT_CDS_JAVA_HEAP_RETURN;
   static void serialize(SerializeClosure* soc) NOT_CDS_JAVA_HEAP_RETURN;

src/hotspot/share/gc/g1/g1CollectedHeap.cpp

@@ -3262,6 +3262,9 @@ public:
     if (process_symbols) {
       SymbolTable::clear_parallel_claimed_index();
     }
+    if (process_strings) {
+      StringTable::reset_dead_counter();
+    }
   }
 
   ~G1StringAndSymbolCleaningTask() {
@@ -3275,6 +3278,9 @@ public:
         "symbols: " SIZE_FORMAT " processed, " SIZE_FORMAT " removed",
         strings_processed(), strings_removed(),
         symbols_processed(), symbols_removed());
+    if (_process_strings) {
+      StringTable::finish_dead_counter();
+    }
   }
 
   void work(uint worker_id) {

src/hotspot/share/gc/z/zRootsIterator.cpp

@@ -307,10 +307,12 @@ ZWeakRootsIterator::ZWeakRootsIterator() :
   assert(SafepointSynchronize::is_at_safepoint(), "Should be at safepoint");
   ZStatTimer timer(ZSubPhasePauseWeakRootsSetup);
   SymbolTable::clear_parallel_claimed_index();
+  StringTable::reset_dead_counter();
 }
 
 ZWeakRootsIterator::~ZWeakRootsIterator() {
   ZStatTimer timer(ZSubPhasePauseWeakRootsTeardown);
+  StringTable::finish_dead_counter();
 }
 
 void ZWeakRootsIterator::do_vm_weak_handles(BoolObjectClosure* is_alive, OopClosure* cl) {
@@ -341,9 +343,34 @@ void ZWeakRootsIterator::do_symbol_table(BoolObjectClosure* is_alive, OopClosure
   SymbolTable::possibly_parallel_unlink(&dummy, &dummy);
 }
 
+class ZStringTableDeadCounterBoolObjectClosure : public BoolObjectClosure  {
+private:
+  BoolObjectClosure* const _cl;
+  size_t                   _ndead;
+
+public:
+  ZStringTableDeadCounterBoolObjectClosure(BoolObjectClosure* cl) :
+      _cl(cl),
+      _ndead(0) {}
+
+  ~ZStringTableDeadCounterBoolObjectClosure() {
+    StringTable::inc_dead_counter(_ndead);
+  }
+
+  virtual bool do_object_b(oop obj) {
+    if (_cl->do_object_b(obj)) {
+      return true;
+    }
+
+    _ndead++;
+    return false;
+  }
+};
+
 void ZWeakRootsIterator::do_string_table(BoolObjectClosure* is_alive, OopClosure* cl) {
   ZStatTimer timer(ZSubPhasePauseWeakRootsStringTable);
-  _string_table_iter.weak_oops_do(is_alive, cl);
+  ZStringTableDeadCounterBoolObjectClosure counter_is_alive(is_alive);
+  _string_table_iter.weak_oops_do(&counter_is_alive, cl);
 }
 
 void ZWeakRootsIterator::weak_oops_do(BoolObjectClosure* is_alive, OopClosure* cl) {
@@ -377,7 +404,13 @@ ZConcurrentWeakRootsIterator::ZConcurrentWeakRootsIterator() :
     _string_table_iter(StringTable::weak_storage()),
     _vm_weak_handles(this),
     _jni_weak_handles(this),
-    _string_table(this) {}
+    _string_table(this) {
+  StringTable::reset_dead_counter();
+}
+
+ZConcurrentWeakRootsIterator::~ZConcurrentWeakRootsIterator() {
+  StringTable::finish_dead_counter();
+}
 
 void ZConcurrentWeakRootsIterator::do_vm_weak_handles(OopClosure* cl) {
   ZStatTimer timer(ZSubPhaseConcurrentWeakRootsVMWeakHandles);
@@ -389,9 +422,36 @@ void ZConcurrentWeakRootsIterator::do_jni_weak_handles(OopClosure* cl) {
   _jni_weak_handles_iter.oops_do(cl);
 }
 
+class ZStringTableDeadCounterOopClosure : public OopClosure  {
+private:
+  OopClosure* const _cl;
+  size_t            _ndead;
+
+public:
+  ZStringTableDeadCounterOopClosure(OopClosure* cl) :
+      _cl(cl),
+      _ndead(0) {}
+
+  ~ZStringTableDeadCounterOopClosure() {
+    StringTable::inc_dead_counter(_ndead);
+  }
+
+  virtual void do_oop(oop* p) {
+    _cl->do_oop(p);
+    if (*p == NULL) {
+      _ndead++;
+    }
+  }
+
+  virtual void do_oop(narrowOop* p) {
+    ShouldNotReachHere();
+  }
+};
+
 void ZConcurrentWeakRootsIterator::do_string_table(OopClosure* cl) {
   ZStatTimer timer(ZSubPhaseConcurrentWeakRootsStringTable);
-  _string_table_iter.oops_do(cl);
+  ZStringTableDeadCounterOopClosure counter_cl(cl);
+  _string_table_iter.oops_do(&counter_cl);
 }
 
 void ZConcurrentWeakRootsIterator::oops_do(OopClosure* cl) {

src/hotspot/share/gc/z/zRootsIterator.hpp

@@ -164,6 +164,7 @@ private:
 
 public:
   ZConcurrentWeakRootsIterator();
+  ~ZConcurrentWeakRootsIterator();
 
   void oops_do(OopClosure* cl);
 };