8315559: Delay TempSymbol cleanup to avoid symbol table churn

8321276: runtime/cds/appcds/dynamicArchive/DynamicSharedSymbols.java failed with "'17 2: jdk/test/lib/apps ' missing from stdout/stderr"

Reviewed-by: coleenp
Backport-of: d23f4f12adf1ea26b8c340efe2c3854e50b68301

src/hotspot/share/oops/symbolHandle.cpp

@@ -0,0 +1,47 @@
+/*
+ * Copyright Amazon.com Inc. 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
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+#include "precompiled.hpp"
+#include "oops/symbolHandle.hpp"
+#include "runtime/atomic.hpp"
+
+Symbol* volatile TempSymbolCleanupDelayer::_queue[QueueSize] = {};
+volatile uint TempSymbolCleanupDelayer::_index = 0;
+
+// Keep this symbol alive for some time to allow for reuse.
+// Temp symbols for the same string can often be created in quick succession,
+// and this queue allows them to be reused instead of churning.
+void TempSymbolCleanupDelayer::delay_cleanup(Symbol* sym) {
+  assert(sym != nullptr, "precondition");
+  sym->increment_refcount();
+  uint i = Atomic::add(&_index, 1u) % QueueSize;
+  Symbol* old = Atomic::xchg(&_queue[i], sym);
+  Symbol::maybe_decrement_refcount(old);
+}
+
+void TempSymbolCleanupDelayer::drain_queue() {
+  for (uint i = 0; i < QueueSize; i++) {
+    Symbol* sym = Atomic::xchg(&_queue[i], (Symbol*) nullptr);
+    Symbol::maybe_decrement_refcount(sym);
+  }
+}

src/hotspot/share/oops/symbolHandle.hpp

@@ -28,6 +28,16 @@
 #include "memory/allocation.hpp"
 #include "oops/symbol.hpp"
 
+class TempSymbolCleanupDelayer : AllStatic {
+  static Symbol* volatile _queue[];
+  static volatile uint _index;
+
+public:
+  static const uint QueueSize = 128;
+  static void delay_cleanup(Symbol* s);
+  static void drain_queue();
+};
+
 // TempNewSymbol acts as a handle class in a handle/body idiom and is
 // responsible for proper resource management of the body (which is a Symbol*).
 // The body is resource managed by a reference counting scheme.
@@ -49,10 +59,17 @@ class SymbolHandleBase : public StackObj {
   SymbolHandleBase() : _temp(nullptr) { }
 
   // Conversion from a Symbol* to a SymbolHandleBase.
-  // Does not increment the current reference count if temporary.
   SymbolHandleBase(Symbol *s) : _temp(s) {
     if (!TEMP) {
       Symbol::maybe_increment_refcount(_temp);
+      return;
+    }
+
+    // Delay cleanup for temp symbols. Refcount is incremented while in
+    // queue. But don't requeue existing entries, or entries that are held
+    // elsewhere - it's a waste of effort.
+    if (s != nullptr && s->refcount() == 1) {
+      TempSymbolCleanupDelayer::delay_cleanup(s);
     }
   }
 

test/hotspot/gtest/classfile/test_placeholders.cpp

@@ -39,10 +39,10 @@ TEST_VM(PlaceholderTable, supername) {
   ThreadInVMfromNative tivfn(THREAD);
 
   // Assert messages assume these symbols are unique, and the refcounts start at one.
-  TempNewSymbol A = SymbolTable::new_symbol("abc2_8_2023_class");
-  TempNewSymbol D = SymbolTable::new_symbol("def2_8_2023_class");
+  Symbol* A = SymbolTable::new_symbol("abc2_8_2023_class");
+  Symbol* D = SymbolTable::new_symbol("def2_8_2023_class");
   Symbol* super = SymbolTable::new_symbol("super2_8_2023_supername");
-  TempNewSymbol interf = SymbolTable::new_symbol("interface2_8_2023_supername");
+  Symbol* interf = SymbolTable::new_symbol("interface2_8_2023_supername");
 
   ClassLoaderData* loader_data = ClassLoaderData::the_null_class_loader_data();
 
@@ -110,4 +110,9 @@ TEST_VM(PlaceholderTable, supername) {
   EXPECT_EQ(A->refcount(), 1) << "first lass name refcount should be 1";
   EXPECT_EQ(D->refcount(), 1) << "second class name refcount should be 1";
   EXPECT_EQ(super->refcount(), 0) << "super class name refcount should be 0 - was unloaded.";
+
+  // clean up temporary symbols
+  A->decrement_refcount();
+  D->decrement_refcount();
+  interf->decrement_refcount();
 }

test/hotspot/gtest/classfile/test_symbolTable.cpp

@@ -27,6 +27,14 @@
 #include "threadHelper.inline.hpp"
 #include "unittest.hpp"
 
+// Helper to avoid interference from the cleanup delay queue by draining it
+// immediately after creation.
+TempNewSymbol stable_temp_symbol(Symbol* sym) {
+  TempNewSymbol t = sym;
+  TempSymbolCleanupDelayer::drain_queue();
+  return t;
+}
+
 TEST_VM(SymbolTable, temp_new_symbol) {
   // Assert messages assume these symbols are unique, and the refcounts start at
   // one, but code does not rely on this.
@@ -36,7 +44,7 @@ TEST_VM(SymbolTable, temp_new_symbol) {
 
   Symbol* abc = SymbolTable::new_symbol("abc");
   int abccount = abc->refcount();
-  TempNewSymbol ss = abc;
+  TempNewSymbol ss = stable_temp_symbol(abc);
   ASSERT_EQ(ss->refcount(), abccount) << "only one abc";
   ASSERT_EQ(ss->refcount(), abc->refcount()) << "should match TempNewSymbol";
 
@@ -45,8 +53,8 @@ TEST_VM(SymbolTable, temp_new_symbol) {
   int efgcount = efg->refcount();
   int hijcount = hij->refcount();
 
-  TempNewSymbol s1 = efg;
-  TempNewSymbol s2 = hij;
+  TempNewSymbol s1 = stable_temp_symbol(efg);
+  TempNewSymbol s2 = stable_temp_symbol(hij);
   ASSERT_EQ(s1->refcount(), efgcount) << "one efg";
   ASSERT_EQ(s2->refcount(), hijcount) << "one hij";
 
@@ -65,13 +73,13 @@ TEST_VM(SymbolTable, temp_new_symbol) {
   TempNewSymbol s3;
   Symbol* klm = SymbolTable::new_symbol("klm");
   int klmcount = klm->refcount();
-  s3 = klm; // assignment
+  s3 = stable_temp_symbol(klm); // assignment
   ASSERT_EQ(s3->refcount(), klmcount) << "only one klm now";
 
   Symbol* xyz = SymbolTable::new_symbol("xyz");
   int xyzcount = xyz->refcount();
   { // inner scope
-    TempNewSymbol s_inner = xyz;
+    TempNewSymbol s_inner = stable_temp_symbol(xyz);
   }
   ASSERT_EQ(xyz->refcount(), xyzcount - 1)
           << "Should have been decremented by dtor in inner scope";
@@ -139,3 +147,50 @@ TEST_VM(SymbolTable, test_cleanup_leak) {
 
   ASSERT_EQ(entry2->refcount(), 1) << "Symbol refcount just created is 1";
 }
+
+TEST_VM(SymbolTable, test_cleanup_delay) {
+  // Check that new temp symbols have an extra refcount increment, which is then
+  // decremented when the queue spills over.
+
+  TempNewSymbol s1 = SymbolTable::new_symbol("temp-s1");
+  ASSERT_EQ(s1->refcount(), 2) << "TempNewSymbol refcount just created is 2";
+
+  // Fill up the queue
+  constexpr int symbol_name_length = 30;
+  char symbol_name[symbol_name_length];
+  for (uint i = 1; i < TempSymbolCleanupDelayer::QueueSize; i++) {
+    os::snprintf(symbol_name, symbol_name_length, "temp-filler-%d", i);
+    TempNewSymbol s = SymbolTable::new_symbol(symbol_name);
+    ASSERT_EQ(s->refcount(), 2) << "TempNewSymbol refcount just created is 2";
+  }
+
+  // Add one more
+  TempNewSymbol spillover = SymbolTable::new_symbol("temp-spillover");
+  ASSERT_EQ(spillover->refcount(), 2) << "TempNewSymbol refcount just created is 2";
+
+  // The first symbol should have been removed from the queue and decremented
+  ASSERT_EQ(s1->refcount(), 1) << "TempNewSymbol off queue refcount is 1";
+}
+
+TEST_VM(SymbolTable, test_cleanup_delay_drain) {
+  // Fill up the queue
+  constexpr int symbol_name_length = 30;
+  char symbol_name[symbol_name_length];
+  TempNewSymbol symbols[TempSymbolCleanupDelayer::QueueSize] = {};
+  for (uint i = 0; i < TempSymbolCleanupDelayer::QueueSize; i++) {
+    os::snprintf(symbol_name, symbol_name_length, "temp-%d", i);
+    TempNewSymbol s = SymbolTable::new_symbol(symbol_name);
+    symbols[i] = s;
+  }
+
+  // While in the queue refcounts are incremented
+  for (uint i = 0; i < TempSymbolCleanupDelayer::QueueSize; i++) {
+    ASSERT_EQ(symbols[i]->refcount(), 2) << "TempNewSymbol refcount in queue is 2";
+  }
+
+  // Draining the queue should decrement the refcounts
+  TempSymbolCleanupDelayer::drain_queue();
+  for (uint i = 0; i < TempSymbolCleanupDelayer::QueueSize; i++) {
+    ASSERT_EQ(symbols[i]->refcount(), 1) << "TempNewSymbol refcount after drain is 1";
+  }
+}

test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/DynamicSharedSymbols.java

@@ -90,7 +90,8 @@ private static void doTest(String topArchiveName) throws Exception {
         ProcessBuilder pb = new ProcessBuilder();
         pb.command(new String[] {JDKToolFinder.getJDKTool("jcmd"), Long.toString(pid), "VM.symboltable", "-verbose"});
         OutputAnalyzer output = CDSTestUtils.executeAndLog(pb, "jcmd-symboltable");
-        output.shouldContain("17 2: jdk/test/lib/apps\n");
+        output.shouldContain("17 3: jdk/test/lib/apps\n");  // 3 because a TempSymbol will be found in the TempSymbolCleanupDelayer queue.
+                   // Note: we might want to drain the queue before CDS dumps but this is correct for now, unless the queue length changes.
         output.shouldContain("Dynamic shared symbols:\n");
         output.shouldContain("5 65535: Hello\n");