8067713: Move clean_weak_method_links for redefinition out of class unloading

Do this work during class redefinition, only verify clean during class unloading in debug mode.

Reviewed-by: sspitsyn, roland, kbarrett

hotspot/src/share/vm/classfile/classLoaderData.cpp

@@ -786,17 +786,12 @@ void ClassLoaderDataGraph::clean_metaspaces() {
   MetadataOnStackMark md_on_stack(has_redefined_a_class);
 
   if (has_redefined_a_class) {
-    // purge_previous_versions also cleans weak method links. Because
-    // one method's MDO can reference another method from another
-    // class loader, we need to first clean weak method links for all
-    // class loaders here. Below, we can then free redefined methods
-    // for all class loaders.
     for (ClassLoaderData* data = _head; data != NULL; data = data->next()) {
       data->classes_do(InstanceKlass::purge_previous_versions);
     }
   }
 
-  // Need to purge the previous version before deallocating.
+  // Should purge the previous version before deallocating.
   free_deallocate_lists();
 }
 
@@ -834,8 +829,6 @@ void ClassLoaderDataGraph::post_class_unload_events(void) {
 
 void ClassLoaderDataGraph::free_deallocate_lists() {
   for (ClassLoaderData* cld = _head; cld != NULL; cld = cld->next()) {
-    // We need to keep this data until InstanceKlass::purge_previous_version has been
-    // called on all alive classes. See the comment in ClassLoaderDataGraph::clean_metaspaces.
     cld->free_deallocate_list();
   }
 }

hotspot/src/share/vm/oops/instanceKlass.cpp

@@ -3543,11 +3543,12 @@ void InstanceKlass::purge_previous_versions(InstanceKlass* ik) {
               ("purge: %s(%s): prev method @%d in version @%d is alive",
               method->name()->as_C_string(),
               method->signature()->as_C_string(), j, version));
+#ifdef ASSERT
             if (method->method_data() != NULL) {
-              // Clean out any weak method links for running methods
+              // Verify MethodData for running methods don't refer to old methods.
-              // (also should include not EMCP methods)
+              method->method_data()->verify_clean_weak_method_links();
-              method->method_data()->clean_weak_method_links();
             }
+#endif // ASSERT
           }
         }
       }
@@ -3561,15 +3562,17 @@ void InstanceKlass::purge_previous_versions(InstanceKlass* ik) {
       deleted_count));
   }
 
-  // Clean MethodData of this class's methods so they don't refer to
+#ifdef ASSERT
+  // Verify clean MethodData for this class's methods, e.g. they don't refer to
   // old methods that are no longer running.
   Array<Method*>* methods = ik->methods();
   int num_methods = methods->length();
-  for (int index2 = 0; index2 < num_methods; ++index2) {
+  for (int index = 0; index < num_methods; ++index) {
-    if (methods->at(index2)->method_data() != NULL) {
+    if (methods->at(index)->method_data() != NULL) {
-      methods->at(index2)->method_data()->clean_weak_method_links();
+      methods->at(index)->method_data()->verify_clean_weak_method_links();
     }
   }
+#endif // ASSERT
 }
 
 void InstanceKlass::mark_newly_obsolete_methods(Array<Method*>* old_methods,

hotspot/src/share/vm/oops/methodData.cpp

@@ -1283,6 +1283,11 @@ ProfileData* MethodData::bci_to_extra_data(int bci, Method* m, bool create_if_mi
          DataLayout::compute_size_in_bytes(SpeculativeTrapData::static_cell_count()),
          "code needs to be adjusted");
 
+  // Do not create one of these if method has been redefined.
+  if (m != NULL && m->is_old()) {
+    return NULL;
+  }
+
   DataLayout* dp  = extra_data_base();
   DataLayout* end = args_data_limit();
 
@@ -1554,9 +1559,7 @@ public:
 class CleanExtraDataMethodClosure : public CleanExtraDataClosure {
 public:
   CleanExtraDataMethodClosure() {}
-  bool is_live(Method* m) {
+  bool is_live(Method* m) { return !m->is_old(); }
-    return !m->is_old() || m->on_stack();
-  }
 };
 
 
@@ -1658,3 +1661,16 @@ void MethodData::clean_weak_method_links() {
   clean_extra_data(&cl);
   verify_extra_data_clean(&cl);
 }
+
+#ifdef ASSERT
+void MethodData::verify_clean_weak_method_links() {
+  for (ProfileData* data = first_data();
+       is_valid(data);
+       data = next_data(data)) {
+    data->verify_clean_weak_method_links();
+  }
+
+  CleanExtraDataMethodClosure cl;
+  verify_extra_data_clean(&cl);
+}
+#endif // ASSERT

hotspot/src/share/vm/oops/methodData.hpp

@@ -254,6 +254,7 @@ public:
 
   // Redefinition support
   void clean_weak_method_links();
+  DEBUG_ONLY(void verify_clean_weak_method_links();)
 };
 
 
@@ -511,6 +512,7 @@ public:
 
   // Redefinition support
   virtual void clean_weak_method_links() {}
+  DEBUG_ONLY(virtual void verify_clean_weak_method_links() {})
 
   // CI translation: ProfileData can represent both MethodDataOop data
   // as well as CIMethodData data. This function is provided for translating
@@ -1971,6 +1973,7 @@ public:
   }
 
   void set_method(Method* m) {
+    assert(!m->is_old(), "cannot add old methods");
     set_intptr_at(speculative_trap_method, (intptr_t)m);
   }
 
