diff --git a/src/hotspot/share/classfile/dictionary.cpp b/src/hotspot/share/classfile/dictionary.cpp index f60d426f5fb..f1f02e11d81 100644 --- a/src/hotspot/share/classfile/dictionary.cpp +++ b/src/hotspot/share/classfile/dictionary.cpp @@ -229,11 +229,13 @@ public: uintx get_hash() const { return _name->identity_hash(); } - bool equals(DictionaryEntry** value, bool* is_dead) { + bool equals(DictionaryEntry** value) { DictionaryEntry *entry = *value; - *is_dead = false; return (entry->instance_klass()->name() == _name); } + bool is_dead(DictionaryEntry** value) { + return false; + } }; // Add a loaded class to the dictionary. diff --git a/src/hotspot/share/classfile/stringTable.cpp b/src/hotspot/share/classfile/stringTable.cpp index 2751148dff3..66b9ca4ff02 100644 --- a/src/hotspot/share/classfile/stringTable.cpp +++ b/src/hotspot/share/classfile/stringTable.cpp @@ -176,11 +176,9 @@ class StringTableLookupJchar : StackObj { uintx get_hash() const { return _hash; } - bool equals(WeakHandle* value, bool* is_dead) { + bool equals(WeakHandle* value) { oop val_oop = value->peek(); if (val_oop == nullptr) { - // dead oop, mark this hash dead for cleaning - *is_dead = true; return false; } bool equals = java_lang_String::equals(val_oop, _str, _len); @@ -191,6 +189,10 @@ class StringTableLookupJchar : StackObj { _found = Handle(_thread, value->resolve()); return true; } + bool is_dead(WeakHandle* value) { + oop val_oop = value->peek(); + return val_oop == nullptr; + } }; class StringTableLookupOop : public StackObj { @@ -208,11 +210,9 @@ class StringTableLookupOop : public StackObj { return _hash; } - bool equals(WeakHandle* value, bool* is_dead) { + bool equals(WeakHandle* value) { oop val_oop = value->peek(); if (val_oop == nullptr) { - // dead oop, mark this hash dead for cleaning - *is_dead = true; return false; } bool equals = java_lang_String::equals(_find(), val_oop); @@ -223,6 +223,11 @@ class StringTableLookupOop : public StackObj { _found = Handle(_thread, value->resolve()); return true; } + + bool is_dead(WeakHandle* value) { + oop val_oop = value->peek(); + return val_oop == nullptr; + } }; void StringTable::create_table() { diff --git a/src/hotspot/share/classfile/symbolTable.cpp b/src/hotspot/share/classfile/symbolTable.cpp index a6fb48ac547..6162be7c0fe 100644 --- a/src/hotspot/share/classfile/symbolTable.cpp +++ b/src/hotspot/share/classfile/symbolTable.cpp @@ -374,7 +374,11 @@ public: uintx get_hash() const { return _hash; } - bool equals(Symbol* value, bool* is_dead) { + // Note: When equals() returns "true", the symbol's refcount is incremented. This is + // needed to ensure that the symbol is kept alive before equals() returns to the caller, + // so that another thread cannot clean the symbol up concurrently. The caller is + // responsible for decrementing the refcount, when the symbol is no longer needed. + bool equals(Symbol* value) { assert(value != nullptr, "expected valid value"); Symbol *sym = value; if (sym->equals(_str, _len)) { @@ -383,14 +387,15 @@ public: return true; } else { assert(sym->refcount() == 0, "expected dead symbol"); - *is_dead = true; return false; } } else { - *is_dead = (sym->refcount() == 0); return false; } } + bool is_dead(Symbol* value) { + return value->refcount() == 0; + } }; class SymbolTableGet : public StackObj { diff --git a/src/hotspot/share/gc/g1/g1CardSet.cpp b/src/hotspot/share/gc/g1/g1CardSet.cpp index 4e3f08ddc9d..f39e2066739 100644 --- a/src/hotspot/share/gc/g1/g1CardSet.cpp +++ b/src/hotspot/share/gc/g1/g1CardSet.cpp @@ -258,10 +258,13 @@ class G1CardSetHashTable : public CHeapObj { uintx get_hash() const { return G1CardSetHashTable::get_hash(_region_idx); } - bool equals(G1CardSetHashTableValue* value, bool* is_dead) { - *is_dead = false; + bool equals(G1CardSetHashTableValue* value) { return value->_region_idx == _region_idx; } + + bool is_dead(G1CardSetHashTableValue*) { + return false; + } }; class G1CardSetHashTableFound : public StackObj { diff --git a/src/hotspot/share/prims/resolvedMethodTable.cpp b/src/hotspot/share/prims/resolvedMethodTable.cpp index fbcbd2b1590..c10fe1f6601 100644 --- a/src/hotspot/share/prims/resolvedMethodTable.cpp +++ b/src/hotspot/share/prims/resolvedMethodTable.cpp @@ -126,11 +126,9 @@ class ResolvedMethodTableLookup : StackObj { uintx get_hash() const { return _hash; } - bool equals(WeakHandle* value, bool* is_dead) { + bool equals(WeakHandle* value) { oop val_oop = value->peek(); if (val_oop == nullptr) { - // dead oop, mark this hash dead for cleaning - *is_dead = true; return false; } bool equals = _method == java_lang_invoke_ResolvedMethodName::vmtarget(val_oop); @@ -141,6 +139,10 @@ class ResolvedMethodTableLookup : StackObj { _found = Handle(_thread, value->resolve()); return true; } + bool is_dead(WeakHandle* value) { + oop val_oop = value->peek(); + return val_oop == nullptr; + } }; diff --git a/src/hotspot/share/services/finalizerService.cpp b/src/hotspot/share/services/finalizerService.cpp index 202a1af0801..ecd9168cd65 100644 --- a/src/hotspot/share/services/finalizerService.cpp +++ b/src/hotspot/share/services/finalizerService.cpp @@ -137,11 +137,14 @@ class FinalizerEntryLookup : StackObj { public: FinalizerEntryLookup(const InstanceKlass* ik) : _ik(ik) {} uintx get_hash() const { return hash_function(_ik); } - bool equals(FinalizerEntry** value, bool* is_dead) { + bool equals(FinalizerEntry** value) { assert(value != nullptr, "invariant"); assert(*value != nullptr, "invariant"); return (*value)->klass() == _ik; } + bool is_dead(FinalizerEntry** value) { + return false; + } }; class FinalizerTableConfig : public AllStatic { diff --git a/src/hotspot/share/services/threadIdTable.cpp b/src/hotspot/share/services/threadIdTable.cpp index ba0e6bdd4fd..168b2e085ad 100644 --- a/src/hotspot/share/services/threadIdTable.cpp +++ b/src/hotspot/share/services/threadIdTable.cpp @@ -187,13 +187,16 @@ public: uintx get_hash() const { return _hash; } - bool equals(ThreadIdTableEntry** value, bool* is_dead) { + bool equals(ThreadIdTableEntry** value) { bool equals = primitive_equals(_tid, (*value)->tid()); if (!equals) { return false; } return true; } + bool is_dead(ThreadIdTableEntry** value) { + return false; + } }; class ThreadGet : public StackObj { diff --git a/src/hotspot/share/utilities/concurrentHashTable.inline.hpp b/src/hotspot/share/utilities/concurrentHashTable.inline.hpp index 0d62a9f162e..b222d379b72 100644 --- a/src/hotspot/share/utilities/concurrentHashTable.inline.hpp +++ b/src/hotspot/share/utilities/concurrentHashTable.inline.hpp @@ -455,9 +455,8 @@ inline bool ConcurrentHashTable:: assert(bucket->is_locked(), "Must be locked."); Node* const volatile * rem_n_prev = bucket->first_ptr(); Node* rem_n = bucket->first(); - bool have_dead = false; while (rem_n != nullptr) { - if (lookup_f.equals(rem_n->value(), &have_dead)) { + if (lookup_f.equals(rem_n->value())) { bucket->release_assign_node_ptr(rem_n_prev, rem_n->next()); break; } else { @@ -546,9 +545,7 @@ inline void ConcurrentHashTable:: Node* const volatile * rem_n_prev = bucket->first_ptr(); Node* rem_n = bucket->first(); while (rem_n != nullptr) { - bool is_dead = false; - lookup_f.equals(rem_n->value(), &is_dead); - if (is_dead) { + if (lookup_f.is_dead(rem_n->value())) { ndel[dels++] = rem_n; Node* next_node = rem_n->next(); bucket->release_assign_node_ptr(rem_n_prev, next_node); @@ -626,12 +623,11 @@ ConcurrentHashTable:: size_t loop_count = 0; Node* node = bucket->first(); while (node != nullptr) { - bool is_dead = false; ++loop_count; - if (lookup_f.equals(node->value(), &is_dead)) { + if (lookup_f.equals(node->value())) { break; } - if (is_dead && !(*have_dead)) { + if (!(*have_dead) && lookup_f.is_dead(node->value())) { *have_dead = true; } node = node->next(); diff --git a/test/hotspot/gtest/classfile/test_symbolTable.cpp b/test/hotspot/gtest/classfile/test_symbolTable.cpp index 77d076ec213..4f4cbfe3e89 100644 --- a/test/hotspot/gtest/classfile/test_symbolTable.cpp +++ b/test/hotspot/gtest/classfile/test_symbolTable.cpp @@ -125,3 +125,17 @@ TEST_VM_FATAL_ERROR_MSG(SymbolTable, test_symbol_underflow, ".*refcount has gone my_symbol->decrement_refcount(); my_symbol->increment_refcount(); // Should crash even in PRODUCT mode } + +TEST_VM(SymbolTable, test_cleanup_leak) { + // Check that dead entry cleanup doesn't increment refcount of live entry in same bucket. + + // Create symbol and release ref, marking it available for cleanup. + Symbol* entry1 = SymbolTable::new_symbol("hash_collision_123"); + entry1->decrement_refcount(); + + // Create a new symbol in the same bucket, which will notice the dead entry and trigger cleanup. + // Note: relies on SymbolTable's use of String::hashCode which collides for these two values. + Symbol* entry2 = SymbolTable::new_symbol("hash_collision_397476851"); + + ASSERT_EQ(entry2->refcount(), 1) << "Symbol refcount just created is 1"; +} diff --git a/test/hotspot/gtest/utilities/test_concurrentHashtable.cpp b/test/hotspot/gtest/utilities/test_concurrentHashtable.cpp index 2094565dea3..ca41fc6b0f1 100644 --- a/test/hotspot/gtest/utilities/test_concurrentHashtable.cpp +++ b/test/hotspot/gtest/utilities/test_concurrentHashtable.cpp @@ -107,9 +107,12 @@ struct SimpleTestLookup { uintx get_hash() { return Pointer::get_hash(_val, NULL); } - bool equals(const uintptr_t* value, bool* is_dead) { + bool equals(const uintptr_t* value) { return _val == *value; } + bool is_dead(const uintptr_t* value) { + return false; + } }; struct ValueGet { @@ -561,9 +564,12 @@ struct TestLookup { uintx get_hash() { return TestInterface::get_hash(_val, NULL); } - bool equals(const uintptr_t* value, bool* is_dead) { + bool equals(const uintptr_t* value) { return _val == *value; } + bool is_dead(const uintptr_t* value) { + return false; + } }; static uintptr_t cht_get_copy(TestTable* cht, Thread* thr, TestLookup tl) { diff --git a/test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/DynamicSharedSymbols.java b/test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/DynamicSharedSymbols.java index 806313e270e..24305c5a166 100644 --- a/test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/DynamicSharedSymbols.java +++ b/test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/DynamicSharedSymbols.java @@ -90,7 +90,7 @@ public class DynamicSharedSymbols extends DynamicArchiveTestBase { 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 3: jdk/test/lib/apps\n"); + output.shouldContain("17 2: jdk/test/lib/apps\n"); output.shouldContain("Dynamic shared symbols:\n"); output.shouldContain("5 65535: Hello\n");