8206424: Use locking for cleaning ProtectionDomainTable

ServiceThread is now in charge of cleaning ProtectionDomainTable entries

Reviewed-by: coleenp, iklam

src/hotspot/share/classfile/protectionDomainCache.cpp

@@ -44,11 +44,19 @@ int ProtectionDomainCacheTable::index_for(Handle protection_domain) {
 
 ProtectionDomainCacheTable::ProtectionDomainCacheTable(int table_size)
   : Hashtable<ClassLoaderWeakHandle, mtClass>(table_size, sizeof(ProtectionDomainCacheEntry))
-{
+{   _dead_entries = false;
+    _total_oops_removed = 0;
+}
+
+void ProtectionDomainCacheTable::trigger_cleanup() {
+  MutexLockerEx ml(Service_lock, Mutex::_no_safepoint_check_flag);
+  _dead_entries = true;
+  Service_lock->notify_all();
 }
 
 void ProtectionDomainCacheTable::unlink() {
-  assert(SafepointSynchronize::is_at_safepoint(), "must be");
+  MutexLocker ml(SystemDictionary_lock);
+  int oops_removed = 0;
   for (int i = 0; i < table_size(); ++i) {
     ProtectionDomainCacheEntry** p = bucket_addr(i);
     ProtectionDomainCacheEntry* entry = bucket(i);
@@ -57,7 +65,8 @@ void ProtectionDomainCacheTable::unlink() {
       if (pd != NULL) {
         p = entry->next_addr();
       } else {
-        LogTarget(Debug, protectiondomain) lt;
+        oops_removed++;
+        LogTarget(Debug, protectiondomain, table) lt;
         if (lt.is_enabled()) {
           LogStream ls(lt);
           ls.print_cr("protection domain unlinked at %d", i);
@@ -69,9 +78,12 @@ void ProtectionDomainCacheTable::unlink() {
       entry = *p;
     }
   }
+  _total_oops_removed += oops_removed;
+  _dead_entries = false;
 }
 
 void ProtectionDomainCacheTable::print_on(outputStream* st) const {
+  assert_locked_or_safepoint(SystemDictionary_lock);
   st->print_cr("Protection domain cache table (table_size=%d, classes=%d)",
                table_size(), number_of_entries());
   for (int index = 0; index < table_size(); index++) {
@@ -124,6 +136,7 @@ ProtectionDomainCacheEntry* ProtectionDomainCacheTable::get(Handle protection_do
 }
 
 ProtectionDomainCacheEntry* ProtectionDomainCacheTable::find_entry(int index, Handle protection_domain) {
+  assert_locked_or_safepoint(SystemDictionary_lock);
   for (ProtectionDomainCacheEntry* e = bucket(index); e != NULL; e = e->next()) {
     if (oopDesc::equals(e->object_no_keepalive(), protection_domain())) {
       return e;
@@ -138,6 +151,13 @@ ProtectionDomainCacheEntry* ProtectionDomainCacheTable::add_entry(int index, uns
   assert(index == index_for(protection_domain), "incorrect index?");
   assert(find_entry(index, protection_domain) == NULL, "no double entry");
 
+  LogTarget(Debug, protectiondomain, table) lt;
+  if (lt.is_enabled()) {
+    LogStream ls(lt);
+    ls.print("protection domain added ");
+    protection_domain->print_value_on(&ls);
+    ls.cr();
+  }
   ClassLoaderWeakHandle w = ClassLoaderWeakHandle::create(protection_domain);
   ProtectionDomainCacheEntry* p = new_entry(hash, w);
   Hashtable<ClassLoaderWeakHandle, mtClass>::add_entry(index, p);

src/hotspot/share/classfile/protectionDomainCache.hpp

@@ -85,6 +85,9 @@ private:
   ProtectionDomainCacheEntry* add_entry(int index, unsigned int hash, Handle protection_domain);
   ProtectionDomainCacheEntry* find_entry(int index, Handle protection_domain);
 
+  bool _dead_entries;
+  int _total_oops_removed;
+
 public:
   ProtectionDomainCacheTable(int table_size);
   ProtectionDomainCacheEntry* get(Handle protection_domain);
@@ -93,6 +96,11 @@ public:
 
   void print_on(outputStream* st) const;
   void verify();
+
+  bool has_work() { return _dead_entries; }
+  void trigger_cleanup();
+
+  int removed_entries_count() { return _total_oops_removed; };
 };
 
 

src/hotspot/share/classfile/systemDictionary.cpp

@@ -1884,7 +1884,7 @@ bool SystemDictionary::do_unloading(GCTimer* gc_timer,
     // Oops referenced by the protection domain cache table may get unreachable independently
     // of the class loader (eg. cached protection domain oops). So we need to
     // explicitly unlink them here.
-    _pd_cache_table->unlink();
+    _pd_cache_table->trigger_cleanup();
   }
 
   if (do_cleaning) {

src/hotspot/share/classfile/systemDictionary.hpp

@@ -376,6 +376,9 @@ public:
   // System loader lock
   static oop system_loader_lock()           { return _system_loader_lock_obj; }
 
+  // Protection Domain Table
+  static ProtectionDomainCacheTable* pd_cache_table() { return _pd_cache_table; }
+
 public:
   // Sharing support.
   static void reorder_dictionary_for_sharing() NOT_CDS_RETURN;

src/hotspot/share/prims/whitebox.cpp

@@ -28,6 +28,7 @@
 
 #include "classfile/classLoaderData.hpp"
 #include "classfile/modules.hpp"
+#include "classfile/protectionDomainCache.hpp"
 #include "classfile/stringTable.hpp"
 #include "code/codeCache.hpp"
 #include "compiler/methodMatcher.hpp"
@@ -1977,6 +1978,10 @@ WB_ENTRY(jint, WB_ResolvedMethodRemovedCount(JNIEnv* env, jobject o))
   return (jint) ResolvedMethodTable::removed_entries_count();
 WB_END
 
+WB_ENTRY(jint, WB_ProtectionDomainRemovedCount(JNIEnv* env, jobject o))
+  return (jint) SystemDictionary::pd_cache_table()->removed_entries_count();
+WB_END
+
 
 #define CC (char*)
 
@@ -2199,6 +2204,7 @@ static JNINativeMethod methods[] = {
   {CC"printOsInfo",               CC"()V",            (void*)&WB_PrintOsInfo },
   {CC"disableElfSectionCache",    CC"()V",            (void*)&WB_DisableElfSectionCache },
   {CC"resolvedMethodRemovedCount",     CC"()I",       (void*)&WB_ResolvedMethodRemovedCount },
+  {CC"protectionDomainRemovedCount",   CC"()I",       (void*)&WB_ProtectionDomainRemovedCount },
 };
 
 

src/hotspot/share/runtime/serviceThread.cpp

@@ -23,8 +23,10 @@
  */
 
 #include "precompiled.hpp"
+#include "classfile/protectionDomainCache.hpp"
 #include "classfile/stringTable.hpp"
 #include "classfile/symbolTable.hpp"
+#include "classfile/systemDictionary.hpp"
 #include "runtime/interfaceSupport.inline.hpp"
 #include "runtime/javaCalls.hpp"
 #include "runtime/serviceThread.hpp"
@@ -88,6 +90,7 @@ void ServiceThread::service_thread_entry(JavaThread* jt, TRAPS) {
     bool stringtable_work = false;
     bool symboltable_work = false;
     bool resolved_method_table_work = false;
+    bool protection_domain_table_work = false;
     JvmtiDeferredEvent jvmti_event;
     {
       // Need state transition ThreadBlockInVM so that this thread
@@ -107,7 +110,8 @@ void ServiceThread::service_thread_entry(JavaThread* jt, TRAPS) {
               !(has_dcmd_notification_event = DCmdFactory::has_pending_jmx_notification()) &&
               !(stringtable_work = StringTable::has_work()) &&
               !(symboltable_work = SymbolTable::has_work()) &&
-              !(resolved_method_table_work = ResolvedMethodTable::has_work())) {
+              !(resolved_method_table_work = ResolvedMethodTable::has_work()) &&
+              !(protection_domain_table_work = SystemDictionary::pd_cache_table()->has_work())) {
         // wait until one of the sensors has pending requests, or there is a
         // pending JVMTI event or JMX GC notification to post
         Service_lock->wait(Mutex::_no_safepoint_check_flag);
@@ -145,6 +149,10 @@ void ServiceThread::service_thread_entry(JavaThread* jt, TRAPS) {
     if (resolved_method_table_work) {
       ResolvedMethodTable::unlink();
     }
+
+    if (protection_domain_table_work) {
+      SystemDictionary::pd_cache_table()->unlink();
+    }
   }
 }
 

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

@@ -0,0 +1,88 @@
+/*
+ * Copyright (c) 2018, 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.
+ */
+
+/*
+ * @test
+ * @summary Verifies the creation and cleaup of entries in the Protection Domain Table
+ * @library /test/lib
+ * @modules java.base/jdk.internal.misc
+ * @build sun.hotspot.WhiteBox
+ * @run driver ClassFileInstaller sun.hotspot.WhiteBox sun.hotspot.WhiteBox$WhiteBoxPermission
+ * @run main CleanProtectionDomain
+ */
+
+import java.security.ProtectionDomain;
+import jdk.test.lib.compiler.InMemoryJavaCompiler;
+import jdk.internal.misc.Unsafe;
+import static jdk.test.lib.Asserts.*;
+import jdk.test.lib.process.OutputAnalyzer;
+import jdk.test.lib.process.ProcessTools;
+import sun.hotspot.WhiteBox;
+
+public class CleanProtectionDomain {
+
+  public static void main(String args[]) throws Exception {
+    ProcessBuilder pb = ProcessTools.createJavaProcessBuilder(
+                                  "-Xlog:protectiondomain+table=debug",
+                                  "--add-exports=java.base/jdk.internal.misc=ALL-UNNAMED",
+                                  "-XX:+WhiteBoxAPI",
+                                  "-Xbootclasspath/a:.",
+                                  Test.class.getName());
+    OutputAnalyzer output = new OutputAnalyzer(pb.start());
+    output.shouldContain("protection domain added");
+    output.shouldContain("protection domain unlinked");
+    output.shouldHaveExitValue(0);
+  }
+
+  static class Test {
+    public static void test() throws Exception {
+      Unsafe unsafe = Unsafe.getUnsafe();
+      TestClassLoader classloader = new TestClassLoader();
+      ProtectionDomain pd = new ProtectionDomain(null, null);
+      byte klassbuf[] = InMemoryJavaCompiler.compile("TestClass", "class TestClass { }");
+      Class klass = unsafe.defineClass(null, klassbuf, 0, klassbuf.length, classloader, pd);
+    }
+
+    public static void main(String[] args) throws Exception {
+      WhiteBox wb = WhiteBox.getWhiteBox();
+      int removedCountOrig =  wb.protectionDomainRemovedCount();
+      int removedCount;
+
+      test();
+
+      System.gc();
+      // Wait until ServiceThread cleans ProtectionDomain table.
+      // When the TestClassLoader is unloaded by GC, at least one
+      // ProtectionDomainCacheEntry will be eligible for removal.
+      do {
+        removedCount = wb.protectionDomainRemovedCount();
+      } while (removedCountOrig == removedCount);
+    }
+
+    private static class TestClassLoader extends ClassLoader {
+      public TestClassLoader() {
+        super();
+      }
+    }
+  }
+}

test/lib/sun/hotspot/WhiteBox.java

@@ -539,4 +539,7 @@ public class WhiteBox {
 
   // Resolved Method Table
   public native int resolvedMethodRemovedCount();
+
+  // Protection Domain Table
+  public native int protectionDomainRemovedCount();
 }