8302108: Clean up placeholder supername code

Reviewed-by: dholmes, iklam

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);
   }
 }
 

src/hotspot/share/classfile/placeholders.hpp

@@ -25,7 +25,7 @@
 #ifndef SHARE_CLASSFILE_PLACEHOLDERS_HPP
 #define SHARE_CLASSFILE_PLACEHOLDERS_HPP
 
-#include "oops/symbol.hpp"
+#include "oops/symbolHandle.hpp"
 
 class PlaceholderEntry;
 class Thread;
@@ -81,7 +81,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
@@ -99,30 +99,6 @@ class PlaceholderEntry {
   void add_seen_thread(JavaThread* thread, PlaceholderTable::classloadAction action);
   bool remove_seen_thread(JavaThread* thread, PlaceholderTable::classloadAction action);
 
- public:
-  PlaceholderEntry() :
-     _supername(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;
-  }
-
-  JavaThread*        definer()             const {return _definer; }
-  void               set_definer(JavaThread* definer) { _definer = definer; }
-
-  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; }
 
@@ -131,6 +107,19 @@ class PlaceholderEntry {
 
   SeenThread*        defineThreadQ()       const { return _defineThreadQ; }
   void               set_defineThreadQ(SeenThread* SeenThread) { _defineThreadQ = SeenThread; }
+ public:
+  PlaceholderEntry() :
+     _definer(nullptr), _instanceKlass(nullptr),
+     _superThreadQ(nullptr), _loadInstanceThreadQ(nullptr), _defineThreadQ(nullptr) { }
+
+  Symbol*            supername()           const { return _supername; }
+  void               set_supername(Symbol* supername);
+
+  JavaThread*        definer()             const {return _definer; }
+  void               set_definer(JavaThread* definer) { _definer = definer; }
+
+  InstanceKlass*     instance_klass()      const {return _instanceKlass; }
+  void               set_instance_klass(InstanceKlass* ik) { _instanceKlass = ik; }
 
   bool super_load_in_progress() {
      return (_superThreadQ != nullptr);

src/hotspot/share/oops/symbolHandle.hpp

@@ -52,8 +52,7 @@ public:
   // 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);
     }
   }
 

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();