diff --git a/src/hotspot/share/cds/cdsProtectionDomain.cpp b/src/hotspot/share/cds/cdsProtectionDomain.cpp index fc2af04d7c241..7f8c25d9ae5b5 100644 --- a/src/hotspot/share/cds/cdsProtectionDomain.cpp +++ b/src/hotspot/share/cds/cdsProtectionDomain.cpp @@ -28,7 +28,6 @@ #include "classfile/classLoaderExt.hpp" #include "classfile/javaClasses.hpp" #include "classfile/moduleEntry.hpp" -#include "classfile/symbolTable.hpp" #include "classfile/systemDictionaryShared.hpp" #include "classfile/vmClasses.hpp" #include "classfile/vmSymbols.hpp" diff --git a/src/hotspot/share/ci/ciEnv.cpp b/src/hotspot/share/ci/ciEnv.cpp index 5c58f512d73e2..9943140782986 100644 --- a/src/hotspot/share/ci/ciEnv.cpp +++ b/src/hotspot/share/ci/ciEnv.cpp @@ -36,7 +36,6 @@ #include "ci/ciUtilities.inline.hpp" #include "classfile/javaClasses.hpp" #include "classfile/javaClasses.inline.hpp" -#include "classfile/symbolTable.hpp" #include "classfile/systemDictionary.hpp" #include "classfile/vmClasses.hpp" #include "classfile/vmSymbols.hpp" @@ -65,6 +64,7 @@ #include "oops/objArrayKlass.hpp" #include "oops/objArrayOop.inline.hpp" #include "oops/oop.inline.hpp" +#include "oops/symbolHandle.hpp" #include "prims/jvmtiExport.hpp" #include "prims/methodHandles.hpp" #include "runtime/fieldDescriptor.inline.hpp" diff --git a/src/hotspot/share/classfile/classFileParser.hpp b/src/hotspot/share/classfile/classFileParser.hpp index 528743b3bd50c..3fe2414e93a3e 100644 --- a/src/hotspot/share/classfile/classFileParser.hpp +++ b/src/hotspot/share/classfile/classFileParser.hpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 1997, 2021, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1997, 2022, 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 @@ -47,7 +47,6 @@ class GrowableArray; class InstanceKlass; class RecordComponent; class Symbol; -class TempNewSymbol; class FieldLayoutBuilder; // Utility to collect and compact oop maps during layout diff --git a/src/hotspot/share/classfile/loaderConstraints.cpp b/src/hotspot/share/classfile/loaderConstraints.cpp index 7a0bc547c19b1..4e660de191c9c 100644 --- a/src/hotspot/share/classfile/loaderConstraints.cpp +++ b/src/hotspot/share/classfile/loaderConstraints.cpp @@ -32,6 +32,7 @@ #include "memory/resourceArea.hpp" #include "oops/klass.inline.hpp" #include "oops/oop.inline.hpp" +#include "oops/symbolHandle.hpp" #include "runtime/handles.inline.hpp" #include "runtime/mutexLocker.hpp" #include "runtime/safepoint.hpp" @@ -110,7 +111,7 @@ class ConstraintSet { // copied into hashtable as }; -ResourceHashtable _loader_constraint_table; +ResourceHashtable _loader_constraint_table; void LoaderConstraint::extend_loader_constraint(Symbol* class_name, Handle loader, @@ -171,8 +172,6 @@ void LoaderConstraintTable::add_loader_constraint(Symbol* name, InstanceKlass* k bool created; ConstraintSet* set = _loader_constraint_table.put_if_absent(name, &created); if (created) { - // Increment the key refcount when putting in the table. - name->increment_refcount(); set->initialize(constraint); } else { set->add_constraint(constraint); @@ -181,7 +180,7 @@ void LoaderConstraintTable::add_loader_constraint(Symbol* name, InstanceKlass* k class PurgeUnloadedConstraints : public StackObj { public: - bool do_entry(Symbol*& name, ConstraintSet& set) { + bool do_entry(SymbolHandle& name, ConstraintSet& set) { LogTarget(Info, class, loader, constraints) lt; int len = set.num_constraints(); for (int i = len - 1; i >= 0; i--) { @@ -242,8 +241,6 @@ class PurgeUnloadedConstraints : public StackObj { } } if (set.num_constraints() == 0) { - // decrement name refcount before freeing - name->decrement_refcount(); return true; } // Don't unlink this set @@ -455,14 +452,14 @@ void LoaderConstraintTable::merge_loader_constraints(Symbol* class_name, void LoaderConstraintTable::verify() { Thread* thread = Thread::current(); - auto check = [&] (Symbol*& key, ConstraintSet& set) { + auto check = [&] (SymbolHandle& key, ConstraintSet& set) { // foreach constraint in the set, check the klass is in the dictionary or placeholder table. int len = set.num_constraints(); for (int i = 0; i < len; i++) { LoaderConstraint* probe = set.constraint_at(i); if (probe->klass() != NULL) { InstanceKlass* ik = probe->klass(); - guarantee(ik->name() == key, "name should match"); + guarantee(key == ik->name(), "name should match"); Symbol* name = ik->name(); ClassLoaderData* loader_data = ik->class_loader_data(); Dictionary* dictionary = loader_data->dictionary(); @@ -492,7 +489,7 @@ void LoaderConstraintTable::verify() { } void LoaderConstraintTable::print_table_statistics(outputStream* st) { - auto size = [&] (Symbol*& key, ConstraintSet& set) { + auto size = [&] (SymbolHandle& key, ConstraintSet& set) { // sizeof set is included in the size of the hashtable node int sum = 0; int len = set.num_constraints(); @@ -508,7 +505,7 @@ void LoaderConstraintTable::print_table_statistics(outputStream* st) { // Called with the system dictionary lock held void LoaderConstraintTable::print_on(outputStream* st) { - auto printer = [&] (Symbol*& key, ConstraintSet& set) { + auto printer = [&] (SymbolHandle& key, ConstraintSet& set) { int len = set.num_constraints(); for (int i = 0; i < len; i++) { LoaderConstraint* probe = set.constraint_at(i); diff --git a/src/hotspot/share/classfile/moduleEntry.cpp b/src/hotspot/share/classfile/moduleEntry.cpp index 2d5d4ec258a71..ad6960a62c48c 100644 --- a/src/hotspot/share/classfile/moduleEntry.cpp +++ b/src/hotspot/share/classfile/moduleEntry.cpp @@ -355,7 +355,7 @@ ModuleEntryTable::ModuleEntryTable() { } ModuleEntryTable::~ModuleEntryTable() { class ModuleEntryTableDeleter : public StackObj { public: - bool do_entry(const Symbol*& name, ModuleEntry*& entry) { + bool do_entry(const SymbolHandle& name, ModuleEntry*& entry) { if (log_is_enabled(Info, module, unload) || log_is_enabled(Debug, module)) { ResourceMark rm; const char* str = name->as_C_string(); @@ -511,7 +511,7 @@ static int compare_module_by_name(ModuleEntry* a, ModuleEntry* b) { } void ModuleEntryTable::iterate_symbols(MetaspaceClosure* closure) { - auto syms = [&] (const Symbol*& key, ModuleEntry*& m) { + auto syms = [&] (const SymbolHandle& key, ModuleEntry*& m) { m->iterate_symbols(closure); }; _table.iterate_all(syms); @@ -520,7 +520,7 @@ void ModuleEntryTable::iterate_symbols(MetaspaceClosure* closure) { Array* ModuleEntryTable::allocate_archived_entries() { Array* archived_modules = ArchiveBuilder::new_rw_array(_table.number_of_entries()); int n = 0; - auto grab = [&] (const Symbol*& key, ModuleEntry*& m) { + auto grab = [&] (const SymbolHandle& key, ModuleEntry*& m) { archived_modules->at_put(n++, m); }; _table.iterate_all(grab); @@ -603,7 +603,7 @@ ModuleEntry* ModuleEntryTable::lookup_only(Symbol* name) { // This should only occur at class unloading. void ModuleEntryTable::purge_all_module_reads() { assert_locked_or_safepoint(Module_lock); - auto purge = [&] (const Symbol*& key, ModuleEntry*& entry) { + auto purge = [&] (const SymbolHandle& key, ModuleEntry*& entry) { entry->purge_reads(); }; _table.iterate_all(purge); @@ -670,7 +670,7 @@ void ModuleEntryTable::patch_javabase_entries(Handle module_handle) { void ModuleEntryTable::print(outputStream* st) { ResourceMark rm; - auto printer = [&] (const Symbol*& name, ModuleEntry*& entry) { + auto printer = [&] (const SymbolHandle& name, ModuleEntry*& entry) { entry->print(st); }; st->print_cr("Module Entry Table (table_size=%d, entries=%d)", @@ -680,14 +680,14 @@ void ModuleEntryTable::print(outputStream* st) { } void ModuleEntryTable::modules_do(void f(ModuleEntry*)) { - auto do_f = [&] (const Symbol*& key, ModuleEntry*& entry) { + auto do_f = [&] (const SymbolHandle& key, ModuleEntry*& entry) { f(entry); }; _table.iterate_all(do_f); } void ModuleEntryTable::modules_do(ModuleClosure* closure) { - auto do_f = [&] (const Symbol*& key, ModuleEntry*& entry) { + auto do_f = [&] (const SymbolHandle& key, ModuleEntry*& entry) { closure->do_module(entry); }; _table.iterate_all(do_f); @@ -705,7 +705,7 @@ void ModuleEntry::print(outputStream* st) { } void ModuleEntryTable::verify() { - auto do_f = [&] (const Symbol*& key, ModuleEntry*& entry) { + auto do_f = [&] (const SymbolHandle& key, ModuleEntry*& entry) { entry->verify(); }; assert_locked_or_safepoint(Module_lock); diff --git a/src/hotspot/share/classfile/moduleEntry.hpp b/src/hotspot/share/classfile/moduleEntry.hpp index 78c6cac4d3d9c..5b9fe99da9550 100644 --- a/src/hotspot/share/classfile/moduleEntry.hpp +++ b/src/hotspot/share/classfile/moduleEntry.hpp @@ -28,6 +28,7 @@ #include "jni.h" #include "oops/oopHandle.hpp" #include "oops/symbol.hpp" +#include "oops/symbolHandle.hpp" #include "runtime/mutexLocker.hpp" #include "utilities/growableArray.hpp" #include "utilities/macros.hpp" @@ -205,8 +206,8 @@ class ModuleClosure: public StackObj { class ModuleEntryTable : public CHeapObj { private: static ModuleEntry* _javabase_module; - ResourceHashtable _table; + ResourceHashtable _table; public: ModuleEntryTable(); diff --git a/src/hotspot/share/classfile/packageEntry.cpp b/src/hotspot/share/classfile/packageEntry.cpp index 39ab87b34f426..96c2fe55f163f 100644 --- a/src/hotspot/share/classfile/packageEntry.cpp +++ b/src/hotspot/share/classfile/packageEntry.cpp @@ -193,7 +193,7 @@ PackageEntryTable::PackageEntryTable() { } PackageEntryTable::~PackageEntryTable() { class PackageEntryTableDeleter : public StackObj { public: - bool do_entry(const Symbol*& name, PackageEntry*& entry) { + bool do_entry(const SymbolHandle& name, PackageEntry*& entry) { if (log_is_enabled(Info, module, unload) || log_is_enabled(Debug, module)) { ResourceMark rm; const char* str = name->as_C_string(); @@ -270,7 +270,7 @@ static int compare_package_by_name(PackageEntry* a, PackageEntry* b) { } void PackageEntryTable::iterate_symbols(MetaspaceClosure* closure) { - auto syms = [&] (const Symbol*& key, PackageEntry*& p) { + auto syms = [&] (const SymbolHandle& key, PackageEntry*& p) { p->iterate_symbols(closure); }; _table.iterate_all(syms); @@ -279,7 +279,7 @@ void PackageEntryTable::iterate_symbols(MetaspaceClosure* closure) { Array* PackageEntryTable::allocate_archived_entries() { // First count the packages in named modules int n = 0; - auto count = [&] (const Symbol*& key, PackageEntry*& p) { + auto count = [&] (const SymbolHandle& key, PackageEntry*& p) { if (p->module()->is_named()) { n++; } @@ -289,7 +289,7 @@ Array* PackageEntryTable::allocate_archived_entries() { Array* archived_packages = ArchiveBuilder::new_rw_array(n); // reset n n = 0; - auto grab = [&] (const Symbol*& key, PackageEntry*& p) { + auto grab = [&] (const SymbolHandle& key, PackageEntry*& p) { if (p->module()->is_named()) { // We don't archive unnamed modules, or packages in unnamed modules. They will be // created on-demand at runtime as classes in such packages are loaded. @@ -374,7 +374,7 @@ PackageEntry* PackageEntryTable::locked_lookup_only(Symbol* name) { // Verify the packages loaded thus far are in java.base's package list. void PackageEntryTable::verify_javabase_packages(GrowableArray *pkg_list) { assert_lock_strong(Module_lock); - auto verifier = [&] (const Symbol*& name, PackageEntry*& entry) { + auto verifier = [&] (const SymbolHandle& name, PackageEntry*& entry) { ModuleEntry* m = entry->module(); Symbol* module_name = (m == NULL ? NULL : m->name()); if (module_name != NULL && @@ -410,7 +410,7 @@ bool PackageEntry::exported_pending_delete() const { // Remove dead entries from all packages' exported list void PackageEntryTable::purge_all_package_exports() { assert_locked_or_safepoint(Module_lock); - auto purge = [&] (const Symbol*& name, PackageEntry*& entry) { + auto purge = [&] (const SymbolHandle& name, PackageEntry*& entry) { if (entry->exported_pending_delete()) { // exported list is pending deletion due to a transition // from qualified to unqualified @@ -423,7 +423,7 @@ void PackageEntryTable::purge_all_package_exports() { } void PackageEntryTable::packages_do(void f(PackageEntry*)) { - auto doit = [&] (const Symbol*&name, PackageEntry*& entry) { + auto doit = [&] (const SymbolHandle&name, PackageEntry*& entry) { f(entry); }; assert_locked_or_safepoint(Module_lock); @@ -433,7 +433,7 @@ void PackageEntryTable::packages_do(void f(PackageEntry*)) { GrowableArray* PackageEntryTable::get_system_packages() { GrowableArray* loaded_class_pkgs = new GrowableArray(50); - auto grab = [&] (const Symbol*&name, PackageEntry*& entry) { + auto grab = [&] (const SymbolHandle& name, PackageEntry*& entry) { if (entry->has_loaded_class()) { loaded_class_pkgs->append(entry); } @@ -446,7 +446,7 @@ GrowableArray* PackageEntryTable::get_system_packages() { } void PackageEntryTable::print(outputStream* st) { - auto printer = [&] (const Symbol*& name, PackageEntry*& entry) { + auto printer = [&] (const SymbolHandle& name, PackageEntry*& entry) { entry->print(st); }; st->print_cr("Package Entry Table (table_size=%d, entries=%d)", diff --git a/src/hotspot/share/classfile/packageEntry.hpp b/src/hotspot/share/classfile/packageEntry.hpp index 5c257191e12db..18ec9ffa991b8 100644 --- a/src/hotspot/share/classfile/packageEntry.hpp +++ b/src/hotspot/share/classfile/packageEntry.hpp @@ -27,6 +27,7 @@ #include "classfile/moduleEntry.hpp" #include "oops/symbol.hpp" +#include "oops/symbolHandle.hpp" #include "runtime/atomic.hpp" #include "utilities/growableArray.hpp" #include "utilities/resourceHash.hpp" @@ -236,8 +237,8 @@ class PackageEntry : public CHeapObj { // The PackageEntryTable is a Hashtable containing a list of all packages defined // by a particular class loader. Each package is represented as a PackageEntry node. class PackageEntryTable : public CHeapObj { - ResourceHashtable _table; + ResourceHashtable _table; public: PackageEntryTable(); ~PackageEntryTable(); diff --git a/src/hotspot/share/classfile/placeholders.cpp b/src/hotspot/share/classfile/placeholders.cpp index 76f17007bad39..c0f4c03462501 100644 --- a/src/hotspot/share/classfile/placeholders.cpp +++ b/src/hotspot/share/classfile/placeholders.cpp @@ -29,12 +29,13 @@ #include "logging/logTag.hpp" #include "logging/logStream.hpp" #include "memory/resourceArea.hpp" +#include "oops/symbolHandle.hpp" #include "runtime/javaThread.hpp" #include "runtime/mutexLocker.hpp" #include "utilities/resourceHash.hpp" class PlaceholderKey { - Symbol* _name; + SymbolHandle _name; ClassLoaderData* _loader_data; public: PlaceholderKey(Symbol* name, ClassLoaderData* l) : _name(name), _loader_data(l) {} @@ -204,8 +205,6 @@ PlaceholderEntry* add_entry(Symbol* class_name, ClassLoaderData* loader_data, PlaceholderEntry entry; entry.set_supername(supername); PlaceholderKey key(class_name, loader_data); - // Since we're storing this key in the hashtable, we need to increment the refcount. - class_name->increment_refcount(); bool created; PlaceholderEntry* table_copy = _placeholders.put_if_absent(key, entry, &created); assert(created, "better be absent"); @@ -218,8 +217,6 @@ void remove_entry(Symbol* class_name, ClassLoaderData* loader_data) { PlaceholderKey key(class_name, loader_data); _placeholders.remove(key); - // Decrement the refcount in key, since it's no longer in the table. - class_name->decrement_refcount(); } diff --git a/src/hotspot/share/classfile/symbolTable.hpp b/src/hotspot/share/classfile/symbolTable.hpp index 0a03bdd28caae..1913eb9a1bf2e 100644 --- a/src/hotspot/share/classfile/symbolTable.hpp +++ b/src/hotspot/share/classfile/symbolTable.hpp @@ -28,63 +28,14 @@ #include "memory/allocation.hpp" #include "memory/padded.hpp" #include "oops/symbol.hpp" +#include "oops/symbolHandle.hpp" #include "utilities/tableStatistics.hpp" class JavaThread; template class GrowableArray; -// 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. -// TempNewSymbol can therefore be used to properly hold a newly created or referenced -// Symbol* temporarily in scope. -// -// Routines in SymbolTable will initialize the reference count of a Symbol* before -// it becomes "managed" by TempNewSymbol instances. As a handle class, TempNewSymbol -// needs to maintain proper reference counting in context of copy semantics. -// -// In SymbolTable, new_symbol() will create a Symbol* if not already in the -// symbol table and add to the symbol's reference count. -// probe() and lookup_only() will increment the refcount if symbol is found. -class TempNewSymbol : public StackObj { - Symbol* _temp; - -public: - TempNewSymbol() : _temp(NULL) {} - - // Conversion from a Symbol* to a TempNewSymbol. - // Does not increment the current reference count. - TempNewSymbol(Symbol *s) : _temp(s) {} - - // Copy constructor increments reference count. - TempNewSymbol(const TempNewSymbol& rhs) : _temp(rhs._temp) { - if (_temp != NULL) { - _temp->increment_refcount(); - } - } - - // Assignment operator uses a c++ trick called copy and swap idiom. - // rhs is passed by value so within the scope of this method it is a copy. - // At method exit it contains the former value of _temp, triggering the correct refcount - // decrement upon destruction. - void operator=(TempNewSymbol rhs) { - Symbol* tmp = rhs._temp; - rhs._temp = _temp; - _temp = tmp; - } - - // Decrement reference counter so it can go away if it's unused - ~TempNewSymbol() { - if (_temp != NULL) { - _temp->decrement_refcount(); - } - } - - // Symbol* conversion operators - Symbol* operator -> () const { return _temp; } - bool operator == (Symbol* o) const { return _temp == o; } - operator Symbol*() { return _temp; } -}; +// TempNewSymbol in symbolHandle.hpp is used with SymbolTable operations, +// so include it here. class CompactHashtableWriter; class SerializeClosure; diff --git a/src/hotspot/share/classfile/systemDictionary.cpp b/src/hotspot/share/classfile/systemDictionary.cpp index f48b9bfb693a5..daae0821c861b 100644 --- a/src/hotspot/share/classfile/systemDictionary.cpp +++ b/src/hotspot/share/classfile/systemDictionary.cpp @@ -115,7 +115,7 @@ class InvokeMethodKey : public StackObj { ResourceHashtable _invoke_method_intrinsic_table; -ResourceHashtable _invoke_method_type_table; +ResourceHashtable _invoke_method_type_table; OopHandle SystemDictionary::_java_system_loader; OopHandle SystemDictionary::_java_platform_loader; diff --git a/src/hotspot/share/interpreter/linkResolver.cpp b/src/hotspot/share/interpreter/linkResolver.cpp index 3b32ae54fd0f4..7dac2a177d450 100644 --- a/src/hotspot/share/interpreter/linkResolver.cpp +++ b/src/hotspot/share/interpreter/linkResolver.cpp @@ -27,7 +27,6 @@ #include "cds/archiveUtils.hpp" #include "classfile/defaultMethods.hpp" #include "classfile/javaClasses.hpp" -#include "classfile/symbolTable.hpp" #include "classfile/systemDictionary.hpp" #include "classfile/vmClasses.hpp" #include "classfile/vmSymbols.hpp" @@ -49,6 +48,7 @@ #include "oops/objArrayKlass.hpp" #include "oops/objArrayOop.hpp" #include "oops/oop.inline.hpp" +#include "oops/symbolHandle.hpp" #include "prims/methodHandles.hpp" #include "runtime/fieldDescriptor.inline.hpp" #include "runtime/frame.inline.hpp" diff --git a/src/hotspot/share/oops/instanceKlass.cpp b/src/hotspot/share/oops/instanceKlass.cpp index a29800c9c1a55..0b207b6ec921b 100644 --- a/src/hotspot/share/oops/instanceKlass.cpp +++ b/src/hotspot/share/oops/instanceKlass.cpp @@ -34,7 +34,6 @@ #include "classfile/classLoaderData.inline.hpp" #include "classfile/javaClasses.hpp" #include "classfile/moduleEntry.hpp" -#include "classfile/symbolTable.hpp" #include "classfile/systemDictionary.hpp" #include "classfile/systemDictionaryShared.hpp" #include "classfile/verifier.hpp" diff --git a/src/hotspot/share/oops/symbolHandle.hpp b/src/hotspot/share/oops/symbolHandle.hpp new file mode 100644 index 0000000000000..b12ba59caced1 --- /dev/null +++ b/src/hotspot/share/oops/symbolHandle.hpp @@ -0,0 +1,97 @@ +/* + * Copyright (c) 2022, 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 + * 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. + * + */ + +#ifndef SHARE_OOPS_SYMBOLHANDLE_HPP +#define SHARE_OOPS_SYMBOLHANDLE_HPP + +#include "memory/allocation.hpp" +#include "oops/symbol.hpp" + +// 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. +// TempNewSymbol can therefore be used to properly hold a newly created or referenced +// Symbol* temporarily in scope. +// +// Routines in SymbolTable will initialize the reference count of a Symbol* before +// it becomes "managed" by TempNewSymbol instances. As a handle class, TempNewSymbol +// needs to maintain proper reference counting in context of copy semantics. +// +// In SymbolTable, new_symbol() will create a Symbol* if not already in the +// symbol table and add to the symbol's reference count. +// probe() and lookup_only() will increment the refcount if symbol is found. +template +class SymbolHandleBase : public StackObj { + Symbol* _temp; + +public: + SymbolHandleBase() : _temp(NULL) { } + + // Conversion from a Symbol* to a SymbolHandleBase. + // Does not increment the current reference count if temporary. + SymbolHandleBase(Symbol *s) : _temp(s) { + if (!TEMP) { + assert(s != nullptr, "must not be null"); + s->increment_refcount(); + } + } + + // Copy constructor increments reference count. + SymbolHandleBase(const SymbolHandleBase& rhs) : _temp(rhs._temp) { + Symbol::maybe_increment_refcount(_temp); + } + + // Assignment operator uses a c++ trick called copy and swap idiom. + // rhs is passed by value so within the scope of this method it is a copy. + // At method exit it contains the former value of _temp, triggering the correct refcount + // decrement upon destruction. + void operator=(SymbolHandleBase rhs) { + Symbol* tmp = rhs._temp; + rhs._temp = _temp; + _temp = tmp; + } + + // Decrement reference counter so it can go away if it's unused + ~SymbolHandleBase() { + Symbol::maybe_decrement_refcount(_temp); + } + + // Symbol* conversion operators + Symbol* operator -> () const { return _temp; } + bool operator == (Symbol* o) const { return _temp == o; } + operator Symbol*() const { return _temp; } + + static unsigned int compute_hash(const SymbolHandleBase& name) { + return (unsigned int) name->identity_hash(); + } +}; + +// TempNewSymbol is a temporary holder for a newly created symbol +using TempNewSymbol = SymbolHandleBase; + +// SymbolHandle is a non-temp symbol used to hold a symbol in a semi permanent place, +// like in a hashtable. The only difference is that the constructor increments the refcount. +using SymbolHandle = SymbolHandleBase; + +#endif // SHARE_OOPS_SYMBOLHANDLE_HPP diff --git a/test/hotspot/gtest/utilities/test_resourceHash.cpp b/test/hotspot/gtest/utilities/test_resourceHash.cpp index 8bf26d7a9b121..839e0cb76a705 100644 --- a/test/hotspot/gtest/utilities/test_resourceHash.cpp +++ b/test/hotspot/gtest/utilities/test_resourceHash.cpp @@ -25,6 +25,7 @@ #include "classfile/symbolTable.hpp" #include "memory/allocation.hpp" #include "memory/resourceArea.hpp" +#include "oops/symbolHandle.hpp" #include "unittest.hpp" #include "utilities/debug.hpp" #include "utilities/globalDefinitions.hpp" @@ -289,44 +290,37 @@ TEST_VM_F(GenericResourceHashtableTest, identity_hash_no_rm) { Runner, 1, ResourceObj::C_HEAP>::test(512); } -// Simple ResourceHashtable whose key is a Symbol* and value is an int -// This test is to show that you need to manipulate the refcount of the Symbol to store +// Simple ResourceHashtable whose key is a SymbolHandle and value is an int +// This test is to show that the SymbolHandle will correctly handle the refcounting // in the table. class SimpleResourceHashtableDeleteTest : public ::testing::Test { public: - ResourceHashtable _simple_test_table; + ResourceHashtable _simple_test_table; class SimpleDeleter : public StackObj { public: - bool do_entry(Symbol*& key, int value) { - // We need to decrement the refcount for the key in the delete function. - // Since we incremented the key, in this case, we should decrement it. - key->decrement_refcount(); + bool do_entry(SymbolHandle& key, int value) { return true; } }; }; TEST_VM_F(SimpleResourceHashtableDeleteTest, simple_remove) { - TempNewSymbol s = SymbolTable::new_symbol("abcdefg_simple"); + TempNewSymbol t = SymbolTable::new_symbol("abcdefg_simple"); + Symbol* s = t; int s_orig_count = s->refcount(); - // Need to increment a Symbol* when you keep it in a table. - s->increment_refcount(); _simple_test_table.put(s, 55); ASSERT_EQ(s->refcount(), s_orig_count + 1) << "refcount should be incremented in table"; // Deleting this value from a hashtable _simple_test_table.remove(s); - // Now decrement the refcount for s since it's no longer in the table. - s->decrement_refcount(); ASSERT_EQ(s->refcount(), s_orig_count) << "refcount should be same as start"; } TEST_VM_F(SimpleResourceHashtableDeleteTest, simple_delete) { - TempNewSymbol s = SymbolTable::new_symbol("abcdefg_simple"); + TempNewSymbol t = SymbolTable::new_symbol("abcdefg_simple"); + Symbol* s = t; int s_orig_count = s->refcount(); - // Need to increment a Symbol* when you keep it in a table. - s->increment_refcount(); _simple_test_table.put(s, 66); ASSERT_EQ(s->refcount(), s_orig_count + 1) << "refcount should be incremented in table"; @@ -336,31 +330,22 @@ TEST_VM_F(SimpleResourceHashtableDeleteTest, simple_delete) { ASSERT_EQ(s->refcount(), s_orig_count) << "refcount should be same as start"; } -// More complicated ResourceHashtable with Symbol* as the key. Since the *same* Symbol is part -// of the value, it's not necessary to maniuplate the refcount of the key, but you must in the value. +// More complicated ResourceHashtable with SymbolHandle in the key. Since the *same* Symbol is part +// of the value, it's not necessary to manipulate the refcount of the key, but you must in the value. +// Luckily SymbolHandle does this. class ResourceHashtableDeleteTest : public ::testing::Test { public: class TestValue : public CHeapObj { - Symbol* _s; + SymbolHandle _s; public: // Never have ctors and dtors fix refcounts without copy ctors and assignment operators! // Unless it's declared and used as a CHeapObj with // NONCOPYABLE(TestValue) - TestValue(Symbol* name) : _s(name) { _s->increment_refcount(); } - TestValue(const TestValue& tv) { _s = tv.s(); _s->increment_refcount(); } - - // Refcounting with assignment operators is tricky. See TempNewSymbol for more information. - // (1) A copy (from) of the argument is created to be passed by value to operator=. This increments - // the refcount of the symbol. - // (2) Exchange the values this->_s and from._s as a trivial pointer exchange. No reference count - // manipulation occurs. this->_s is the desired new value, with its refcount incremented appropriately - // (by the copy that created from). - // (3) The operation completes and from goes out of scope, calling its destructor. This decrements the - // refcount for from._s, which is the _old_ value of this->_s. - TestValue& operator=(TestValue tv) { swap(_s, tv._s); return *this; } - - ~TestValue() { _s->decrement_refcount(); } - Symbol* s() const { return _s; } + + // Using SymbolHandle deals with refcount manipulation so this class doesn't have to + // have dtors, copy ctors and assignment operators to do so. + TestValue(Symbol* name) : _s(name) { } + // Symbol* s() const { return _s; } // needed for conversion from TempNewSymbol to SymbolHandle member }; // ResourceHashtable whose value is a *copy* of TestValue. diff --git a/test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/DynamicSharedSymbols.java b/test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/DynamicSharedSymbols.java index 24305c5a1667d..806313e270e79 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 @@ 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"); output.shouldContain("Dynamic shared symbols:\n"); output.shouldContain("5 65535: Hello\n");