# HG changeset patch
# User coleenp
# Date 1548948593 18000
# Node ID 739eaf4ac4adf9cad6dcb13ec68799802ead2a7c
# Parent  5bc1634bc0ca0165e0d18dc26989dcdc18e9759d
8213753: SymbolTable is double walked during class unloading and clean up table timing in do_unloading
Summary: remove gc timing for short runtime cleanup triggering; make symbol table cleaning triggered automatically on unloading
Reviewed-by: shade, stefank, gziemski

diff -r 5bc1634bc0ca -r 739eaf4ac4ad src/hotspot/share/classfile/stringTable.cpp
--- a/src/hotspot/share/classfile/stringTable.cpp	Thu Jan 31 10:18:41 2019 -0500
+++ b/src/hotspot/share/classfile/stringTable.cpp	Thu Jan 31 10:29:53 2019 -0500
@@ -468,7 +468,7 @@
   if ((dead_factor > load_factor) ||
       (load_factor > PREF_AVG_LIST_LEN) ||
       (dead_factor > CLEAN_DEAD_HIGH_WATER_MARK)) {
-    log_debug(stringtable)("Concurrent work triggered, live factor:%g dead factor:%g",
+    log_debug(stringtable)("Concurrent work triggered, live factor: %g dead factor: %g",
                            load_factor, dead_factor);
     trigger_concurrent_work();
   }
diff -r 5bc1634bc0ca -r 739eaf4ac4ad src/hotspot/share/classfile/symbolTable.cpp
--- a/src/hotspot/share/classfile/symbolTable.cpp	Thu Jan 31 10:18:41 2019 -0500
+++ b/src/hotspot/share/classfile/symbolTable.cpp	Thu Jan 31 10:29:53 2019 -0500
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1997, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2019, Oracle and/or its affiliates. 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
@@ -49,10 +49,6 @@
 const size_t END_SIZE = 17;
 // If a chain gets to 100 something might be wrong
 const size_t REHASH_LEN = 100;
-// We only get a chance to check whether we need
-// to clean infrequently (on class unloading),
-// so if we have even one dead entry then mark table for cleaning
-const double CLEAN_DEAD_HIGH_WATER_MARK = 0.0;
 
 const size_t ON_STACK_BUFFER_LENGTH = 128;
 
@@ -142,7 +138,7 @@
 SymbolTable::SymbolTable() :
   _symbols_removed(0), _symbols_counted(0), _local_table(NULL),
   _current_size(0), _has_work(0), _needs_rehashing(false),
-  _items_count(0), _uncleaned_items_count(0) {
+  _items_count(0), _has_items_to_clean(false) {
 
   size_t start_size_log_2 = ceil_log2(SymbolTableSize);
   _current_size = ((size_t)1) << start_size_log_2;
@@ -171,22 +167,14 @@
   }
 }
 
+void SymbolTable::reset_has_items_to_clean() { Atomic::store(false, &_has_items_to_clean); }
+void SymbolTable::mark_has_items_to_clean()  { Atomic::store(true, &_has_items_to_clean); }
+bool SymbolTable::has_items_to_clean() const { return Atomic::load(&_has_items_to_clean); }
+
 void SymbolTable::item_added() {
   Atomic::inc(&(SymbolTable::the_table()->_items_count));
 }
 
-void SymbolTable::set_item_clean_count(size_t ncl) {
-  Atomic::store(ncl, &(SymbolTable::the_table()->_uncleaned_items_count));
-  log_trace(symboltable)("Set uncleaned items:" SIZE_FORMAT, SymbolTable::the_table()->_uncleaned_items_count);
-}
-
-// Mark one item as needing to be cleaned, but only if no other items are marked yet
-void SymbolTable::mark_item_clean_count() {
-  if (Atomic::cmpxchg((size_t)1, &(SymbolTable::the_table()->_uncleaned_items_count), (size_t)0) == 0) {
-    log_trace(symboltable)("Marked uncleaned items:" SIZE_FORMAT, SymbolTable::the_table()->_uncleaned_items_count);
-  }
-}
-
 void SymbolTable::item_removed() {
   Atomic::inc(&(SymbolTable::the_table()->_symbols_removed));
   Atomic::dec(&(SymbolTable::the_table()->_items_count));
@@ -196,15 +184,11 @@
   return (double)_items_count/_current_size;
 }
 
-double SymbolTable::get_dead_factor() const {
-  return (double)_uncleaned_items_count/_current_size;
-}
-
 size_t SymbolTable::table_size() {
   return ((size_t)1) << _local_table->get_size_log2(Thread::current());
 }
 
-void SymbolTable::trigger_concurrent_work() {
+void SymbolTable::trigger_cleanup() {
   MutexLockerEx ml(Service_lock, Mutex::_no_safepoint_check_flag);
   SymbolTable::the_table()->_has_work = true;
   Service_lock->notify_all();
@@ -490,12 +474,7 @@
   update_needs_rehash(rehash_warning);
 
   if (clean_hint) {
-    // we just found out that there is a dead item,
-    // which we were unable to clean right now,
-    // but we have no way of telling whether it's
-    // been previously counted or not, so mark
-    // it only if no other items were found yet
-    mark_item_clean_count();
+    mark_has_items_to_clean();
     check_concurrent_work();
   }
 
@@ -697,7 +676,7 @@
       }
       bdt.cont(jt);
     }
-    SymbolTable::the_table()->set_item_clean_count(0);
+    SymbolTable::the_table()->reset_has_items_to_clean();
     bdt.done(jt);
   }
 
