From d23f4f12adf1ea26b8c340efe2c3854e50b68301 Mon Sep 17 00:00:00 2001 From: Oli Gillespie Date: Mon, 4 Dec 2023 12:25:51 +0000 Subject: [PATCH] 8315559: Delay TempSymbol cleanup to avoid symbol table churn Reviewed-by: coleenp, kbarrett, shade --- src/hotspot/share/oops/symbolHandle.cpp | 47 ++++++++++++++ src/hotspot/share/oops/symbolHandle.hpp | 19 +++++- .../gtest/classfile/test_placeholders.cpp | 11 +++- .../gtest/classfile/test_symbolTable.cpp | 65 +++++++++++++++++-- 4 files changed, 133 insertions(+), 9 deletions(-) create mode 100644 src/hotspot/share/oops/symbolHandle.cpp diff --git a/src/hotspot/share/oops/symbolHandle.cpp b/src/hotspot/share/oops/symbolHandle.cpp new file mode 100644 index 00000000000..350f0dd96c8 --- /dev/null +++ b/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); + } +} diff --git a/src/hotspot/share/oops/symbolHandle.hpp b/src/hotspot/share/oops/symbolHandle.hpp index 249da936761..f1b2d2470c5 100644 --- a/src/hotspot/share/oops/symbolHandle.hpp +++ b/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 @@ public: 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); } } diff --git a/test/hotspot/gtest/classfile/test_placeholders.cpp b/test/hotspot/gtest/classfile/test_placeholders.cpp index 9fa06d49f73..4179a1fc695 100644 --- a/test/hotspot/gtest/classfile/test_placeholders.cpp +++ b/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(); } diff --git a/test/hotspot/gtest/classfile/test_symbolTable.cpp b/test/hotspot/gtest/classfile/test_symbolTable.cpp index 4f4cbfe3e89..10f9560530e 100644 --- a/test/hotspot/gtest/classfile/test_symbolTable.cpp +++ b/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"; + } +}