8164692: InstanceKlass::_previous_version_count goes negative
authorcoleenp
Wed, 31 Aug 2016 06:35:19 -0400
changeset 40894 7d17619c0140
parent 40893 12787d18650e
child 40895 791a3458971d
child 40896 cff45787a061
8164692: InstanceKlass::_previous_version_count goes negative Summary: decrement previous_version_count when it's removed from the list. Reviewed-by: dcubed, dlong, 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/RedefineCount.java
--- a/hotspot/src/share/vm/classfile/classLoaderData.cpp	Wed Aug 31 11:47:14 2016 +0300
+++ b/hotspot/src/share/vm/classfile/classLoaderData.cpp	Wed Aug 31 06:35:19 2016 -0400
@@ -962,7 +962,7 @@
 
   // Mark metadata seen on the stack only so we can delete unneeded entries.
   // Only walk all metadata, including the expensive code cache walk, for Full GC
-  // and only if class redefinition and if there's previous versions of
+  // and only if class redefinition occurred and if there are previous versions of
   // Klasses to delete.
   bool walk_all_metadata = clean_previous_versions &&
                            JvmtiExport::has_redefined_a_class() &&
--- a/hotspot/src/share/vm/oops/instanceKlass.cpp	Wed Aug 31 11:47:14 2016 +0300
+++ b/hotspot/src/share/vm/oops/instanceKlass.cpp	Wed Aug 31 06:35:19 2016 -0400
@@ -3370,6 +3370,7 @@
 
 // Purge previous versions before adding new previous versions of the class.
 void InstanceKlass::purge_previous_versions(InstanceKlass* ik) {
+  assert(SafepointSynchronize::is_at_safepoint(), "only called at safepoint");
   if (ik->previous_versions() != NULL) {
     // This klass has previous versions so see what we can cleanup
     // while it is safe to do so.
@@ -3398,7 +3399,12 @@
         // 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));
+        //
+        // Update count for class unloading.
+        _previous_version_count--;
+        log_trace(redefine, class, iklass, purge)
+          ("previous version " INTPTR_FORMAT " is dead.  previous_version_count = %d",
+           p2i(pv_node), _previous_version_count);
         // For debugging purposes.
         pv_node->set_is_scratch_class();
         pv_node->class_loader_data()->add_to_deallocate_list(pv_node);
@@ -3513,6 +3519,7 @@
                                          int emcp_method_count) {
   assert(Thread::current()->is_VM_thread(),
          "only VMThread can add previous versions");
+  assert(SafepointSynchronize::is_at_safepoint(), "only called at safepoint");
 
   ResourceMark rm;
   log_trace(redefine, class, iklass, add)
@@ -3536,8 +3543,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 +3570,14 @@
   }
 
   // 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");
+  // Update count for class unloading.
+  _previous_version_count++;
+  log_trace(redefine, class, iklass, add)
+    ("scratch class added; one of its methods is on_stack.  previous_version_count = %d",
+      _previous_version_count);
   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 Aug 31 11:47:14 2016 +0300
+++ b/hotspot/src/share/vm/oops/instanceKlass.hpp	Wed Aug 31 06:35:19 2016 -0400
@@ -771,7 +771,10 @@
   static int  _previous_version_count;
  public:
   static void purge_previous_versions(InstanceKlass* ik);
-  static bool has_previous_versions() { return _previous_version_count > 0; }
+  static bool has_previous_versions() {
+    assert(_previous_version_count >= 0, "count should never be negative");
+    return _previous_version_count > 0;
+  }
 
   // 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) {
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/hotspot/test/runtime/RedefineTests/RedefineCount.java	Wed Aug 31 06:35:19 2016 -0400
@@ -0,0 +1,56 @@
+/*
+ * 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 8164692
+ * @summary Redefine previous_versions count goes negative
+ * @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 -javaagent:redefineagent.jar -Xlog:redefine+class+iklass+add=trace,redefine+class+iklass+purge=trace RedefineCount
+ */
+public class RedefineCount {
+
+    public static String newB =
+                "class RedefineCount$B {" +
+                "}";
+
+    static class B { }
+
+    public static void main(String[] args) throws Exception {
+
+        // 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 count should stay zero and not go negative
+        RedefineClassHelper.redefineClass(B.class, newB);
+
+        for (int i = 0; i < 20 ; i++) {
+            String s = new String("some garbage");
+            System.gc();
+        }
+    }
+}