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

Reviewed-by: kvn, thartmann

src/hotspot/share/code/compiledIC.cpp

@@ -539,17 +539,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());
-  }
-}
-
 // ----------------------------------------------------------------------------
 
 void CompiledStaticCall::set_to_clean(bool in_use) {

src/hotspot/share/code/compiledIC.hpp

@@ -210,10 +210,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

@@ -322,15 +322,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_locked_or_safepoint(CompiledIC_lock);
   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

@@ -347,7 +347,7 @@ public:
   }
 
   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

@@ -1076,6 +1076,9 @@ void nmethod::make_unloaded(oop cause) {
     CodeCache::set_needs_cache_clean(true);
   }
 
+  // Clear ICStubs and release any CompiledICHolders.
+  clear_ic_callsites();
+
   // Unregister must be done before the state change
   Universe::heap()->unregister_nmethod(this);
 
@@ -1286,6 +1289,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

@@ -591,27 +591,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;
-    MutexLocker ml_patch(CompiledIC_lock);
-    RelocIterator iter(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");
@@ -638,7 +617,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()) {
@@ -650,7 +629,7 @@ NMethodSweeper::MethodStateChange NMethodSweeper::process_compiled_method(Compil
       // nmethods during the next safepoint (see ICStub::finalize).
       {
         MutexLocker cl(CompiledIC_lock);
-        cm->clear_ic_stubs();
+        cm->clear_ic_callsites();
       }
       // Code cache state change is tracked in make_zombie()
       cm->make_zombie();
@@ -663,7 +642,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 {
@@ -680,16 +659,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.
-      MutexLocker cl(CompiledIC_lock);
-      cm->cleanup_inline_caches();
-    }
     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

@@ -91,7 +91,6 @@ class NMethodSweeper : public AllStatic {
   static Monitor*  _stat_lock;
 
   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();