8202447: Fix unloading_occurred to mean unloading_occurred
authorcoleenp
Fri, 27 Apr 2018 15:00:04 -0400
changeset 49969 8624981f1ffa
parent 49968 33a76b934213
child 49970 fc778e86381f
8202447: Fix unloading_occurred to mean unloading_occurred Summary: nmethod unloading does not need to test for jvmti to set unloading_occurred, nor do we need to clean weak Klasses in metadata if unloading does not occur. Reviewed-by: sspitsyn, rehn
src/hotspot/share/code/compiledMethod.cpp
src/hotspot/share/gc/cms/concurrentMarkSweepGeneration.cpp
src/hotspot/share/gc/g1/g1CollectedHeap.cpp
src/hotspot/share/gc/parallel/psMarkSweep.cpp
src/hotspot/share/gc/parallel/psParallelCompact.cpp
src/hotspot/share/gc/serial/genMarkSweep.cpp
src/hotspot/share/oops/instanceKlass.hpp
src/hotspot/share/oops/klass.cpp
src/hotspot/share/oops/klass.hpp
src/hotspot/share/oops/methodData.cpp
src/hotspot/share/prims/jvmtiExport.hpp
--- a/src/hotspot/share/code/compiledMethod.cpp	Thu May 03 08:07:20 2018 -0400
+++ b/src/hotspot/share/code/compiledMethod.cpp	Fri Apr 27 15:00:04 2018 -0400
@@ -389,22 +389,24 @@
 
 void CompiledMethod::clean_ic_if_metadata_is_dead(CompiledIC *ic) {
   if (ic->is_icholder_call()) {
-    // The only exception is compiledICHolder oops which may
+    // The only exception is compiledICHolder metdata which may
     // yet be marked below. (We check this further below).
-    CompiledICHolder* cichk_oop = ic->cached_icholder();
+    CompiledICHolder* cichk_metdata = ic->cached_icholder();
 
-    if (cichk_oop->is_loader_alive()) {
+    if (cichk_metdata->is_loader_alive()) {
       return;
     }
   } else {
-    Metadata* ic_oop = ic->cached_metadata();
-    if (ic_oop != NULL) {
-      if (ic_oop->is_klass()) {
-        if (((Klass*)ic_oop)->is_loader_alive()) {
+    Metadata* ic_metdata = ic->cached_metadata();
+    if (ic_metdata != NULL) {
+      if (ic_metdata->is_klass()) {
+        if (((Klass*)ic_metdata)->is_loader_alive()) {
           return;
         }
-      } else if (ic_oop->is_method()) {
-        if (((Method*)ic_oop)->method_holder()->is_loader_alive()) {
+      } else if (ic_metdata->is_method()) {
+        Method* method = (Method*)ic_metdata;
+        assert(!method->is_old(), "old method should have been cleaned");
+        if (method->method_holder()->is_loader_alive()) {
           return;
         }
       } else {
@@ -493,16 +495,6 @@
     // (See comment above.)
   }
 
-  // The RedefineClasses() API can cause the class unloading invariant
-  // to no longer be true. See jvmtiExport.hpp for details.
-  // Also, leave a debugging breadcrumb in local flag.
-  if (JvmtiExport::has_redefined_a_class()) {
-    // This set of the unloading_occurred flag is done before the
-    // call to post_compiled_method_unload() so that the unloading
-    // of this nmethod is reported.
-    unloading_occurred = true;
-  }
-
   // Exception cache
   clean_exception_cache();
 
@@ -581,16 +573,6 @@
     // (See comment above.)
   }
 
-  // The RedefineClasses() API can cause the class unloading invariant
-  // to no longer be true. See jvmtiExport.hpp for details.
-  // Also, leave a debugging breadcrumb in local flag.
-  if (JvmtiExport::has_redefined_a_class()) {
-    // This set of the unloading_occurred flag is done before the
-    // call to post_compiled_method_unload() so that the unloading
-    // of this nmethod is reported.
-    unloading_occurred = true;
-  }
-
   // Exception cache
   clean_exception_cache();
 
--- a/src/hotspot/share/gc/cms/concurrentMarkSweepGeneration.cpp	Thu May 03 08:07:20 2018 -0400
+++ b/src/hotspot/share/gc/cms/concurrentMarkSweepGeneration.cpp	Fri Apr 27 15:00:04 2018 -0400
@@ -5245,7 +5245,7 @@
       CodeCache::do_unloading(&_is_alive_closure, purged_class);
 
       // Prune dead klasses from subklass/sibling/implementor lists.
-      Klass::clean_weak_klass_links();
+      Klass::clean_weak_klass_links(purged_class);
     }
 
     {
--- a/src/hotspot/share/gc/g1/g1CollectedHeap.cpp	Thu May 03 08:07:20 2018 -0400
+++ b/src/hotspot/share/gc/g1/g1CollectedHeap.cpp	Fri Apr 27 15:00:04 2018 -0400
@@ -3529,6 +3529,7 @@
 // To minimize the remark pause times, the tasks below are done in parallel.
 class G1ParallelCleaningTask : public AbstractGangTask {
 private:
+  bool                          _unloading_occurred;
   G1StringAndSymbolCleaningTask _string_symbol_task;
   G1CodeCacheUnloadingTask      _code_cache_task;
   G1KlassCleaningTask           _klass_cleaning_task;
@@ -3541,6 +3542,7 @@
       _string_symbol_task(is_alive, true, true, G1StringDedup::is_enabled()),
       _code_cache_task(num_workers, is_alive, unloading_occurred),
       _klass_cleaning_task(),
+      _unloading_occurred(unloading_occurred),
       _resolved_method_cleaning_task() {
   }
 
@@ -3566,7 +3568,11 @@
     _code_cache_task.work_second_pass(worker_id);
 
     // Clean all klasses that were not unloaded.
-    _klass_cleaning_task.work();
+    // The weak metadata in klass doesn't need to be
+    // processed if there was no unloading.
+    if (_unloading_occurred) {
+      _klass_cleaning_task.work();
+    }
   }
 };
 
--- a/src/hotspot/share/gc/parallel/psMarkSweep.cpp	Thu May 03 08:07:20 2018 -0400
+++ b/src/hotspot/share/gc/parallel/psMarkSweep.cpp	Fri Apr 27 15:00:04 2018 -0400
@@ -565,7 +565,7 @@
     CodeCache::do_unloading(is_alive_closure(), purged_class);
 
     // Prune dead klasses from subklass/sibling/implementor lists.
-    Klass::clean_weak_klass_links();
+    Klass::clean_weak_klass_links(purged_class);
   }
 
   {
--- a/src/hotspot/share/gc/parallel/psParallelCompact.cpp	Thu May 03 08:07:20 2018 -0400
+++ b/src/hotspot/share/gc/parallel/psParallelCompact.cpp	Fri Apr 27 15:00:04 2018 -0400
@@ -2140,7 +2140,7 @@
     CodeCache::do_unloading(is_alive_closure(), purged_class);
 
     // Prune dead klasses from subklass/sibling/implementor lists.
-    Klass::clean_weak_klass_links();
+    Klass::clean_weak_klass_links(purged_class);
   }
 
   {
--- a/src/hotspot/share/gc/serial/genMarkSweep.cpp	Thu May 03 08:07:20 2018 -0400
+++ b/src/hotspot/share/gc/serial/genMarkSweep.cpp	Fri Apr 27 15:00:04 2018 -0400
@@ -234,7 +234,7 @@
     CodeCache::do_unloading(&is_alive, purged_class);
 
     // Prune dead klasses from subklass/sibling/implementor lists.
-    Klass::clean_weak_klass_links();
+    Klass::clean_weak_klass_links(purged_class);
   }
 
   {
--- a/src/hotspot/share/oops/instanceKlass.hpp	Thu May 03 08:07:20 2018 -0400
+++ b/src/hotspot/share/oops/instanceKlass.hpp	Fri Apr 27 15:00:04 2018 -0400
@@ -1150,9 +1150,11 @@
 #endif // INCLUDE_JVMTI
 
   void clean_weak_instanceklass_links();
+ private:
   void clean_implementors_list();
   void clean_method_data();
 
+ public:
   // Explicit metaspace deallocation of fields
   // For RedefineClasses and class file parsing errors, we need to deallocate
   // instanceKlasses and the metadata they point to.
--- a/src/hotspot/share/oops/klass.cpp	Thu May 03 08:07:20 2018 -0400
+++ b/src/hotspot/share/oops/klass.cpp	Fri Apr 27 15:00:04 2018 -0400
@@ -384,8 +384,8 @@
   debug_only(verify();)
 }
 
-void Klass::clean_weak_klass_links(bool clean_alive_klasses) {
-  if (!ClassUnloading) {
+void Klass::clean_weak_klass_links(bool unloading_occurred, bool clean_alive_klasses) {
+  if (!ClassUnloading || !unloading_occurred) {
     return;
   }
 
--- a/src/hotspot/share/oops/klass.hpp	Thu May 03 08:07:20 2018 -0400
+++ b/src/hotspot/share/oops/klass.hpp	Fri Apr 27 15:00:04 2018 -0400
@@ -638,9 +638,9 @@
   // Klass is considered alive.  Has already been marked as unloading.
   bool is_loader_alive() const { return !class_loader_data()->is_unloading(); }
 
-  static void clean_weak_klass_links(bool clean_alive_klasses = true);
+  static void clean_weak_klass_links(bool unloading_occurred, bool clean_alive_klasses = true);
   static void clean_subklass_tree() {
-    clean_weak_klass_links(false /* clean_alive_klasses */);
+    clean_weak_klass_links(/*unloading_occurred*/ true , /* clean_alive_klasses */ false);
   }
 
   // GC specific object visitors
--- a/src/hotspot/share/oops/methodData.cpp	Thu May 03 08:07:20 2018 -0400
+++ b/src/hotspot/share/oops/methodData.cpp	Fri Apr 27 15:00:04 2018 -0400
@@ -429,7 +429,7 @@
   ReceiverTypeData::clean_weak_method_links();
   for (uint row = 0; row < method_row_limit(); row++) {
     Method* p = method(row);
-    if (p != NULL && !p->on_stack()) {
+    if (p != NULL && p->is_old()) {
       clear_method_row(row);
     }
   }
@@ -1770,6 +1770,8 @@
   verify_extra_data_clean(&cl);
 }
 
+// This is called during redefinition to clean all "old" redefined
+// methods out of MethodData for all methods.
 void MethodData::clean_weak_method_links() {
   ResourceMark rm;
   for (ProfileData* data = first_data();
--- a/src/hotspot/share/prims/jvmtiExport.hpp	Thu May 03 08:07:20 2018 -0400
+++ b/src/hotspot/share/prims/jvmtiExport.hpp	Fri Apr 27 15:00:04 2018 -0400
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 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
@@ -170,19 +170,6 @@
   static void post_dynamic_code_generated(JvmtiEnv* env, const char *name, const void *code_begin,
                                           const void *code_end) NOT_JVMTI_RETURN;
 
-  // The RedefineClasses() API breaks some invariants in the "regular"
-  // system. For example, there are sanity checks when GC'ing nmethods
-  // that require the containing class to be unloading. However, when a
-  // method is redefined, the old method and nmethod can become GC'able
-  // without the containing class unloading. The state of becoming
-  // GC'able can be asynchronous to the RedefineClasses() call since
-  // the old method may still be running and cannot be GC'ed until
-  // after all old invocations have finished. Additionally, a method
-  // that has not been redefined may have an nmethod that depends on
-  // the redefined method. The dependent nmethod will get deopted in
-  // this case and may also be GC'able without the containing class
-  // being unloaded.
-  //
   // This flag indicates whether RedefineClasses() has ever redefined
   // one or more classes during the lifetime of the VM. The flag should
   // only be set by the friend class and can be queried by other sub