@@ -2480,6 +2483,7 @@ public:
 
   void clean_method_data(BoolObjectClosure* is_alive);
   void clean_weak_method_links();
+  DEBUG_ONLY(void verify_clean_weak_method_links();)
   Mutex* extra_data_lock() { return &_extra_data_lock; }
 };
 

hotspot/src/share/vm/prims/jvmtiRedefineClasses.cpp

@@ -148,6 +148,10 @@ void VM_RedefineClasses::doit() {
     _scratch_classes[i] = NULL;
   }
 
+  // Clean out MethodData pointing to old Method*
+  MethodDataCleaner clean_weak_method_links;
+  ClassLoaderDataGraph::classes_do(&clean_weak_method_links);
+
   // Disable any dependent concurrent compilations
   SystemDictionary::notice_modification();
 
@@ -155,8 +159,8 @@ void VM_RedefineClasses::doit() {
   // See jvmtiExport.hpp for detailed explanation.
   JvmtiExport::set_has_redefined_a_class();
 
-// check_class() is optionally called for product bits, but is
+  // check_class() is optionally called for product bits, but is
-// always called for non-product bits.
+  // always called for non-product bits.
 #ifdef PRODUCT
   if (RC_TRACE_ENABLED(0x00004000)) {
 #endif
@@ -3445,6 +3449,22 @@ void VM_RedefineClasses::AdjustCpoolCacheAndVtable::do_klass(Klass* k) {
   }
 }
 
+// Clean method data for this class
+void VM_RedefineClasses::MethodDataCleaner::do_klass(Klass* k) {
+  if (k->oop_is_instance()) {
+    InstanceKlass *ik = InstanceKlass::cast(k);
+    // Clean MethodData of this class's methods so they don't refer to
+    // old methods that are no longer running.
+    Array<Method*>* methods = ik->methods();
+    int num_methods = methods->length();
+    for (int index = 0; index < num_methods; ++index) {
+      if (methods->at(index)->method_data() != NULL) {
+        methods->at(index)->method_data()->clean_weak_method_links();
+      }
+    }
+  }
+}
+
 void VM_RedefineClasses::update_jmethod_ids() {
   for (int j = 0; j < _matching_methods_length; ++j) {
     Method* old_method = _matching_old_methods[j];

hotspot/src/share/vm/prims/jvmtiRedefineClasses.hpp

@@ -511,6 +511,12 @@ class VM_RedefineClasses: public VM_Operation {
     void do_klass(Klass* k);
   };
 
+  // Clean MethodData out
+  class MethodDataCleaner : public KlassClosure {
+   public:
+    MethodDataCleaner() {}
+    void do_klass(Klass* k);
+  };
  public:
   VM_RedefineClasses(jint class_count,
                      const jvmtiClassDefinition *class_defs,