8165246: [REDO] InstanceKlass::_previous_version_count goes negative
authorcoleenp
Wed, 07 Sep 2016 15:25:21 -0400
changeset 40927 59f3c8a69541
parent 40925 04b19236aa98
child 40928 9a72c96b0c48
8165246: [REDO] InstanceKlass::_previous_version_count goes negative Summary: make _has_previous_version a boolean that is set to true when previous version of a class is added or during class unloading call to purge_previous_versions Reviewed-by: gtriantafill, dcubed, sspitsyn
hotspot/src/share/vm/classfile/classLoaderData.cpp
hotspot/src/share/vm/oops/instanceKlass.cpp
hotspot/src/share/vm/oops/instanceKlass.hpp
hotspot/test/runtime/RedefineTests/RedefinePreviousVersions.java
--- a/hotspot/src/share/vm/classfile/classLoaderData.cpp	Wed Sep 07 15:21:45 2016 +0200
+++ b/hotspot/src/share/vm/classfile/classLoaderData.cpp	Wed Sep 07 15:25:21 2016 -0400
@@ -966,7 +966,7 @@
   // Klasses to delete.
   bool walk_all_metadata = clean_previous_versions &&
                            JvmtiExport::has_redefined_a_class() &&
-                           InstanceKlass::has_previous_versions();
+                           InstanceKlass::has_previous_versions_and_reset();
   MetadataOnStackMark md_on_stack(walk_all_metadata);
 
   // Save previous _unloading pointer for CMS which may add to unloading list before
--- a/hotspot/src/share/vm/oops/instanceKlass.cpp	Wed Sep 07 15:21:45 2016 +0200
+++ b/hotspot/src/share/vm/oops/instanceKlass.cpp	Wed Sep 07 15:25:21 2016 -0400
@@ -3365,88 +3365,119 @@
 
 #if INCLUDE_JVMTI
 
