8216541: CompiledICHolders of VM locked unloaded nmethods are released too late

Reviewed-by: kvn, thartmann

src/hotspot/share/code/compiledIC.cpp

@@ -51,7 +51,8 @@
 CompiledICLocker::CompiledICLocker(CompiledMethod* method)
   : _method(method),
     _behaviour(CompiledICProtectionBehaviour::current()),
-    _locked(_behaviour->lock(_method)){
+    _locked(_behaviour->lock(_method)),
+    _nsv(true, !SafepointSynchronize::is_at_safepoint()) {
 }
 
 CompiledICLocker::~CompiledICLocker() {
@@ -583,17 +584,6 @@ bool CompiledIC::is_icholder_call_site(virtual_call_Relocation* call_site, const
   return is_icholder_entry(dest);
 }
 
-// Release the CompiledICHolder* associated with this call site is there is one.
-void CompiledIC::cleanup_call_site(virtual_call_Relocation* call_site, const CompiledMethod* cm) {
-  assert(cm->is_nmethod(), "must be nmethod");
-  // This call site might have become stale so inspect it carefully.
-  NativeCall* call = nativeCall_at(call_site->addr());
-  if (is_icholder_entry(call->destination())) {
-    NativeMovConstReg* value = nativeMovConstReg_at(call_site->cached_value());
-    InlineCacheBuffer::queue_for_release((CompiledICHolder*)value->data());
-  }
-}
-
 // ----------------------------------------------------------------------------
 
 bool CompiledStaticCall::set_to_clean(bool in_use) {

src/hotspot/share/code/compiledIC.hpp

@@ -226,10 +226,6 @@ class CompiledIC: public ResourceObj {
   friend CompiledIC* CompiledIC_at(Relocation* call_site);
   friend CompiledIC* CompiledIC_at(RelocIterator* reloc_iter);
 
-  // This is used to release CompiledICHolder*s from nmethods that
-  // are about to be freed.  The callsite might contain other stale
-  // values of other kinds so it must be careful.
-  static void cleanup_call_site(virtual_call_Relocation* call_site, const CompiledMethod* cm);
   static bool is_icholder_call_site(virtual_call_Relocation* call_site, const CompiledMethod* cm);
 
   // Return the cached_metadata/destination associated with this inline cache. If the cache currently points

src/hotspot/share/code/compiledMethod.cpp

@@ -399,15 +399,16 @@ void CompiledMethod::clear_inline_caches() {
   }
 }
 
-// Clear ICStubs of all compiled ICs
-void CompiledMethod::clear_ic_stubs() {
+// Clear IC callsites, releasing ICStubs of all compiled ICs
+// as well as any associated CompiledICHolders.
+void CompiledMethod::clear_ic_callsites() {
   assert(CompiledICLocker::is_safe(this), "mt unsafe call");
   ResourceMark rm;
   RelocIterator iter(this);
   while(iter.next()) {
     if (iter.type() == relocInfo::virtual_call_type) {
       CompiledIC* ic = CompiledIC_at(&iter);
-      ic->clear_ic_stub();
+      ic->set_to_clean(false);
     }
   }
 }

src/hotspot/share/code/compiledMethod.hpp

@@ -359,7 +359,7 @@ public:
   void cleanup_inline_caches(bool clean_all);
 
   virtual void clear_inline_caches();
-  void clear_ic_stubs();
+  void clear_ic_callsites();
 
   // Verify and count cached icholder relocations.
   int  verify_icholder_relocations();

src/hotspot/share/code/nmethod.cpp

@@ -1099,6 +1099,12 @@ void nmethod::make_unloaded() {
   assert(SafepointSynchronize::is_at_safepoint() || Thread::current()->is_ConcurrentGC_thread(),
          "must be at safepoint");
 
+  {
+    // Clear ICStubs and release any CompiledICHolders.
+    CompiledICLocker ml(this);
+    clear_ic_callsites();
+  }
+
   // Unregister must be done before the state change
   {
     MutexLockerEx ml(SafepointSynchronize::is_at_safepoint() ? NULL : CodeCache_lock,
@@ -1291,10 +1297,11 @@ bool nmethod::make_not_entrant_or_zombie(int state) {
     }
 
     // Clear ICStubs to prevent back patching stubs of zombie or flushed
-    // nmethods during the next safepoint (see ICStub::finalize).
+    // nmethods during the next safepoint (see ICStub::finalize), as well
+    // as to free up CompiledICHolder resources.
     {
       CompiledICLocker ml(this);
-      clear_ic_stubs();
+      clear_ic_callsites();
     }
 
     // zombie only - if a JVMTI agent has enabled the CompiledMethodUnload
@@ -1326,6 +1333,7 @@ bool nmethod::make_not_entrant_or_zombie(int state) {
 }
 
 void nmethod::flush() {
+  MutexLockerEx mu(CodeCache_lock, Mutex::_no_safepoint_check_flag);
   // Note that there are no valid oops in the nmethod anymore.
   assert(!is_osr_method() || is_unloaded() || is_zombie(),
          "osr nmethod must be unloaded or zombie before flushing");

src/hotspot/share/runtime/sweeper.cpp

@@ -663,27 +663,6 @@ class CompiledMethodMarker: public StackObj {
   }
 };
 
-void NMethodSweeper::release_compiled_method(CompiledMethod* nm) {
-  // Make sure the released nmethod is no longer referenced by the sweeper thread
-  CodeCacheSweeperThread* thread = (CodeCacheSweeperThread*)JavaThread::current();
-  thread->set_scanned_compiled_method(NULL);
-
-  // Clean up any CompiledICHolders
-  {
-    ResourceMark rm;
-    RelocIterator iter(nm);
-    CompiledICLocker ml(nm);
-    while (iter.next()) {
-      if (iter.type() == relocInfo::virtual_call_type) {
-        CompiledIC::cleanup_call_site(iter.virtual_call_reloc(), nm);
-      }
-    }
-  }
-
-  MutexLockerEx mu(CodeCache_lock, Mutex::_no_safepoint_check_flag);
-  nm->flush();
-}
-
 NMethodSweeper::MethodStateChange NMethodSweeper::process_compiled_method(CompiledMethod* cm) {
   assert(cm != NULL, "sanity");
   assert(!CodeCache_lock->owned_by_self(), "just checking");
@@ -697,7 +676,7 @@ NMethodSweeper::MethodStateChange NMethodSweeper::process_compiled_method(Compil
   // Skip methods that are currently referenced by the VM
   if (cm->is_locked_by_vm()) {
     // But still remember to clean-up inline caches for alive nmethods
-    if (cm->is_alive() && !cm->is_unloading()) {
+    if (cm->is_alive()) {
       // Clean inline caches that point to zombie/non-entrant/unloaded nmethods
       cm->cleanup_inline_caches(false);
       SWEEP(cm);
@@ -709,7 +688,7 @@ NMethodSweeper::MethodStateChange NMethodSweeper::process_compiled_method(Compil
     // All inline caches that referred to this nmethod were cleaned in the
     // previous sweeper cycle. Now flush the nmethod from the code cache.
     assert(!cm->is_locked_by_vm(), "must not flush locked Compiled Methods");
-    release_compiled_method(cm);
+    cm->flush();
     assert(result == None, "sanity");
     result = Flushed;
   } else if (cm->is_not_entrant()) {
@@ -728,7 +707,7 @@ NMethodSweeper::MethodStateChange NMethodSweeper::process_compiled_method(Compil
         // Make sure that we unregistered the nmethod with the heap and flushed all
         // dependencies before removing the nmethod (done in make_zombie()).
         assert(cm->is_zombie(), "nmethod must be unregistered");
-        release_compiled_method(cm);
+        cm->flush();
         assert(result == None, "sanity");
         result = Flushed;
       } else {
@@ -744,14 +723,10 @@ NMethodSweeper::MethodStateChange NMethodSweeper::process_compiled_method(Compil
   } else if (cm->is_unloaded()) {
     // Code is unloaded, so there are no activations on the stack.
     // Convert the nmethod to zombie or flush it directly in the OSR case.
-
-    // Clean ICs of unloaded nmethods as well because they may reference other
-    // unloaded nmethods that may be flushed earlier in the sweeper cycle.
-    cm->cleanup_inline_caches(false);
     if (cm->is_osr_method()) {
       SWEEP(cm);
       // No inline caches will ever point to osr methods, so we can just remove it
-      release_compiled_method(cm);
+      cm->flush();
       assert(result == None, "sanity");
       result = Flushed;
     } else {

src/hotspot/share/runtime/sweeper.hpp

@@ -89,7 +89,6 @@ class NMethodSweeper : public AllStatic {
   static Tickspan  _peak_sweep_fraction_time;     // Peak time sweeping one fraction
 
   static MethodStateChange process_compiled_method(CompiledMethod *nm);
-  static void              release_compiled_method(CompiledMethod* nm);
 
   static void init_sweeper_log() NOT_DEBUG_RETURN;
   static bool wait_for_stack_scanning();