8165246: [REDO] InstanceKlass::_previous_version_count goes negative

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

@@ -966,7 +966,7 @@ bool ClassLoaderDataGraph::do_unloading(BoolObjectClosure* is_alive_closure,
   // 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

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

@@ -3365,26 +3365,51 @@ void InstanceKlass::set_init_state(ClassState state) {
 
 #if INCLUDE_JVMTI
 
-// RedefineClasses() support for previous versions:
-int InstanceKlass::_previous_version_count = 0;
+// 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;
+  }
 
-// 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();
+  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", ik->external_name());
+  log_trace(redefine, class, iklass, purge)("%s: previous versions", external_name());
 
   // previous versions are linked together through the InstanceKlass
-    InstanceKlass* pv_node = ik->previous_versions();
-    InstanceKlass* last = ik;
+  InstanceKlass* pv_node = previous_versions();
+  InstanceKlass* last = this;
   int version = 0;
 
   // check the previous versions list
@@ -3398,12 +3423,18 @@ void InstanceKlass::purge_previous_versions(InstanceKlass* ik) {
       // 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));
+      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);
+      // 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;
@@ -3412,6 +3443,8 @@ void InstanceKlass::purge_previous_versions(InstanceKlass* ik) {
       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.
@@ -3444,9 +3477,7 @@ void InstanceKlass::purge_previous_versions(InstanceKlass* ik) {
     version++;
   }
   log_trace(redefine, class, iklass, purge)
-      ("previous version stats: live=%d, deleted=%d",
-       live_count, deleted_count);
-  }
+    ("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 @@ void InstanceKlass::add_previous_version(instanceKlassHandle scratch_class,
   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 @@ void InstanceKlass::add_previous_version(instanceKlassHandle scratch_class,
     // 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 @@ void InstanceKlass::add_previous_version(instanceKlassHandle scratch_class,
   }
 
   // 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

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

@@ -709,6 +709,7 @@ class InstanceKlass: public Klass {
 
   // 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 @@ public:
   }
 
  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 @@ public:
 #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");

hotspot/test/runtime/RedefineTests/RedefinePreviousVersions.java

@@ -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();
+        }
+    }
+}