8259242: Remove ProtectionDomainSet_lock

Reviewed-by: dholmes, pchilanomate

Author: coleenp

src/hotspot/share/classfile/dictionary.cpp

@@ -44,6 +44,7 @@
 #include "runtime/javaCalls.hpp"
 #include "runtime/mutexLocker.hpp"
 #include "runtime/safepointVerifiers.hpp"
+#include "utilities/growableArray.hpp"
 #include "utilities/hashtable.inline.hpp"
 
 // Optimization: if any dictionary needs resizing, we set this flag,
@@ -78,20 +79,19 @@ Dictionary::~Dictionary() {
 
 DictionaryEntry* Dictionary::new_entry(unsigned int hash, InstanceKlass* klass) {
   DictionaryEntry* entry = (DictionaryEntry*)Hashtable<InstanceKlass*, mtClass>::new_entry(hash, klass);
-  entry->set_pd_set(NULL);
+  entry->release_set_pd_set(NULL);
   assert(klass->is_instance_klass(), "Must be");
   return entry;
 }
 
-
 void Dictionary::free_entry(DictionaryEntry* entry) {
   // avoid recursion when deleting linked list
   // pd_set is accessed during a safepoint.
   // This doesn't require a lock because nothing is reading this
   // entry anymore.  The ClassLoader is dead.
-  while (entry->pd_set() != NULL) {
-    ProtectionDomainEntry* to_delete = entry->pd_set();
-    entry->set_pd_set(to_delete->next());
+  while (entry->pd_set_acquire() != NULL) {
+    ProtectionDomainEntry* to_delete = entry->pd_set_acquire();
+    entry->release_set_pd_set(to_delete->next_acquire());
     delete to_delete;
   }
   BasicHashtable<mtClass>::free_entry(entry);
@@ -141,15 +141,26 @@ bool DictionaryEntry::is_valid_protection_domain(Handle protection_domain) {
         : contains_protection_domain(protection_domain());
 }
 
+// Reading the pd_set on each DictionaryEntry is lock free and cannot safepoint.
+// Adding and deleting entries is under the SystemDictionary_lock
+// Deleting unloaded entries on ClassLoaderData for dictionaries that are not unloaded
+// is a three step process:
+//     moving the entries to a separate list, handshake to wait for
+//     readers to complete (see NSV here), and then actually deleting the entries.
+// Deleting entries is done by the ServiceThread when triggered by class unloading.
+
 bool DictionaryEntry::contains_protection_domain(oop protection_domain) const {
+  assert(Thread::current()->is_Java_thread() || SafepointSynchronize::is_at_safepoint(),
+         "can only be called by a JavaThread or at safepoint");
+  // This cannot safepoint while reading the protection domain set.
+  NoSafepointVerifier nsv;
 #ifdef ASSERT
   if (protection_domain == instance_klass()->protection_domain()) {
-    MutexLocker ml(ProtectionDomainSet_lock, Mutex::_no_safepoint_check_flag);
     // Ensure this doesn't show up in the pd_set (invariant)
     bool in_pd_set = false;
-    for (ProtectionDomainEntry* current = pd_set();
+    for (ProtectionDomainEntry* current = pd_set_acquire();
                                 current != NULL;
-                                current = current->next()) {
+                                current = current->next_acquire()) {
       if (current->object_no_keepalive() == protection_domain) {
         in_pd_set = true;
         break;
@@ -167,30 +178,23 @@ bool DictionaryEntry::contains_protection_domain(oop protection_domain) const {
     return true;
   }
 
-  // Lock the pd_set list.  This lock cannot safepoint since the caller holds
-  // a Dictionary entry, which can be moved if the Dictionary is resized.
-  MutexLocker ml(ProtectionDomainSet_lock, Mutex::_no_safepoint_check_flag);
-  for (ProtectionDomainEntry* current = pd_set();
+  for (ProtectionDomainEntry* current = pd_set_acquire();
                               current != NULL;
-                              current = current->next()) {
+                              current = current->next_acquire()) {
     if (current->object_no_keepalive() == protection_domain) {
       return true;
     }
   }
   return false;
 }
 
-
 void DictionaryEntry::add_protection_domain(Dictionary* dict, Handle protection_domain) {
   assert_locked_or_safepoint(SystemDictionary_lock);
   if (!contains_protection_domain(protection_domain())) {
     ProtectionDomainCacheEntry* entry = SystemDictionary::cache_get(protection_domain);
-    // The pd_set in the dictionary entry is protected by a low level lock.
-    // With concurrent PD table cleanup, these links could be broken.
-    MutexLocker ml(ProtectionDomainSet_lock, Mutex::_no_safepoint_check_flag);
-    ProtectionDomainEntry* new_head =
-                new ProtectionDomainEntry(entry, pd_set());
-    set_pd_set(new_head);
+    // Additions and deletions hold the SystemDictionary_lock, readers are lock-free
+    ProtectionDomainEntry* new_head = new ProtectionDomainEntry(entry, _pd_set);
+    release_set_pd_set(new_head);
   }
   LogTarget(Trace, protectiondomain) lt;
   if (lt.is_enabled()) {
@@ -420,8 +424,9 @@ void Dictionary::validate_protection_domain(unsigned int name_hash,
 
 // During class loading we may have cached a protection domain that has
 // since been unreferenced, so this entry should be cleared.
-void Dictionary::clean_cached_protection_domains() {
-  assert_locked_or_safepoint(SystemDictionary_lock);
+void Dictionary::clean_cached_protection_domains(GrowableArray<ProtectionDomainEntry*>* delete_list) {
+  assert(Thread::current()->is_Java_thread(), "only called by JavaThread");
+  assert_lock_strong(SystemDictionary_lock);
   assert(!loader_data()->has_class_mirror_holder(), "cld should have a ClassLoader holder not a Class holder");
 
   if (loader_data()->is_the_null_class_loader_data()) {
@@ -435,8 +440,7 @@ void Dictionary::clean_cached_protection_domains() {
                           probe = probe->next()) {
       Klass* e = probe->instance_klass();
 
-      MutexLocker ml(ProtectionDomainSet_lock, Mutex::_no_safepoint_check_flag);
-      ProtectionDomainEntry* current = probe->pd_set();
+      ProtectionDomainEntry* current = probe->pd_set_acquire();
       ProtectionDomainEntry* prev = NULL;
       while (current != NULL) {
         if (current->object_no_keepalive() == NULL) {
@@ -450,18 +454,19 @@ void Dictionary::clean_cached_protection_domains() {
             ls.print(" loading: "); probe->instance_klass()->print_value_on(&ls);
             ls.cr();
           }
-          if (probe->pd_set() == current) {
-            probe->set_pd_set(current->next());
+          if (probe->pd_set_acquire() == current) {
+            probe->release_set_pd_set(current->next_acquire());
           } else {
             assert(prev != NULL, "should be set by alive entry");
-            prev->set_next(current->next());
+            prev->release_set_next(current->next_acquire());
           }
-          ProtectionDomainEntry* to_delete = current;
-          current = current->next();
-          delete to_delete;
+          // Mark current for deletion but in the meantime it can still be
+          // traversed.
+          delete_list->push(current);
+          current = current->next_acquire();
         } else {
           prev = current;
-          current = current->next();
+          current = current->next_acquire();
         }
       }
     }
@@ -552,20 +557,20 @@ void SymbolPropertyTable::free_entry(SymbolPropertyEntry* entry) {
 }
 
 void DictionaryEntry::verify_protection_domain_set() {
-  MutexLocker ml(ProtectionDomainSet_lock, Mutex::_no_safepoint_check_flag);
-  for (ProtectionDomainEntry* current = pd_set(); // accessed at a safepoint
+  assert(SafepointSynchronize::is_at_safepoint(), "must only be called as safepoint");
+  for (ProtectionDomainEntry* current = pd_set_acquire(); // accessed at a safepoint
                               current != NULL;
-                              current = current->_next) {
-    guarantee(oopDesc::is_oop_or_null(current->_pd_cache->object_no_keepalive()), "Invalid oop");
+                              current = current->next_acquire()) {
+    guarantee(oopDesc::is_oop_or_null(current->object_no_keepalive()), "Invalid oop");
   }
 }
 
 void DictionaryEntry::print_count(outputStream *st) {
-  MutexLocker ml(ProtectionDomainSet_lock, Mutex::_no_safepoint_check_flag);
+  assert_locked_or_safepoint(SystemDictionary_lock);
   int count = 0;
-  for (ProtectionDomainEntry* current = pd_set();  // accessed inside SD lock
+  for (ProtectionDomainEntry* current = pd_set_acquire();
                               current != NULL;
-                              current = current->_next) {
+                              current = current->next_acquire()) {
     count++;
   }
   st->print_cr("pd set count = #%d", count);
@@ -596,6 +601,8 @@ void Dictionary::print_on(outputStream* st) const {
         // redundant and obvious.
         st->print(", ");
         cld->print_value_on(st);
+        st->print(", ");
+        probe->print_count(st);
       }
       st->cr();
     }

src/hotspot/share/classfile/dictionary.hpp

@@ -33,6 +33,7 @@
 
 class DictionaryEntry;
 class ProtectionDomainEntry;
+template <typename T> class GrowableArray;
 
 //~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 // The data structure for the class loader data dictionaries.
@@ -67,7 +68,7 @@ class Dictionary : public Hashtable<InstanceKlass*, mtClass> {
   void all_entries_do(KlassClosure* closure);
   void classes_do(MetaspaceClosure* it);
 
-  void clean_cached_protection_domains();
+  void clean_cached_protection_domains(GrowableArray<ProtectionDomainEntry*>* delete_list);
 
   // Protection domains
   InstanceKlass* find(unsigned int hash, Symbol* name, Handle protection_domain);
@@ -111,18 +112,11 @@ class DictionaryEntry : public HashtableEntry<InstanceKlass*, mtClass> {
   // Contains the set of approved protection domains that can access
   // this dictionary entry.
   //
-  // This protection domain set is a set of tuples:
-  //
-  // (InstanceKlass C, initiating class loader ICL, Protection Domain PD)
-  //
   // [Note that C.protection_domain(), which is stored in the java.lang.Class
   // mirror of C, is NOT the same as PD]
   //
-  // If such an entry (C, ICL, PD) exists in the table, it means that
-  // it is okay for a class Foo to reference C, where
-  //
-  //    Foo.protection_domain() == PD, and
-  //    Foo's defining class loader == ICL
+  // If an entry for PD exists in the list, it means that
+  // it is okay for a caller class to reference the class in this dictionary entry.
   //
   // The usage of the PD set can be seen in SystemDictionary::validate_protection_domain()
   // It is essentially a cache to avoid repeated Java up-calls to
@@ -147,8 +141,8 @@ class DictionaryEntry : public HashtableEntry<InstanceKlass*, mtClass> {
     return (DictionaryEntry**)HashtableEntry<InstanceKlass*, mtClass>::next_addr();
   }
 
-  ProtectionDomainEntry* pd_set() const            { return _pd_set; }
-  void set_pd_set(ProtectionDomainEntry* new_head) {  _pd_set = new_head; }
+  ProtectionDomainEntry* pd_set_acquire() const            { return Atomic::load_acquire(&_pd_set); }
+  void release_set_pd_set(ProtectionDomainEntry* entry)    { Atomic::release_store(&_pd_set, entry); }
 
   // Tells whether the initiating class' protection domain can access the klass in this entry
   inline bool is_valid_protection_domain(Handle protection_domain);

src/hotspot/share/classfile/protectionDomainCache.cpp

@@ -34,6 +34,8 @@
 #include "memory/universe.hpp"
 #include "oops/oop.inline.hpp"
 #include "oops/weakHandle.inline.hpp"
+#include "runtime/atomic.hpp"
+#include "utilities/growableArray.hpp"
 #include "utilities/hashtable.inline.hpp"
 
 unsigned int ProtectionDomainCacheTable::compute_hash(Handle protection_domain) {
@@ -58,29 +60,75 @@ void ProtectionDomainCacheTable::trigger_cleanup() {
 }
 
 class CleanProtectionDomainEntries : public CLDClosure {
+  GrowableArray<ProtectionDomainEntry*>* _delete_list;
+ public:
+  CleanProtectionDomainEntries(GrowableArray<ProtectionDomainEntry*>* delete_list) :
+                               _delete_list(delete_list) {}
+
   void do_cld(ClassLoaderData* data) {
     Dictionary* dictionary = data->dictionary();
     if (dictionary != NULL) {
-      dictionary->clean_cached_protection_domains();
+      dictionary->clean_cached_protection_domains(_delete_list);
     }
   }
 };
 
+static GrowableArray<ProtectionDomainEntry*>* _delete_list = NULL;
+
+class HandshakeForPD : public HandshakeClosure {
+ public:
+  HandshakeForPD() : HandshakeClosure("HandshakeForPD") {}
+
+  void do_thread(Thread* thread) {
+    log_trace(protectiondomain)("HandshakeForPD::do_thread: thread="
+                                INTPTR_FORMAT, p2i(thread));
+  }
+};
+
+static void purge_deleted_entries() {
+  // If there are any deleted entries, Handshake-all then they'll be
+  // safe to remove since traversing the pd_set list does not stop for
+  // safepoints and only JavaThreads will read the pd_set.
+  // This is actually quite rare because the protection domain is generally associated
+  // with the caller class and class loader, which if still alive will keep this
+  // protection domain entry alive.
+  if (_delete_list->length() >= 10) {
+    HandshakeForPD hs_pd;
+    Handshake::execute(&hs_pd);
+
+    for (int i = _delete_list->length() - 1; i >= 0; i--) {
+      ProtectionDomainEntry* entry = _delete_list->at(i);
+      _delete_list->remove_at(i);
+      delete entry;
+    }
+    assert(_delete_list->length() == 0, "should be cleared");
+  }
+}
+
 void ProtectionDomainCacheTable::unlink() {
   // The dictionary entries _pd_set field should be null also, so nothing to do.
   assert(java_lang_System::allow_security_manager(), "should not be called otherwise");
 
+  // Create a list for holding deleted entries
+  if (_delete_list == NULL) {
+    _delete_list = new (ResourceObj::C_HEAP, mtClass)
+                       GrowableArray<ProtectionDomainEntry*>(20, mtClass);
+  }
+
   {
     // First clean cached pd lists in loaded CLDs
     // It's unlikely, but some loaded classes in a dictionary might
     // point to a protection_domain that has been unloaded.
     // The dictionary pd_set points at entries in the ProtectionDomainCacheTable.
     MutexLocker ml(ClassLoaderDataGraph_lock);
     MutexLocker mldict(SystemDictionary_lock);  // need both.
-    CleanProtectionDomainEntries clean;
+    CleanProtectionDomainEntries clean(_delete_list);
     ClassLoaderDataGraph::loaded_cld_do(&clean);
   }
 
+  // Purge any deleted entries outside of the SystemDictionary_lock.
+  purge_deleted_entries();
+
   MutexLocker ml(SystemDictionary_lock);
   int oops_removed = 0;
   for (int i = 0; i < table_size(); ++i) {
@@ -129,10 +177,6 @@ oop ProtectionDomainCacheEntry::object() {
   return literal().resolve();
 }
 
-oop ProtectionDomainEntry::object() {
-  return _pd_cache->object();
-}
-
 // The object_no_keepalive() call peeks at the phantomly reachable oop without
 // keeping it alive. This is okay to do in the VM thread state if it is not
 // leaked out to become strongly reachable.

src/hotspot/share/classfile/protectionDomainCache.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2017, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2017, 2021, 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
@@ -27,7 +27,7 @@
 
 #include "oops/oop.hpp"
 #include "oops/weakHandle.hpp"
-#include "memory/iterator.hpp"
+#include "runtime/atomic.hpp"
 #include "utilities/hashtable.hpp"
 
 // This class caches the approved protection domains that can access loaded classes.
@@ -62,8 +62,6 @@ class ProtectionDomainCacheEntry : public HashtableEntry<WeakHandle, mtClass> {
 // The amount of different protection domains used is typically magnitudes smaller
 // than the number of system dictionary entries (loaded classes).
 class ProtectionDomainCacheTable : public Hashtable<WeakHandle, mtClass> {
-  friend class VMStructs;
-private:
   ProtectionDomainCacheEntry* bucket(int i) const {
     return (ProtectionDomainCacheEntry*) Hashtable<WeakHandle, mtClass>::bucket(i);
   }
@@ -104,20 +102,17 @@ class ProtectionDomainCacheTable : public Hashtable<WeakHandle, mtClass> {
 };
 
 
+// This describes the linked list protection domain for each DictionaryEntry in pd_set.
 class ProtectionDomainEntry :public CHeapObj<mtClass> {
-  friend class VMStructs;
- public:
-  ProtectionDomainEntry* _next;
   ProtectionDomainCacheEntry* _pd_cache;
+  ProtectionDomainEntry* volatile _next;
+ public:
 
-  ProtectionDomainEntry(ProtectionDomainCacheEntry* pd_cache, ProtectionDomainEntry* next) {
-    _pd_cache = pd_cache;
-    _next     = next;
-  }
+  ProtectionDomainEntry(ProtectionDomainCacheEntry* pd_cache,
+                        ProtectionDomainEntry* head) : _pd_cache(pd_cache), _next(head) {}
 
-  ProtectionDomainEntry* next() { return _next; }
-  void set_next(ProtectionDomainEntry* entry) { _next = entry; }
-  oop object();
+  ProtectionDomainEntry* next_acquire() { return Atomic::load_acquire(&_next); }
+  void release_set_next(ProtectionDomainEntry* entry) { Atomic::release_store(&_next, entry); }
   oop object_no_keepalive();
 };
 #endif // SHARE_CLASSFILE_PROTECTIONDOMAINCACHE_HPP

src/hotspot/share/runtime/mutexLocker.cpp

@@ -42,7 +42,6 @@
 Mutex*   Patching_lock                = NULL;
 Mutex*   CompiledMethod_lock          = NULL;
 Monitor* SystemDictionary_lock        = NULL;
-Mutex*   ProtectionDomainSet_lock     = NULL;
 Mutex*   SharedDictionary_lock        = NULL;
 Mutex*   Module_lock                  = NULL;
 Mutex*   CompiledIC_lock              = NULL;
@@ -261,7 +260,6 @@ void mutex_init() {
   def(JmethodIdCreation_lock       , PaddedMutex  , special-2,   true,  _safepoint_check_never); // used for creating jmethodIDs.
 
   def(SystemDictionary_lock        , PaddedMonitor, leaf,        true,  _safepoint_check_always);
-  def(ProtectionDomainSet_lock     , PaddedMutex  , leaf-1,      true,  _safepoint_check_never);
   def(SharedDictionary_lock        , PaddedMutex  , leaf,        true,  _safepoint_check_always);
   def(Module_lock                  , PaddedMutex  , leaf+2,      false, _safepoint_check_always);
   def(InlineCacheBuffer_lock       , PaddedMutex  , leaf,        true,  _safepoint_check_never);

src/hotspot/share/runtime/mutexLocker.hpp

@@ -34,7 +34,6 @@
 extern Mutex*   Patching_lock;                   // a lock used to guard code patching of compiled code
 extern Mutex*   CompiledMethod_lock;             // a lock used to guard a compiled method and OSR queues
 extern Monitor* SystemDictionary_lock;           // a lock on the system dictionary
-extern Mutex*   ProtectionDomainSet_lock;        // a lock on the pd_set list in the system dictionary
 extern Mutex*   SharedDictionary_lock;           // a lock on the CDS shared dictionary
 extern Mutex*   Module_lock;                     // a lock on module and package related data structures
 extern Mutex*   CompiledIC_lock;                 // a lock used to guard compiled IC patching and access

test/hotspot/jtreg/runtime/Dictionary/ClassForName.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2018, 2021, 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
@@ -37,7 +37,7 @@ public class ClassForName {
 
     public ClassForName() {
         try {
-            // class_loader = URLClassLoader, protection_domain = ClassForName.getProtectionDomain()
+            // class_loader = App$ClassLoader, protection_domain = ClassForName.getProtectionDomain()
             Class.forName(java.util.List.class.getName(), false,
                           ClassLoader.getSystemClassLoader());
         } catch (Throwable e) {

test/hotspot/jtreg/runtime/Dictionary/ProtectionDomainCacheTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2018, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2018, 2021, 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
@@ -84,7 +84,9 @@ public static void main(final String[] args) throws Exception {
                                                               CLASSFILENAME);
             Files.delete(classFile);
 
-            loadAndRun(jarFilePath);
+            for (int i = 0; i < 20; i++) {
+                loadAndRun(jarFilePath);
+            }
 
             // Give the GC a chance to unload protection domains
             for (int i = 0; i < 100; i++) {
@@ -101,11 +103,12 @@ public static void main(String args[]) throws Exception {
                                       "-XX:+UnlockDiagnosticVMOptions",
                                       "-XX:VerifySubSet=dictionary",
                                       "-XX:+VerifyAfterGC",
-                                      "-Xlog:gc+verify,protectiondomain=debug",
+                                      "-Xlog:gc+verify,protectiondomain=trace",
                                       "-Djava.security.manager",
                                       Test.class.getName());
         OutputAnalyzer output = new OutputAnalyzer(pb.start());
         output.shouldContain("PD in set is not alive");
+        output.shouldContain("HandshakeForPD::do_thread");
         output.shouldHaveExitValue(0);
     }
 }

test/hotspot/jtreg/vmTestbase/vm/mlvm/mixed/stress/java/findDeadlock/TestDescription.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2018, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2018, 2021, 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
@@ -43,7 +43,14 @@
  * @comment build test class and indify classes
  * @build vm.mlvm.mixed.stress.java.findDeadlock.INDIFY_Test
  * @run driver vm.mlvm.share.IndifiedClassesBuilder
- *
  * @run main/othervm -Xlog:gc,safepoint vm.mlvm.mixed.stress.java.findDeadlock.INDIFY_Test
+ *
+ * To see code that takes more time to safepoint run with:
+ * main/othervm -XX:+SafepointTimeout -XX:+UnlockDiagnosticVMOptions
+ *                   -XX:+AbortVMOnSafepointTimeout
+ *                   -XX:SafepointTimeoutDelay=500
+ *                   -XX:+PrintSystemDictionaryAtExit
+ *                   -Xlog:gc,safepoint
+ *                   vm.mlvm.mixed.stress.java.findDeadlock.INDIFY_Test
  */
 

test/hotspot/jtreg/vmTestbase/vm/mlvm/tools/Indify.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2010, 2021, 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
@@ -109,7 +109,7 @@ public static void main(String... av) throws IOException {
     public boolean keepgoing = false;
     public boolean expandProperties = false;
     public boolean overwrite = false;
-    public boolean quiet = false;
+    public boolean quiet = true;
     public boolean verbose = false;
     public boolean transitionalJSR292 = true;  // default to false later
     public boolean all = false;