From 1d5fd3c0d2666c6cd71b8ff123a49d83b98a33f4 Mon Sep 17 00:00:00 2001 From: Coleen Phillimore Date: Wed, 8 Feb 2023 23:37:53 +0000 Subject: [PATCH 1/2] 8302108: Clean up placeholder supername code --- src/hotspot/share/classfile/placeholders.cpp | 27 ++++++++------- src/hotspot/share/classfile/placeholders.hpp | 35 ++++++++------------ src/hotspot/share/oops/symbolHandle.hpp | 3 +- 3 files changed, 29 insertions(+), 36 deletions(-) diff --git a/src/hotspot/share/classfile/placeholders.cpp b/src/hotspot/share/classfile/placeholders.cpp index f6614881e0da3..ff9e34d274afe 100644 --- a/src/hotspot/share/classfile/placeholders.cpp +++ b/src/hotspot/share/classfile/placeholders.cpp @@ -191,7 +191,11 @@ bool PlaceholderEntry::remove_seen_thread(JavaThread* thread, PlaceholderTable:: } -// Placeholder methods +void PlaceholderEntry::set_supername(Symbol* supername) { + assert_locked_or_safepoint(SystemDictionary_lock); + assert(_supername == nullptr || _supername->refcount() > 1, "must be referenced also by the loader"); + _supername = supername; +} // Placeholder objects represent classes currently being loaded. // All threads examining the placeholder table must hold the @@ -272,7 +276,6 @@ PlaceholderEntry* PlaceholderTable::find_and_add(Symbol* name, // placeholder is used to track class loading internal states -// placeholder existence now for loading superclass/superinterface // superthreadQ tracks class circularity, while loading superclass/superinterface // loadInstanceThreadQ tracks load_instance_class calls // definer() tracks the single thread that owns define token @@ -283,21 +286,21 @@ PlaceholderEntry* PlaceholderTable::find_and_add(Symbol* name, // On removal: if definer and all queues empty, remove entry // Note: you can be in both placeholders and systemDictionary // Therefore - must always check SD first -// Ignores the case where entry is not found void PlaceholderTable::find_and_remove(Symbol* name, ClassLoaderData* loader_data, classloadAction action, JavaThread* thread) { assert_locked_or_safepoint(SystemDictionary_lock); PlaceholderEntry* probe = get_entry(name, loader_data); - if (probe != nullptr) { - log(name, probe, "find_and_remove", action); - probe->remove_seen_thread(thread, action); - // If no other threads using this entry, and this thread is not using this entry for other states - if ((probe->superThreadQ() == nullptr) && (probe->loadInstanceThreadQ() == nullptr) - && (probe->defineThreadQ() == nullptr) && (probe->definer() == nullptr)) { - probe->clear_supername(); - remove_entry(name, loader_data); - } + assert(probe != nullptr, "must find an entry"); + log(name, probe, "find_and_remove", action); + probe->remove_seen_thread(thread, action); + if (probe->superThreadQ() == nullptr) { + probe->set_supername(nullptr); + } + // If no other threads using this entry, and this thread is not using this entry for other states + if ((probe->superThreadQ() == nullptr) && (probe->loadInstanceThreadQ() == nullptr) + && (probe->defineThreadQ() == nullptr) && (probe->definer() == nullptr)) { + remove_entry(name, loader_data); } } diff --git a/src/hotspot/share/classfile/placeholders.hpp b/src/hotspot/share/classfile/placeholders.hpp index bc5a4c373a72c..9d4d6aed640c6 100644 --- a/src/hotspot/share/classfile/placeholders.hpp +++ b/src/hotspot/share/classfile/placeholders.hpp @@ -25,6 +25,8 @@ #ifndef SHARE_CLASSFILE_PLACEHOLDERS_HPP #define SHARE_CLASSFILE_PLACEHOLDERS_HPP +#include "oops/symbolHandle.hpp" + class PlaceholderEntry; class Thread; class ClassLoaderData; @@ -80,7 +82,7 @@ class SeenThread; class PlaceholderEntry { friend class PlaceholderTable; private: - Symbol* _supername; + SymbolHandle _supername; JavaThread* _definer; // owner of define token InstanceKlass* _instanceKlass; // InstanceKlass from successful define SeenThread* _superThreadQ; // doubly-linked queue of Threads loading a superclass for this class @@ -98,23 +100,21 @@ class PlaceholderEntry { void add_seen_thread(JavaThread* thread, PlaceholderTable::classloadAction action); bool remove_seen_thread(JavaThread* thread, PlaceholderTable::classloadAction action); + SeenThread* superThreadQ() const { return _superThreadQ; } + void set_superThreadQ(SeenThread* SeenThread) { _superThreadQ = SeenThread; } + + SeenThread* loadInstanceThreadQ() const { return _loadInstanceThreadQ; } + void set_loadInstanceThreadQ(SeenThread* SeenThread) { _loadInstanceThreadQ = SeenThread; } + + SeenThread* defineThreadQ() const { return _defineThreadQ; } + void set_defineThreadQ(SeenThread* SeenThread) { _defineThreadQ = SeenThread; } public: PlaceholderEntry() : - _supername(nullptr), _definer(nullptr), _instanceKlass(nullptr), + _definer(nullptr), _instanceKlass(nullptr), _superThreadQ(nullptr), _loadInstanceThreadQ(nullptr), _defineThreadQ(nullptr) { } Symbol* supername() const { return _supername; } - void set_supername(Symbol* supername) { - if (supername != _supername) { - Symbol::maybe_decrement_refcount(_supername); - _supername = supername; - Symbol::maybe_increment_refcount(_supername); - } - } - void clear_supername() { - Symbol::maybe_decrement_refcount(_supername); - _supername = nullptr; - } + void set_supername(Symbol* supername); JavaThread* definer() const {return _definer; } void set_definer(JavaThread* definer) { _definer = definer; } @@ -122,15 +122,6 @@ class PlaceholderEntry { InstanceKlass* instance_klass() const {return _instanceKlass; } void set_instance_klass(InstanceKlass* ik) { _instanceKlass = ik; } - SeenThread* superThreadQ() const { return _superThreadQ; } - void set_superThreadQ(SeenThread* SeenThread) { _superThreadQ = SeenThread; } - - SeenThread* loadInstanceThreadQ() const { return _loadInstanceThreadQ; } - void set_loadInstanceThreadQ(SeenThread* SeenThread) { _loadInstanceThreadQ = SeenThread; } - - SeenThread* defineThreadQ() const { return _defineThreadQ; } - void set_defineThreadQ(SeenThread* SeenThread) { _defineThreadQ = SeenThread; } - bool super_load_in_progress() { return (_superThreadQ != nullptr); } diff --git a/src/hotspot/share/oops/symbolHandle.hpp b/src/hotspot/share/oops/symbolHandle.hpp index b12ba59caced1..1b073c1d137ab 100644 --- a/src/hotspot/share/oops/symbolHandle.hpp +++ b/src/hotspot/share/oops/symbolHandle.hpp @@ -52,8 +52,7 @@ class SymbolHandleBase : public StackObj { // 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(); + Symbol::maybe_increment_refcount(_temp); } } From 378170e628dec7a0145e0b4edc1583518ab95ec5 Mon Sep 17 00:00:00 2001 From: Coleen Phillimore Date: Fri, 10 Feb 2023 19:06:05 +0000 Subject: [PATCH 2/2] Fix test for cleanup for nulling out supername when no longer used. --- test/hotspot/gtest/classfile/test_placeholders.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/hotspot/gtest/classfile/test_placeholders.cpp b/test/hotspot/gtest/classfile/test_placeholders.cpp index 612482ba8b9b8..9fa06d49f7306 100644 --- a/test/hotspot/gtest/classfile/test_placeholders.cpp +++ b/test/hotspot/gtest/classfile/test_placeholders.cpp @@ -63,14 +63,12 @@ TEST_VM(PlaceholderTable, supername) { PlaceholderTable::find_and_add(D, loader_data, super_action, interf, T2); PlaceholderTable::find_and_remove(D, loader_data, super_action, T2); - ASSERT_EQ(interf->refcount(), 3) << "supername isn't replaced until super set"; + ASSERT_EQ(interf->refcount(), 1) << "supername is replaced with null"; // Add placeholder to the table for loading A and super, and D also loading super PlaceholderTable::find_and_add(A, loader_data, super_action, super, THREAD); PlaceholderTable::find_and_add(D, loader_data, super_action, super, T2); - ASSERT_EQ(interf->refcount(), 1) << "now should be one"; - // Another thread comes in and finds A loading Super PlaceholderEntry* placeholder = PlaceholderTable::get_entry(A, loader_data); SymbolHandle supername = placeholder->supername();