@@ -711,17 +690,13 @@
   if (_has_work) {
     return;
   }
-  double load_factor = SymbolTable::get_load_factor();
-  double dead_factor = SymbolTable::get_dead_factor();
-  // We should clean/resize if we have more dead than alive,
+  // We should clean/resize if we have
   // more items than preferred load factor or
   // more dead items than water mark.
-  if ((dead_factor > load_factor) ||
-      (load_factor > PREF_AVG_LIST_LEN) ||
-      (dead_factor > CLEAN_DEAD_HIGH_WATER_MARK)) {
-    log_debug(symboltable)("Concurrent work triggered, live factor:%f dead factor:%f",
-                           load_factor, dead_factor);
-    trigger_concurrent_work();
+  if (has_items_to_clean() || (get_load_factor() > PREF_AVG_LIST_LEN)) {
+    log_debug(symboltable)("Concurrent work triggered, load factor: %f, items to clean: %s",
+                           get_load_factor(), has_items_to_clean() ? "true" : "false");
+    trigger_cleanup();
   }
 }
 
@@ -737,34 +712,6 @@
   _has_work = false;
 }
 
-class CountDead : StackObj {
-  size_t _count;
-public:
-  CountDead() : _count(0) {}
-  bool operator()(Symbol** value) {
-    assert(value != NULL, "expected valid value");
-    assert(*value != NULL, "value should point to a symbol");
-    Symbol* sym = *value;
-    if (sym->refcount() == 0) {
-      _count++;
-    }
-    return true;
-  };
-  size_t get_dead_count() const {
-    return _count;
-  }
-};
-
-void SymbolTable::do_check_concurrent_work() {
-  CountDead counter;
-  if (!SymbolTable::the_table()->_local_table->try_scan(Thread::current(), counter)) {
-    log_info(symboltable)("count dead unavailable at this moment");
-  } else {
-    SymbolTable::the_table()->set_item_clean_count(counter.get_dead_count());
-    SymbolTable::the_table()->check_concurrent_work();
-  }
-}
-
 void SymbolTable::do_concurrent_work(JavaThread* jt) {
   SymbolTable::the_table()->concurrent_work(jt);
 }
@@ -800,7 +747,7 @@
   if (get_load_factor() > PREF_AVG_LIST_LEN &&
       !_local_table->is_max_size_reached()) {
     log_debug(symboltable)("Choosing growing over rehashing.");
-    trigger_concurrent_work();
+    trigger_cleanup();
     _needs_rehashing = false;
     return;
   }
