8306929: Avoid CleanClassLoaderDataMetaspaces safepoints when previous versions are shared

Reviewed-by: coleenp, sspitsyn

src/hotspot/share/classfile/classLoaderDataGraph.cpp

@@ -194,7 +194,7 @@ void ClassLoaderDataGraph::walk_metadata_and_clean_metaspaces() {
   // on the stack or in the code cache, so we only have to repeat the full walk if
   // they were found at that time.
   // TODO: have redefinition clean old methods out of the code cache.  They still exist in some places.
-  bool walk_all_metadata = InstanceKlass::has_previous_versions_and_reset();
+  bool walk_all_metadata = InstanceKlass::should_clean_previous_versions_and_reset();
 
   MetadataOnStackMark md_on_stack(walk_all_metadata, /*redefinition_walk*/false);
   clean_deallocate_lists(walk_all_metadata);

src/hotspot/share/classfile/classLoaderDataGraph.inline.hpp

@@ -73,7 +73,7 @@ bool ClassLoaderDataGraph::should_clean_metaspaces_and_reset() {
   // Only clean metaspaces after full GC.
   bool do_cleaning = _safepoint_cleanup_needed;
 #if INCLUDE_JVMTI
-  do_cleaning = do_cleaning && (_should_clean_deallocate_lists || InstanceKlass::has_previous_versions());
+  do_cleaning = do_cleaning && (_should_clean_deallocate_lists || InstanceKlass::should_clean_previous_versions());
 #else
   do_cleaning = do_cleaning && _should_clean_deallocate_lists;
 #endif

src/hotspot/share/oops/instanceKlass.cpp

@@ -4022,18 +4022,18 @@ void InstanceKlass::set_init_state(ClassState state) {
 // 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;
+bool InstanceKlass::_should_clean_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::clean_deallocate_lists().
-bool InstanceKlass::has_previous_versions_and_reset() {
+bool InstanceKlass::should_clean_previous_versions_and_reset() {
-  bool ret = _has_previous_versions;
+  bool ret = _should_clean_previous_versions;
-  log_trace(redefine, class, iklass, purge)("Class unloading: has_previous_versions = %s",
+  log_trace(redefine, class, iklass, purge)("Class unloading: should_clean_previous_versions = %s",
      ret ? "true" : "false");
-  _has_previous_versions = false;
+  _should_clean_previous_versions = false;
   return ret;
 }
 
@@ -4090,12 +4090,17 @@ void InstanceKlass::purge_previous_version_list() {
       version++;
       continue;
     } else {
-      log_trace(redefine, class, iklass, purge)("previous version " PTR_FORMAT " is alive", p2i(pv_node));
       assert(pvcp->pool_holder() != nullptr, "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
+      if (pvcp->is_shared()) {
-      _has_previous_versions = true;
+        // Shared previous versions can never be removed so no cleaning is needed.
+        log_trace(redefine, class, iklass, purge)("previous version " PTR_FORMAT " is shared", p2i(pv_node));
+      } else {
+        // Previous version alive, set that clean is needed for next time.
+        _should_clean_previous_versions = true;
+        log_trace(redefine, class, iklass, purge)("previous version " PTR_FORMAT " is alive", p2i(pv_node));
+      }
     }
 
     // next previous version
@@ -4195,13 +4200,19 @@ void InstanceKlass::add_previous_version(InstanceKlass* scratch_class,
     return;
   }
 
-  // Add previous version if any methods are still running.
+  // Add previous version if any methods are still running or if this is
-  // Set has_previous_version flag for processing during class unloading.
+  // a shared class which should never be removed.
-  _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() == nullptr, "shouldn't have a previous version");
   scratch_class->link_previous_versions(previous_versions());
   link_previous_versions(scratch_class);
+  if (cp_ref->is_shared()) {
+    log_trace(redefine, class, iklass, add) ("scratch class added; class is shared");
+  } else {
+    //  We only set clean_previous_versions flag for processing during class
+    // unloading for non-shared classes.
+    _should_clean_previous_versions = true;
+    log_trace(redefine, class, iklass, add) ("scratch class added; one of its methods is on_stack.");
+  }
 } // end add_previous_version()
 
 #endif // INCLUDE_JVMTI

src/hotspot/share/oops/instanceKlass.hpp

@@ -715,7 +715,7 @@ public:
   }
 
  private:
-  static bool  _has_previous_versions;
+  static bool  _should_clean_previous_versions;
  public:
   static void purge_previous_versions(InstanceKlass* ik) {
     if (ik->has_been_redefined()) {
@@ -723,8 +723,8 @@ public:
     }
   }
 
-  static bool has_previous_versions_and_reset();
+  static bool should_clean_previous_versions_and_reset();
-  static bool has_previous_versions() { return _has_previous_versions; }
+  static bool should_clean_previous_versions() { return _should_clean_previous_versions; }
 
   // 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) {
@@ -744,7 +744,7 @@ public:
 #else // INCLUDE_JVMTI
 
   static void purge_previous_versions(InstanceKlass* ik) { return; };
-  static bool has_previous_versions_and_reset() { return false; }
+  static bool should_clean_previous_versions_and_reset() { return false; }
 
   void set_cached_class_file(JvmtiCachedClassFileData *data) {
     assert(data == nullptr, "unexpected call with JVMTI disabled");

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

@@ -24,7 +24,7 @@
 /*
  * @test
  * @bug 8165246 8010319
- * @summary Test has_previous_versions flag and processing during class unloading.
+ * @summary Test clean_previous_versions flag and processing during class unloading.
  * @requires vm.jvmti
  * @requires vm.opt.final.ClassUnloading
  * @requires vm.flagless
@@ -88,8 +88,8 @@ public class RedefinePreviousVersions {
                "-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: should_clean_previous_versions = false")
-              .shouldContain("Class unloading: has_previous_versions = true")
+              .shouldContain("Class unloading: should_clean_previous_versions = true")
               .shouldHaveExitValue(0);
             return;
         }
@@ -99,7 +99,7 @@ public class RedefinePreviousVersions {
 
         // 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
+        // previous_version_list and the flag _should_clean_previous_versions should stay false
         RedefineClassHelper.redefineClass(RedefinePreviousVersions_B.class, newB);
 
         for (int i = 0; i < 10 ; i++) {
@@ -119,7 +119,7 @@ public class RedefinePreviousVersions {
         }
 
         // 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.
+        // of Running, and _should_clean_previous_versions should return true at class unloading.
         RedefineClassHelper.redefineClass(RedefinePreviousVersions_Running.class, newRunning);
 
         for (int i = 0; i < 10 ; i++) {

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

@@ -0,0 +1,113 @@
+/*
+ * Copyright (c) 2023, 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 8306929
+ * @summary Verify clean_previous_versions when run with JFR and CDS
+ * @requires vm.jvmti
+ * @requires vm.cds
+ * @requires vm.hasJFR
+ * @requires vm.opt.final.ClassUnloading
+ * @requires vm.flagless
+ * @library /test/lib
+ * @run driver RedefineSharedClassJFR xshare-off
+ * @run driver RedefineSharedClassJFR xshare-on
+ */
+import java.util.ArrayList;
+import java.util.List;
+
+import jdk.test.lib.Platform;
+import jdk.test.lib.process.ProcessTools;
+import jdk.test.lib.process.OutputAnalyzer;
+
+import jtreg.SkippedException;
+
+public class RedefineSharedClassJFR {
+
+    private static final String SHOULD_CLEAN_TRUE = "Class unloading: should_clean_previous_versions = true";
+    private static final String SHOULD_CLEAN_FALSE = "Class unloading: should_clean_previous_versions = false";
+    private static final String SCRATCH_CLASS_ADDED_SHARED = "scratch class added; class is shared";
+    private static final String SCRATCH_CLASS_ADDED_ON_STACK = "scratch class added; one of its methods is on_stack.";
+
+    public static void main(String[] args) throws Exception {
+        // Skip test if default archive is supported.
+        if (!Platform.isDefaultCDSArchiveSupported()) {
+            throw new SkippedException("Supported platform");
+        }
+
+        // Test is run with JFR which will transform a number of classes. Depending
+        // on if the test is run with or without CDS the output will be different,
+        // due to the fact that shared classes can never be cleaned out after retranform.
+        if (args.length > 0) {
+            // When run with an argument the class is used as driver and should parse
+            // the output to verify it is correct given the command line.
+            List<String> baseCommand = List.of(
+                "-XX:StartFlightRecording",
+                "-Xlog:redefine+class+iklass+add=trace,redefine+class+iklass+purge=trace",
+                "RedefineSharedClassJFR");
+
+            if (args[0].equals("xshare-off")) {
+                // First case is with -Xshare:off. In this case no classes are shared
+                // and we should be able to clean out the retransformed classes. Verify
+                // that the cleaning is done when the GC is triggered.
+                List<String> offCommand = new ArrayList<>();
+                offCommand.add("-Xshare:off");
+                offCommand.addAll(baseCommand);
+                ProcessBuilder pb = ProcessTools.createJavaProcessBuilder(offCommand);
+                new OutputAnalyzer(pb.start())
+                    .shouldContain(SHOULD_CLEAN_TRUE)
+                    .shouldNotContain(SHOULD_CLEAN_FALSE)
+                    // We expect at least one of the transformed classes to be in use, if
+                    // not the above check that should_clean_previous should be true will also
+                    // fail. This check is to show what is expected.
+                    .shouldContain(SCRATCH_CLASS_ADDED_ON_STACK)
+                    .shouldNotContain(SCRATCH_CLASS_ADDED_SHARED)
+                    .shouldHaveExitValue(0);
+                return;
+            } else if (args[0].equals("xshare-on")) {
+                // With -Xshare:on, the shared classes can never be cleaned out. Check the
+                // logs to verify we don't try to clean when we know it is not needed.
+                List<String> onCommand = new ArrayList<>();
+                onCommand.add("-Xshare:on");
+                onCommand.addAll(baseCommand);
+                ProcessBuilder pb = ProcessTools.createJavaProcessBuilder(onCommand);
+                new OutputAnalyzer(pb.start())
+                    .shouldContain(SHOULD_CLEAN_FALSE)
+                    .shouldNotContain(SHOULD_CLEAN_TRUE)
+                    .shouldContain(SCRATCH_CLASS_ADDED_SHARED)
+                    // If the below line occurs, then should_clean_previous_versions will be
+                    // true and the above shouldContain will trigger. This check is to
+                    // show the intention that we don't expect any non-shared transformed
+                    // classes to be in use.
+                    .shouldNotContain(SCRATCH_CLASS_ADDED_ON_STACK)
+                    .shouldHaveExitValue(0);
+                return;
+            }
+        }
+
+        // When run without any argument this class acts as test and we do a system GC
+        // to trigger cleaning and get the output we want to check.
+        System.gc();
+    }
+}