-// RedefineClasses() support for previous versions:
-int InstanceKlass::_previous_version_count = 0;
-
-// Purge previous versions before adding new previous versions of the class.
-void InstanceKlass::purge_previous_versions(InstanceKlass* ik) {
-  if (ik->previous_versions() != NULL) {
-    // This klass has previous versions so see what we can cleanup
-    // while it is safe to do so.
-
-    int deleted_count = 0;    // leave debugging breadcrumbs
-    int live_count = 0;
-    ClassLoaderData* loader_data = ik->class_loader_data();
-    assert(loader_data != NULL, "should never be null");
-
-    ResourceMark rm;
-    log_trace(redefine, class, iklass, purge)("%s: previous versions", ik->external_name());
-
-    // previous versions are linked together through the InstanceKlass
-    InstanceKlass* pv_node = ik->previous_versions();
-    InstanceKlass* last = ik;
-    int version = 0;
-
-    // check the previous versions list
-    for (; pv_node != NULL; ) {
-
-      ConstantPool* pvcp = pv_node->constants();
-      assert(pvcp != NULL, "cp ref was unexpectedly cleared");
-
-      if (!pvcp->on_stack()) {
-        // If the constant pool isn't on stack, none of the methods
-        // are executing.  Unlink this previous_version.
-        // The previous version InstanceKlass is on the ClassLoaderData deallocate list
-        // so will be deallocated during the next phase of class unloading.
-        log_trace(redefine, class, iklass, purge)("previous version " INTPTR_FORMAT " is dead", p2i(pv_node));
-        // For debugging purposes.
-        pv_node->set_is_scratch_class();
-        pv_node->class_loader_data()->add_to_deallocate_list(pv_node);
-        pv_node = pv_node->previous_versions();
-        last->link_previous_versions(pv_node);
-        deleted_count++;
-        version++;
-        continue;
-      } else {
-        log_trace(redefine, class, iklass, purge)("previous version " INTPTR_FORMAT " is alive", p2i(pv_node));
-        assert(pvcp->pool_holder() != NULL, "Constant pool with no holder");
-        guarantee (!loader_data->is_unloading(), "unloaded classes can't be on the stack");
-        live_count++;
-      }
-
-      // At least one method is live in this previous version.
-      // Reset dead EMCP methods not to get breakpoints.
-      // All methods are deallocated when all of the methods for this class are no
-      // longer running.
-      Array<Method*>* method_refs = pv_node->methods();
-      if (method_refs != NULL) {
-        log_trace(redefine, class, iklass, purge)("previous methods length=%d", method_refs->length());
-        for (int j = 0; j < method_refs->length(); j++) {
-          Method* method = method_refs->at(j);
-
-          if (!method->on_stack()) {
-            // no breakpoints for non-running methods
-            if (method->is_running_emcp()) {
-              method->set_running_emcp(false);
-            }
-          } else {
-            assert (method->is_obsolete() || method->is_running_emcp(),
-                    "emcp method cannot run after emcp bit is cleared");
-            log_trace(redefine, class, iklass, purge)
-              ("purge: %s(%s): prev method @%d in version @%d is alive",
-               method->name()->as_C_string(), method->signature()->as_C_string(), j, version);
+// RedefineClasses() support for previous versions
+
+// Globally, there is at least one previous version of a class to walk
+// during class unloading, which is saved because old methods in the class
+// are still running.   Otherwise the previous version list is cleaned up.
+bool InstanceKlass::_has_previous_versions = false;
+
+// Returns true if there are previous versions of a class for class
+// unloading only. Also resets the flag to false. purge_previous_version
+// will set the flag to true if there are any left, i.e., if there's any
+// work to do for next time. This is to avoid the expensive code cache
+// walk in CLDG::do_unloading().
+bool InstanceKlass::has_previous_versions_and_reset() {
+  bool ret = _has_previous_versions;
+  log_trace(redefine, class, iklass, purge)("Class unloading: has_previous_versions = %s",
+     ret ? "true" : "false");
+  _has_previous_versions = false;
+  return ret;
+}
+
+// Purge previous versions before adding new previous versions of the class and
+// during class unloading.
+void InstanceKlass::purge_previous_version_list() {
+  assert(SafepointSynchronize::is_at_safepoint(), "only called at safepoint");
+  assert(has_been_redefined(), "Should only be called for main class");
+
+  // Quick exit.
+  if (previous_versions() == NULL) {
+    return;
+  }
+
+  // This klass has previous versions so see what we can cleanup
+  // while it is safe to do so.
+
+  int deleted_count = 0;    // leave debugging breadcrumbs
+  int live_count = 0;
+  ClassLoaderData* loader_data = class_loader_data();
+  assert(loader_data != NULL, "should never be null");
+
+  ResourceMark rm;
+  log_trace(redefine, class, iklass, purge)("%s: previous versions", external_name());
+
+  // previous versions are linked together through the InstanceKlass
+  InstanceKlass* pv_node = previous_versions();
+  InstanceKlass* last = this;
+  int version = 0;
+
+  // check the previous versions list
+  for (; pv_node != NULL; ) {
+
+    ConstantPool* pvcp = pv_node->constants();
+    assert(pvcp != NULL, "cp ref was unexpectedly cleared");
+
+    if (!pvcp->on_stack()) {
+      // If the constant pool isn't on stack, none of the methods
+      // are executing.  Unlink this previous_version.
+      // The previous version InstanceKlass is on the ClassLoaderData deallocate list
+      // so will be deallocated during the next phase of class unloading.
+      log_trace(redefine, class, iklass, purge)
+        ("previous version " INTPTR_FORMAT " is dead.", p2i(pv_node));
+      // For debugging purposes.
+      pv_node->set_is_scratch_class();
+      // Unlink from previous version list.
+      assert(pv_node->class_loader_data() == loader_data, "wrong loader_data");
+      InstanceKlass* next = pv_node->previous_versions();
+      pv_node->link_previous_versions(NULL);   // point next to NULL
+      last->link_previous_versions(next);
+      // Add to the deallocate list after unlinking
+      loader_data->add_to_deallocate_list(pv_node);
+      pv_node = next;
+      deleted_count++;
+      version++;
+      continue;
+    } else {
+      log_trace(redefine, class, iklass, purge)("previous version " INTPTR_FORMAT " is alive", p2i(pv_node));
+      assert(pvcp->pool_holder() != NULL, "Constant pool with no holder");
+      guarantee (!loader_data->is_unloading(), "unloaded classes can't be on the stack");
+      live_count++;
+      // found a previous version for next time we do class unloading
+      _has_previous_versions = true;
+    }
+
+    // At least one method is live in this previous version.
+    // Reset dead EMCP methods not to get breakpoints.
+    // All methods are deallocated when all of the methods for this class are no
+    // longer running.
+    Array<Method*>* method_refs = pv_node->methods();
+    if (method_refs != NULL) {
+      log_trace(redefine, class, iklass, purge)("previous methods length=%d", method_refs->length());
+      for (int j = 0; j < method_refs->length(); j++) {
+        Method* method = method_refs->at(j);
+
+        if (!method->on_stack()) {
+          // no breakpoints for non-running methods
+          if (method->is_running_emcp()) {
+            method->set_running_emcp(false);
           }
+        } else {
+          assert (method->is_obsolete() || method->is_running_emcp(),
+                  "emcp method cannot run after emcp bit is cleared");
+          log_trace(redefine, class, iklass, purge)
+            ("purge: %s(%s): prev method @%d in version @%d is alive",
+             method->name()->as_C_string(), method->signature()->as_C_string(), j, version);
         }
       }
-      // next previous version
-      last = pv_node;
-      pv_node = pv_node->previous_versions();
-      version++;
     }
-    log_trace(redefine, class, iklass, purge)
-      ("previous version stats: live=%d, deleted=%d",
-       live_count, deleted_count);
+    // next previous version
+    last = pv_node;
+    pv_node = pv_node->previous_versions();
+    version++;
   }
+  log_trace(redefine, class, iklass, purge)
+    ("previous version stats: live=%d, deleted=%d", live_count, deleted_count);
 }
 
 void InstanceKlass::mark_newly_obsolete_methods(Array<Method*>* old_methods,
@@ -3518,8 +3549,8 @@
   log_trace(redefine, class, iklass, add)
     ("adding previous version ref for %s, EMCP_cnt=%d", scratch_class->external_name(), emcp_method_count);
 
-  // Clean out old previous versions
-  purge_previous_versions(this);
+  // Clean out old previous versions for this class
+  purge_previous_version_list();
 
   // Mark newly obsolete methods in remaining previous versions.  An EMCP method from
   // a previous redefinition may be made obsolete by this redefinition.
@@ -3536,8 +3567,6 @@
     // For debugging purposes.
     scratch_class->set_is_scratch_class();
     scratch_class->class_loader_data()->add_to_deallocate_list(scratch_class());
-    // Update count for class unloading.
-    _previous_version_count--;
     return;
   }
 
@@ -3565,12 +3594,12 @@
   }
 
   // Add previous version if any methods are still running.
-  log_trace(redefine, class, iklass, add)("scratch class added; one of its methods is on_stack");
+  // Set has_previous_version flag for processing during class unloading.
+  _has_previous_versions = true;
+  log_trace(redefine, class, iklass, add) ("scratch class added; one of its methods is on_stack.");
   assert(scratch_class->previous_versions() == NULL, "shouldn't have a previous version");
   scratch_class->link_previous_versions(previous_versions());
   link_previous_versions(scratch_class());
-  // Update count for class unloading.
-  _previous_version_count++;
 } // end add_previous_version()
 
 #endif // INCLUDE_JVMTI
--- a/hotspot/src/share/vm/oops/instanceKlass.hpp	Wed Sep 07 15:21:45 2016 +0200
+++ b/hotspot/src/share/vm/oops/instanceKlass.hpp	Wed Sep 07 15:25:21 2016 -0400
@@ -709,6 +709,7 @@
 
   // RedefineClasses() support for previous versions:
   void add_previous_version(instanceKlassHandle ikh, int emcp_method_count);
+  void purge_previous_version_list();
 
   InstanceKlass* previous_versions() const { return _previous_versions; }
 #else
@@ -768,10 +769,15 @@
   }
 
  private:
-  static int  _previous_version_count;
+  static bool  _has_previous_versions;
  public:
-  static void purge_previous_versions(InstanceKlass* ik);
-  static bool has_previous_versions() { return _previous_version_count > 0; }
+  static void purge_previous_versions(InstanceKlass* ik) {
+    if (ik->has_been_redefined()) {
+      ik->purge_previous_version_list();
+    }
+  }
+
+  static bool has_previous_versions_and_reset();
 
   // JVMTI: Support for caching a class file before it is modified by an agent that can do retransformation
   void set_cached_class_file(JvmtiCachedClassFileData *data) {
@@ -792,7 +798,7 @@
 #else // INCLUDE_JVMTI
 
   static void purge_previous_versions(InstanceKlass* ik) { return; };
-  static bool has_previous_versions() { return false; }
+  static bool has_previous_versions_and_reset() { return false; }
 
   void set_cached_class_file(JvmtiCachedClassFileData *data) {
     assert(data == NULL, "unexpected call with JVMTI disabled");
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/hotspot/test/runtime/RedefineTests/RedefinePreviousVersions.java	Wed Sep 07 15:25:21 2016 -0400
@@ -0,0 +1,120 @@
+/*
+ * Copyright (c) 2016, 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
+ * @bug 8165246
+ * @summary Test has_previous_versions flag and processing during class unloading.
+ * @library /test/lib
+ * @modules java.base/jdk.internal.misc
+ * @modules java.compiler
+ *          java.instrument
+ *          jdk.jartool/sun.tools.jar
+ * @run main RedefineClassHelper
+ * @run main/othervm RedefinePreviousVersions test
+ */
+
+import jdk.test.lib.process.ProcessTools;
+import jdk.test.lib.process.OutputAnalyzer;
+
+public class RedefinePreviousVersions {
+
+    public static String newB =
+                "class RedefinePreviousVersions$B {" +
+                "}";
+
+    static class B { }
+
+    public static String newRunning =
+        "class RedefinePreviousVersions$Running {" +
+        "    public static volatile boolean stop = true;" +
+        "    static void localSleep() { }" +
+        "    public static void infinite() { }" +
+        "}";
+
+    static class Running {
+        public static volatile boolean stop = false;
+        static void localSleep() {
+          try{
+            Thread.currentThread().sleep(10);//sleep for 10 ms
+          } catch(InterruptedException ie) {
+          }
+        }
+
+        public static void infinite() {
+            while (!stop) { localSleep(); }
+        }
+    }
+
+    public static void main(String[] args) throws Exception {
+
+        if (args.length > 0) {
+
+            String jarFile = System.getProperty("test.src") + "/testcase.jar";
+
+            // java -javaagent:redefineagent.jar -Xlog:stuff RedefinePreviousVersions
+            ProcessBuilder pb = ProcessTools.createJavaProcessBuilder( "-javaagent:redefineagent.jar",
+               "-Xlog:redefine+class+iklass+add=trace,redefine+class+iklass+purge=trace",
+               "RedefinePreviousVersions");
+            new OutputAnalyzer(pb.start())
+              .shouldContain("Class unloading: has_previous_versions = false")
+              .shouldContain("Class unloading: has_previous_versions = true")
+              .shouldHaveExitValue(0);
+            return;
+        }
+
+        // Redefine a class and create some garbage
+        // Since there are no methods running, the previous version is never added to the
+        // previous_version_list and the flag _has_previous_versions should stay false
+        RedefineClassHelper.redefineClass(B.class, newB);
+
+        for (int i = 0; i < 10 ; i++) {
+            String s = new String("some garbage");
+            System.gc();
+        }
+
+        // Start a class that has a method running
+        new Thread() {
+            public void run() {
+                Running.infinite();
+            }
+        }.start();
+
+        // Since a method of newRunning is running, this class should be added to the previous_version_list
+        // of Running, and _has_previous_versions should return true at class unloading.
+        RedefineClassHelper.redefineClass(Running.class, newRunning);
+
+        for (int i = 0; i < 10 ; i++) {
+            String s = new String("some garbage");
+            System.gc();
+        }
+
+        // purge should clean everything up, except Xcomp it might not.
+        Running.stop = true;
+
+        for (int i = 0; i < 10 ; i++) {
+            String s = new String("some garbage");
+            System.gc();
+        }
+    }
+}