@@ -808,7 +755,7 @@
   // Already rehashed.
   if (rehashed) {
     log_warning(symboltable)("Rehashing already done, still long lists.");
-    trigger_concurrent_work();
+    trigger_cleanup();
     _needs_rehashing = false;
     return;
   }
diff -r 5bc1634bc0ca -r 739eaf4ac4ad src/hotspot/share/classfile/symbolTable.hpp
--- a/src/hotspot/share/classfile/symbolTable.hpp	Thu Jan 31 10:18:41 2019 -0500
+++ b/src/hotspot/share/classfile/symbolTable.hpp	Thu Jan 31 10:29:53 2019 -0500
@@ -123,18 +123,19 @@
   volatile bool _needs_rehashing;
 
   volatile size_t _items_count;
-  volatile size_t _uncleaned_items_count;
+  volatile bool   _has_items_to_clean;
 
   double get_load_factor() const;
-  double get_dead_factor() const;
 
   void check_concurrent_work();
-  void trigger_concurrent_work();
 
   static void item_added();
   static void item_removed();
-  static void set_item_clean_count(size_t ncl);
-  static void mark_item_clean_count();
+
+  // For cleaning
+  void reset_has_items_to_clean();
+  void mark_has_items_to_clean();
+  bool has_items_to_clean() const;
 
   SymbolTable();
 
@@ -190,12 +191,9 @@
     initialize_symbols(symbol_alloc_arena_size);
   }
 
-  static void unlink() {
-    do_check_concurrent_work();
-  }
-  static void do_check_concurrent_work();
   static void do_concurrent_work(JavaThread* jt);
   static bool has_work() { return the_table()->_has_work; }
+  static void trigger_cleanup();
 
   // Probing
   static Symbol* lookup(const char* name, int len, TRAPS);
diff -r 5bc1634bc0ca -r 739eaf4ac4ad src/hotspot/share/classfile/systemDictionary.cpp
--- a/src/hotspot/share/classfile/systemDictionary.cpp	Thu Jan 31 10:18:41 2019 -0500
+++ b/src/hotspot/share/classfile/systemDictionary.cpp	Thu Jan 31 10:29:53 2019 -0500
@@ -1825,40 +1825,27 @@
     if (unloading_occurred) {
       MutexLockerEx ml2(is_concurrent ? Module_lock : NULL);
       JFR_ONLY(Jfr::on_unloading_classes();)
+
       MutexLockerEx ml1(is_concurrent ? SystemDictionary_lock : NULL);
       ClassLoaderDataGraph::clean_module_and_package_info();
-    }
-  }
-
-  // Cleanup ResolvedMethodTable even if no unloading occurred.
-  {
-    GCTraceTime(Debug, gc, phases) t("ResolvedMethodTable", gc_timer);
-    ResolvedMethodTable::trigger_cleanup();
-  }
-
-  if (unloading_occurred) {
-    {
-      GCTraceTime(Debug, gc, phases) t("SymbolTable", gc_timer);
-      // Check if there's work to do in the SymbolTable
-      SymbolTable::do_check_concurrent_work();
-    }
-
-    {
-      MutexLockerEx ml(is_concurrent ? SystemDictionary_lock : NULL);
-      GCTraceTime(Debug, gc, phases) t("Dictionary", gc_timer);
       constraints()->purge_loader_constraints();
       resolution_errors()->purge_resolution_errors();
     }
-
-    {
-      GCTraceTime(Debug, gc, phases) t("ResolvedMethodTable", gc_timer);
-      // Oops referenced by the protection domain cache table may get unreachable independently
-      // of the class loader (eg. cached protection domain oops). So we need to
-      // explicitly unlink them here.
-      // All protection domain oops are linked to the caller class, so if nothing
-      // unloads, this is not needed.
-      _pd_cache_table->trigger_cleanup();
-    }
+  }
+
+  GCTraceTime(Debug, gc, phases) t("Trigger cleanups", gc_timer);
+  // Trigger cleaning the ResolvedMethodTable even if no unloading occurred.
+  ResolvedMethodTable::trigger_cleanup();
+
+  if (unloading_occurred) {
+    SymbolTable::trigger_cleanup();
+
+    // Oops referenced by the protection domain cache table may get unreachable independently
+    // of the class loader (eg. cached protection domain oops). So we need to
+    // explicitly unlink them here.
+    // All protection domain oops are linked to the caller class, so if nothing
+    // unloads, this is not needed.
+    _pd_cache_table->trigger_cleanup();
   }
 
   return unloading_occurred;
