Skip to content

Commit

Permalink
8315559: Delay TempSymbol cleanup to avoid symbol table churn
Browse files Browse the repository at this point in the history
Reviewed-by: coleenp, kbarrett, shade
  • Loading branch information
olivergillespie authored and coleenp committed Dec 4, 2023
1 parent c17b8cf commit d23f4f1
Show file tree
Hide file tree
Showing 4 changed files with 133 additions and 9 deletions.
47 changes: 47 additions & 0 deletions src/hotspot/share/oops/symbolHandle.cpp
Original file line number Diff line number Diff line change
@@ -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);
}
}
19 changes: 18 additions & 1 deletion src/hotspot/share/oops/symbolHandle.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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);
}
}

Expand Down
11 changes: 8 additions & 3 deletions test/hotspot/gtest/classfile/test_placeholders.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down Expand Up @@ -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();
}
65 changes: 60 additions & 5 deletions test/hotspot/gtest/classfile/test_symbolTable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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";

Expand All @@ -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";

Expand All @@ -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";
Expand Down Expand Up @@ -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";
}
}

1 comment on commit d23f4f1

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.