Loom support for RedefineClasses

src/hotspot/share/oops/constantPool.cpp

@@ -32,6 +32,7 @@
 #include "classfile/systemDictionary.hpp"
 #include "classfile/vmClasses.hpp"
 #include "classfile/vmSymbols.hpp"
+#include "code/codeCache.hpp"
 #include "interpreter/bootstrapInfo.hpp"
 #include "interpreter/linkResolver.hpp"
 #include "logging/log.hpp"
@@ -2241,6 +2242,16 @@ int ConstantPool::copy_cpool_bytes(int cpool_size,
 
 #undef DBG
 
+// For redefinition, if any methods found in InstanceStackChunks, the marking_cycle is
+// recorded in their constant pool cache. The on_stack-ness of the constant pool controls whether
+// memory for the method is reclaimed.
+bool ConstantPool::on_stack() const {
+  if (_cache == NULL || _pool_holder == NULL) return false; // removed in loading
+
+  // See nmethod::is_not_on_continuation_stack for explanation of what this means.
+  bool not_on_vthread_stack = CodeCache::marking_cycle() >= align_up(cache()->marking_cycle(), 2) + 2;
+  return (_flags &_on_stack) != 0 || !not_on_vthread_stack;
+}
 
 void ConstantPool::set_on_stack(const bool value) {
   if (value) {

src/hotspot/share/oops/constantPool.hpp

@@ -228,7 +228,7 @@ class ConstantPool : public Metadata {
   // is on the executing stack, or as a handle in vm code, this constant pool
   // can't be removed from the set of previous versions saved in the instance
   // class.
-  bool on_stack() const                      { return (_flags &_on_stack) != 0; }
+  bool on_stack() const;
   void set_on_stack(const bool value);
 
   // Faster than MetaspaceObj::is_shared() - used by set_on_stack()

src/hotspot/share/oops/cpCache.cpp

@@ -27,6 +27,7 @@
 #include "classfile/resolutionErrors.hpp"
 #include "classfile/systemDictionary.hpp"
 #include "classfile/vmClasses.hpp"
+#include "code/codeCache.hpp"
 #include "interpreter/bytecodeStream.hpp"
 #include "interpreter/bytecodes.hpp"
 #include "interpreter/interpreter.hpp"
@@ -705,6 +706,11 @@ void ConstantPoolCache::initialize(const intArray& inverse_index_map,
   }
 }
 
+// Record the GC marking cycle when redefined vs. when found in the InstanceStackChunks.
+void ConstantPoolCache::record_marking_cycle() {
+  _marking_cycle = CodeCache::marking_cycle();
+}
+
 void ConstantPoolCache::verify_just_initialized() {
   DEBUG_ONLY(walk_entries_for_initialization(/*check_only = */ true));
 }

src/hotspot/share/oops/cpCache.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2021, 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
@@ -414,6 +414,10 @@ class ConstantPoolCache: public MetaspaceObj {
   // object index to original constant pool index
   OopHandle            _resolved_references;
   Array<u2>*           _reference_map;
+
+  // RedefineClasses support
+  uint64_t             _marking_cycle;
+
   // The narrowOop pointer to the archived resolved_references. Set at CDS dump
   // time when caching java heap object is supported.
   CDS_JAVA_HEAP_ONLY(int _archived_references_index;)
@@ -507,6 +511,8 @@ class ConstantPoolCache: public MetaspaceObj {
   DEBUG_ONLY(bool on_stack() { return false; })
   void deallocate_contents(ClassLoaderData* data);
   bool is_klass() const { return false; }
+  void record_marking_cycle();
+  uint64_t marking_cycle() { return _marking_cycle; }
 
   // Printing
   void print_on(outputStream* st) const;

src/hotspot/share/oops/cpCache.inline.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2018, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2018, 2021, 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
@@ -88,7 +88,8 @@ inline ConstantPoolCache::ConstantPoolCache(int length,
                                             const intStack& invokedynamic_inverse_index_map,
                                             const intStack& invokedynamic_references_map) :
                                                   _length(length),
-                                                  _constant_pool(NULL) {
+                                                  _constant_pool(NULL),
+                                                  _marking_cycle(0) {
   CDS_JAVA_HEAP_ONLY(_archived_references_index = -1;)
   initialize(inverse_index_map, invokedynamic_inverse_index_map,
              invokedynamic_references_map);

src/hotspot/share/oops/instanceKlass.cpp

@@ -3978,30 +3978,6 @@ void InstanceKlass::purge_previous_version_list() {
       _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();
@@ -4101,29 +4077,6 @@ void InstanceKlass::add_previous_version(InstanceKlass* scratch_class,
     return;
   }
 
-  if (emcp_method_count != 0) {
-    // At least one method is still running, check for EMCP methods
-    for (int i = 0; i < old_methods->length(); i++) {
-      Method* old_method = old_methods->at(i);
-      if (!old_method->is_obsolete() && old_method->on_stack()) {
-        // if EMCP method (not obsolete) is on the stack, mark as EMCP so that
-        // we can add breakpoints for it.
-
-        // We set the method->on_stack bit during safepoints for class redefinition
-        // and use this bit to set the is_running_emcp bit.
-        // After the safepoint, the on_stack bit is cleared and the running emcp
-        // method may exit.   If so, we would set a breakpoint in a method that
-        // is never reached, but this won't be noticeable to the programmer.
-        old_method->set_running_emcp(true);
-        log_trace(redefine, class, iklass, add)
-          ("EMCP method %s is on_stack " INTPTR_FORMAT, old_method->name_and_sig_as_C_string(), p2i(old_method));
-      } else if (!old_method->is_obsolete()) {
-        log_trace(redefine, class, iklass, add)
-          ("EMCP method %s is NOT on_stack " INTPTR_FORMAT, old_method->name_and_sig_as_C_string(), p2i(old_method));
-      }
-    }
-  }
-
   // Add previous version if any methods are still running.
   // Set has_previous_version flag for processing during class unloading.
   _has_previous_versions = true;

src/hotspot/share/oops/instanceStackChunkKlass.inline.hpp

@@ -790,11 +790,18 @@ inline BitMap::idx_t InstanceStackChunkKlass::bit_offset(int stack_size_in_words
 
 template <bool mixed>
 void InstanceStackChunkKlass::run_nmethod_entry_barrier_if_needed(const StackChunkFrameStream<mixed>& f) {
-  CodeBlob* cb = f.cb();
-  if ((mixed && cb == nullptr) || !cb->is_nmethod()) return;
-  nmethod* nm = cb->as_nmethod();
-  if (BarrierSet::barrier_set()->barrier_set_nmethod()->is_armed(nm)) {
-    nm->run_nmethod_entry_barrier();
+  if (f.is_interpreted()) {
+    // Mark interpreted frames for marking_cycle
+    Method* im = f.to_frame().interpreter_frame_method();
+    im->record_marking_cycle();
+  } else {
+    CodeBlob* cb = f.cb();
+    if (cb->is_nmethod()) {
+      nmethod* nm = cb->as_nmethod();
+      if (BarrierSet::barrier_set()->barrier_set_nmethod()->is_armed(nm)) {
+        nm->run_nmethod_entry_barrier();
+      }
+    }
   }
 }
 

src/hotspot/share/oops/method.cpp

@@ -2255,8 +2255,13 @@ void Method::set_on_stack(const bool value) {
   if (value && !already_set) {
     MetadataOnStackMark::record(this);
   }
-  assert(!value || !is_old() || is_obsolete() || is_running_emcp(),
-         "emcp methods cannot run after emcp bit is cleared");
+}
+
+void Method::record_marking_cycle() {
+  // If any method is on the stack in continuations, none of them can be reclaimed,
+  // so save the marking cycle to check for the whole class in the cpCache.
+  // The cpCache is writeable.
+  constants()->cache()->record_marking_cycle();
 }
 
 // Called when the class loader is unloaded to make all methods weak.

src/hotspot/share/oops/method.hpp

@@ -89,12 +89,11 @@ class Method : public Metadata {
     _dont_inline           = 1 << 2,
     _hidden                = 1 << 3,
     _has_injected_profile  = 1 << 4,
-    _running_emcp          = 1 << 5,
-    _intrinsic_candidate   = 1 << 6,
-    _reserved_stack_access = 1 << 7,
-    _scoped                = 1 << 8,
-    _changes_current_thread = 1 << 9,
-    _jvmti_mount_transition = 1 << 10,
+    _intrinsic_candidate   = 1 << 5,
+    _reserved_stack_access = 1 << 6,
+    _scoped                = 1 << 7,
+    _changes_current_thread = 1 << 8,
+    _jvmti_mount_transition = 1 << 9,
   };
   mutable u2 _flags;
 
@@ -758,23 +757,11 @@ class Method : public Metadata {
   bool is_deleted() const                           { return access_flags().is_deleted(); }
   void set_is_deleted()                             { _access_flags.set_is_deleted(); }
 
-  bool is_running_emcp() const {
-    // EMCP methods are old but not obsolete or deleted. Equivalent
-    // Modulo Constant Pool means the method is equivalent except
-    // the constant pool and instructions that access the constant
-    // pool might be different.
-    // If a breakpoint is set in a redefined method, its EMCP methods that are
-    // still running must have a breakpoint also.
-    return (_flags & _running_emcp) != 0;
-  }
-
-  void set_running_emcp(bool x) {
-    _flags = x ? (_flags | _running_emcp) : (_flags & ~_running_emcp);
-  }
-
   bool on_stack() const                             { return access_flags().on_stack(); }
   void set_on_stack(const bool value);
 
+  void record_marking_cycle();
+
   // see the definition in Method*.cpp for the gory details
   bool should_not_be_cached() const;
 

src/hotspot/share/prims/jvmtiImpl.cpp

@@ -245,7 +245,16 @@ void JvmtiBreakpoint::each_method_version_do(method_action meth_act) {
     for (int i = methods->length() - 1; i >= 0; i--) {
       Method* method = methods->at(i);
       // Only set breakpoints in running EMCP methods.
-      if (method->is_running_emcp() &&
+      // EMCP methods are old but not obsolete. Equivalent
+      // Modulo Constant Pool means the method is equivalent except
+      // the constant pool and instructions that access the constant
+      // pool might be different.
+      // If a breakpoint is set in a redefined method, its EMCP methods
+      // must have a breakpoint also.
+      // None of the methods are deleted until none are running.
+      // This code could set a breakpoint in a method that
+      // is never reached, but this won't be noticeable to the programmer.
+      if (!method->is_obsolete() &&
           method->name() == m_name &&
           method->signature() == m_signature) {
         ResourceMark rm;

src/hotspot/share/runtime/continuation.cpp

@@ -1497,8 +1497,10 @@ class Freeze {
   }
 #endif
 
+    Method* frame_method = Frame::frame_method(f);
+
     log_develop_trace(jvmcont)("recurse_freeze_interpreted_frame %s _size: %d fsize: %d argsize: %d callee_interpreted: %d callee_argsize: %d :: " INTPTR_FORMAT " - " INTPTR_FORMAT,
-      Frame::frame_method(f)->name_and_sig_as_C_string(), _size, fsize, argsize, callee_interpreted, callee_argsize, p2i(vsp), p2i(vsp+fsize));
+      frame_method->name_and_sig_as_C_string(), _size, fsize, argsize, callee_interpreted, callee_argsize, p2i(vsp), p2i(vsp+fsize));
 
     freeze_result result = recurse_freeze_java_frame<Interpreted>(f, caller, fsize, argsize);
     if (UNLIKELY(result > freeze_ok_bottom)) return result;
@@ -1520,6 +1522,10 @@ class Freeze {
     _cont.inc_num_interpreted_frames();
     DEBUG_ONLY(after_freeze_java_frame(hf, bottom);)
     caller = hf;
+
+    // Mark frame_method's marking cycle for GC and redefinition on_stack calculation.
+    frame_method->record_marking_cycle();
+
     return freeze_ok;
   }
 

test/hotspot/jtreg/ProblemList.txt

@@ -167,7 +167,6 @@ compiler/vectorization/TestMacroLogicVector.java                              82
 compiler/whitebox/ForceNMethodSweepTest.java                                  8265181 generic-all
 
 runtime/clinit/ClassInitBarrier.java                                          8254936 generic-all
-runtime/vthread/RedefineClass.java                                            8253378 generic-all
 runtime/vthread/TestObjectAllocationSampleEvent.java                          8261379 generic-all
 
 serviceability/jvmti/vthread/DoContinueSingleStepTest/DoContinueSingleStepTest.java    0000000 generic-all

test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineLeak.java

@@ -62,6 +62,7 @@ public byte[] transform(ClassLoader loader, String className, Class classBeingRe
     }
 
     public static void premain(String agentArgs, Instrumentation inst) throws Exception {
+        System.gc();
         LoggingTransformer t = new LoggingTransformer();
         inst.addTransformer(t, true);
         {

test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefinePreviousVersions.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 2021, 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
@@ -91,6 +91,9 @@ public static void main(String[] args) throws Exception {
             return;
         }
 
+        // Start with a full GC.
+        System.gc();
+
         // 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

test/hotspot/jtreg/serviceability/jvmti/vthread/RedefineClasses/RedefineRunningMethods.java

@@ -0,0 +1,179 @@
+/*
+ * Copyright (c) 2014, 2019, 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 8055008 8197901 8010319
+ * @summary Redefine EMCP and non-EMCP methods that are running in an infinite loop
+ * @requires vm.jvmti
+ * @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/timeout=180 -Xint -javaagent:redefineagent.jar -Xlog:redefine+class+iklass+add=trace,redefine+class+iklass+purge=trace,class+loader+data=debug,safepoint+cleanup,gc+phases=debug:rt.log RedefineRunningMethods
+ */
+
+
+class RedefineContinuation {
+    static final ContinuationScope FOO = new ContinuationScope() {};
+
+    static void moveToHeap(int a) {
+        boolean res = Continuation.yield(FOO);
+    }
+}
+
+// package access top-level class to avoid problem with RedefineClassHelper
+// and nested types.
+class RedefineRunningMethods_B {
+    static int count1 = 0;
+    static int count2 = 0;
+    public static volatile boolean stop = false;
+    static void localSleep() {
+        RedefineContinuation.moveToHeap(1);
+    }
+
+    public static void infinite() {
+        while (!stop) { count1++; localSleep(); }
+    }
+    public static void infinite_emcp() {
+        System.out.println("infinite_emcp called");
+        while (!stop) { count2++; localSleep(); }
+    }
+}
+
+
+public class RedefineRunningMethods {
+
+    public static String newB =
+                "class RedefineRunningMethods_B {" +
+                "   static int count1 = 0;" +
+                "   static int count2 = 0;" +
+                "   public static volatile boolean stop = false;" +
+                "  static void localSleep() { " +
+                "      RedefineContinuation.moveToHeap(2);" +
+                " } " +
+                "   public static void infinite() { " +
+                "       System.out.println(\"infinite called\");" +
+                "   }" +
+                "   public static void infinite_emcp() { " +
+                "       System.out.println(\"infinite_emcp called\");" +
+                "       while (!stop) { count2++; localSleep(); }" +
+                "   }" +
+                "}";
+
+    public static String evenNewerB =
+                "class RedefineRunningMethods_B {" +
+                "   static int count1 = 0;" +
+                "   static int count2 = 0;" +
+                "   public static volatile boolean stop = false;" +
+                "  static void localSleep() { " +
+                "      RedefineContinuation.moveToHeap(3);" +
+                " } " +
+                "   public static void infinite() { }" +
+                "   public static void infinite_emcp() { " +
+                "       System.out.println(\"infinite_emcp now obsolete called\");" +
+                "   }" +
+                "}";
+
+    static void test_redef_emcp() {
+        System.out.println("test_redef");
+        Continuation cont = new Continuation(RedefineContinuation.FOO, ()-> {
+              RedefineRunningMethods_B.infinite_emcp();
+        });
+
+        while (!cont.isDone()) {
+            cont.run();
+            // System.gc();
+        }
+    }
+
+    static void test_redef_infinite() {
+        System.out.println("test_redef");
+        Continuation cont = new Continuation(RedefineContinuation.FOO, ()-> {
+              RedefineRunningMethods_B.infinite();
+        });
+
+        while (!cont.isDone()) {
+            cont.run();
+            // System.gc();
+        }
+    }
+
+    public static void main(String[] args) throws Exception {
+
+        // Start with GC
+        System.gc();
+
+        new Thread() {
+            public void run() {
+                test_redef_infinite();
+            }
+        }.start();
+
+        new Thread() {
+            public void run() {
+                test_redef_emcp();
+            }
+        }.start();
+
+        RedefineClassHelper.redefineClass(RedefineRunningMethods_B.class, newB);
+
+        System.gc();
+
+        RedefineRunningMethods_B.infinite();
+
+        // Start a thread with the second version of infinite_emcp running
+        new Thread() {
+            public void run() {
+                test_redef_emcp();
+            }
+        }.start();
+
+        for (int i = 0; i < 20 ; i++) {
+            String s = new String("some garbage");
+            System.gc();
+        }
+
+        RedefineClassHelper.redefineClass(RedefineRunningMethods_B.class, evenNewerB);
+        System.gc();
+
+        for (int i = 0; i < 20 ; i++) {
+            RedefineRunningMethods_B.infinite();
+            String s = new String("some garbage");
+            System.gc();
+        }
+
+        RedefineRunningMethods_B.infinite_emcp();
+
+        // purge should clean everything up.
+        RedefineRunningMethods_B.stop = true;
+
+        for (int i = 0; i < 20 ; i++) {
+            // RedefineRunningMethods_B.infinite();
+            String s = new String("some garbage");
+            System.gc();
+        }
+    }
+}