diff -r 5bc1634bc0ca -r 739eaf4ac4ad src/hotspot/share/gc/cms/concurrentMarkSweepGeneration.cpp
--- a/src/hotspot/share/gc/cms/concurrentMarkSweepGeneration.cpp	Thu Jan 31 10:18:41 2019 -0500
+++ b/src/hotspot/share/gc/cms/concurrentMarkSweepGeneration.cpp	Thu Jan 31 10:29:53 2019 -0500
@@ -5269,12 +5269,6 @@
       // Prune dead klasses from subklass/sibling/implementor lists.
       Klass::clean_weak_klass_links(purged_class);
     }
-
-    {
-      GCTraceTime(Debug, gc, phases) t("Scrub Symbol Table", _gc_timer_cm);
-      // Clean up unreferenced symbols in symbol table.
-      SymbolTable::unlink();
-    }
   }
 
   // Restore any preserved marks as a result of mark stack or
diff -r 5bc1634bc0ca -r 739eaf4ac4ad src/hotspot/share/gc/parallel/psMarkSweep.cpp
--- a/src/hotspot/share/gc/parallel/psMarkSweep.cpp	Thu Jan 31 10:18:41 2019 -0500
+++ b/src/hotspot/share/gc/parallel/psMarkSweep.cpp	Thu Jan 31 10:29:53 2019 -0500
@@ -566,12 +566,6 @@
     Klass::clean_weak_klass_links(purged_class);
   }
 
-  {
-    GCTraceTime(Debug, gc, phases) t("Scrub Symbol Table", _gc_timer);
-    // Clean up unreferenced symbols in symbol table.
-    SymbolTable::unlink();
-  }
-
   _gc_tracer->report_object_count_after_gc(is_alive_closure());
 }
 
diff -r 5bc1634bc0ca -r 739eaf4ac4ad src/hotspot/share/gc/parallel/psParallelCompact.cpp
--- a/src/hotspot/share/gc/parallel/psParallelCompact.cpp	Thu Jan 31 10:18:41 2019 -0500
+++ b/src/hotspot/share/gc/parallel/psParallelCompact.cpp	Thu Jan 31 10:29:53 2019 -0500
@@ -2184,12 +2184,6 @@
     Klass::clean_weak_klass_links(purged_class);
   }
 
-  {
-    GCTraceTime(Debug, gc, phases) t("Scrub Symbol Table", &_gc_timer);
-    // Clean up unreferenced symbols in symbol table.
-    SymbolTable::unlink();
-  }
-
   _gc_tracer.report_object_count_after_gc(is_alive_closure());
 }
 
diff -r 5bc1634bc0ca -r 739eaf4ac4ad src/hotspot/share/gc/serial/genMarkSweep.cpp
--- a/src/hotspot/share/gc/serial/genMarkSweep.cpp	Thu Jan 31 10:18:41 2019 -0500
+++ b/src/hotspot/share/gc/serial/genMarkSweep.cpp	Thu Jan 31 10:29:53 2019 -0500
@@ -239,12 +239,6 @@
     Klass::clean_weak_klass_links(purged_class);
   }
 
-  {
-    GCTraceTime(Debug, gc, phases) t("Scrub Symbol Table", gc_timer());
-    // Clean up unreferenced symbols in symbol table.
-    SymbolTable::unlink();
-  }
-
   gc_tracer()->report_object_count_after_gc(&is_alive);
 }