Skip to content

Commit 408cec5

Browse files
committed
8306929: Avoid CleanClassLoaderDataMetaspaces safepoints when previous versions are shared
Reviewed-by: coleenp, sspitsyn
1 parent cecf817 commit 408cec5

File tree

6 files changed

+147
-23
lines changed

6 files changed

+147
-23
lines changed

src/hotspot/share/classfile/classLoaderDataGraph.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -194,7 +194,7 @@ void ClassLoaderDataGraph::walk_metadata_and_clean_metaspaces() {
194194
// on the stack or in the code cache, so we only have to repeat the full walk if
195195
// they were found at that time.
196196
// TODO: have redefinition clean old methods out of the code cache. They still exist in some places.
197-
bool walk_all_metadata = InstanceKlass::has_previous_versions_and_reset();
197+
bool walk_all_metadata = InstanceKlass::should_clean_previous_versions_and_reset();
198198

199199
MetadataOnStackMark md_on_stack(walk_all_metadata, /*redefinition_walk*/false);
200200
clean_deallocate_lists(walk_all_metadata);

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ bool ClassLoaderDataGraph::should_clean_metaspaces_and_reset() {
7373
// Only clean metaspaces after full GC.
7474
bool do_cleaning = _safepoint_cleanup_needed;
7575
#if INCLUDE_JVMTI
76-
do_cleaning = do_cleaning && (_should_clean_deallocate_lists || InstanceKlass::has_previous_versions());
76+
do_cleaning = do_cleaning && (_should_clean_deallocate_lists || InstanceKlass::should_clean_previous_versions());
7777
#else
7878
do_cleaning = do_cleaning && _should_clean_deallocate_lists;
7979
#endif

src/hotspot/share/oops/instanceKlass.cpp

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -4022,18 +4022,18 @@ void InstanceKlass::set_init_state(ClassState state) {
40224022
// Globally, there is at least one previous version of a class to walk
40234023
// during class unloading, which is saved because old methods in the class
40244024
// are still running. Otherwise the previous version list is cleaned up.
4025-
bool InstanceKlass::_has_previous_versions = false;
4025+
bool InstanceKlass::_should_clean_previous_versions = false;
40264026

40274027
// Returns true if there are previous versions of a class for class
40284028
// unloading only. Also resets the flag to false. purge_previous_version
40294029
// will set the flag to true if there are any left, i.e., if there's any
40304030
// work to do for next time. This is to avoid the expensive code cache
40314031
// walk in CLDG::clean_deallocate_lists().
4032-
bool InstanceKlass::has_previous_versions_and_reset() {
4033-
bool ret = _has_previous_versions;
4034-
log_trace(redefine, class, iklass, purge)("Class unloading: has_previous_versions = %s",
4032+
bool InstanceKlass::should_clean_previous_versions_and_reset() {
4033+
bool ret = _should_clean_previous_versions;
4034+
log_trace(redefine, class, iklass, purge)("Class unloading: should_clean_previous_versions = %s",
40354035
ret ? "true" : "false");
4036-
_has_previous_versions = false;
4036+
_should_clean_previous_versions = false;
40374037
return ret;
40384038
}
40394039

@@ -4090,12 +4090,17 @@ void InstanceKlass::purge_previous_version_list() {
40904090
version++;
40914091
continue;
40924092
} else {
4093-
log_trace(redefine, class, iklass, purge)("previous version " PTR_FORMAT " is alive", p2i(pv_node));
40944093
assert(pvcp->pool_holder() != nullptr, "Constant pool with no holder");
40954094
guarantee (!loader_data->is_unloading(), "unloaded classes can't be on the stack");
40964095
live_count++;
4097-
// found a previous version for next time we do class unloading
4098-
_has_previous_versions = true;
4096+
if (pvcp->is_shared()) {
4097+
// Shared previous versions can never be removed so no cleaning is needed.
4098+
log_trace(redefine, class, iklass, purge)("previous version " PTR_FORMAT " is shared", p2i(pv_node));
4099+
} else {
4100+
// Previous version alive, set that clean is needed for next time.
4101+
_should_clean_previous_versions = true;
4102+
log_trace(redefine, class, iklass, purge)("previous version " PTR_FORMAT " is alive", p2i(pv_node));
4103+
}
40994104
}
41004105

41014106
// next previous version
@@ -4195,13 +4200,19 @@ void InstanceKlass::add_previous_version(InstanceKlass* scratch_class,
41954200
return;
41964201
}
41974202

4198-
// Add previous version if any methods are still running.
4199-
// Set has_previous_version flag for processing during class unloading.
4200-
_has_previous_versions = true;
4201-
log_trace(redefine, class, iklass, add) ("scratch class added; one of its methods is on_stack.");
4203+
// Add previous version if any methods are still running or if this is
4204+
// a shared class which should never be removed.
42024205
assert(scratch_class->previous_versions() == nullptr, "shouldn't have a previous version");
42034206
scratch_class->link_previous_versions(previous_versions());
42044207
link_previous_versions(scratch_class);
4208+
if (cp_ref->is_shared()) {
4209+
log_trace(redefine, class, iklass, add) ("scratch class added; class is shared");
4210+
} else {
4211+
// We only set clean_previous_versions flag for processing during class
4212+
// unloading for non-shared classes.
4213+
_should_clean_previous_versions = true;
4214+
log_trace(redefine, class, iklass, add) ("scratch class added; one of its methods is on_stack.");
4215+
}
42054216
} // end add_previous_version()
42064217

42074218
#endif // INCLUDE_JVMTI

src/hotspot/share/oops/instanceKlass.hpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -715,16 +715,16 @@ class InstanceKlass: public Klass {
715715
}
716716

717717
private:
718-
static bool _has_previous_versions;
718+
static bool _should_clean_previous_versions;
719719
public:
720720
static void purge_previous_versions(InstanceKlass* ik) {
721721
if (ik->has_been_redefined()) {
722722
ik->purge_previous_version_list();
723723
}
724724
}
725725

726-
static bool has_previous_versions_and_reset();
727-
static bool has_previous_versions() { return _has_previous_versions; }
726+
static bool should_clean_previous_versions_and_reset();
727+
static bool should_clean_previous_versions() { return _should_clean_previous_versions; }
728728

729729
// JVMTI: Support for caching a class file before it is modified by an agent that can do retransformation
730730
void set_cached_class_file(JvmtiCachedClassFileData *data) {
@@ -744,7 +744,7 @@ class InstanceKlass: public Klass {
744744
#else // INCLUDE_JVMTI
745745

746746
static void purge_previous_versions(InstanceKlass* ik) { return; };
747-
static bool has_previous_versions_and_reset() { return false; }
747+
static bool should_clean_previous_versions_and_reset() { return false; }
748748

749749
void set_cached_class_file(JvmtiCachedClassFileData *data) {
750750
assert(data == nullptr, "unexpected call with JVMTI disabled");

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

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@
2424
/*
2525
* @test
2626
* @bug 8165246 8010319
27-
* @summary Test has_previous_versions flag and processing during class unloading.
27+
* @summary Test clean_previous_versions flag and processing during class unloading.
2828
* @requires vm.jvmti
2929
* @requires vm.opt.final.ClassUnloading
3030
* @requires vm.flagless
@@ -88,8 +88,8 @@ public static void main(String[] args) throws Exception {
8888
"-Xlog:redefine+class+iklass+add=trace,redefine+class+iklass+purge=trace",
8989
"RedefinePreviousVersions");
9090
new OutputAnalyzer(pb.start())
91-
.shouldContain("Class unloading: has_previous_versions = false")
92-
.shouldContain("Class unloading: has_previous_versions = true")
91+
.shouldContain("Class unloading: should_clean_previous_versions = false")
92+
.shouldContain("Class unloading: should_clean_previous_versions = true")
9393
.shouldHaveExitValue(0);
9494
return;
9595
}
@@ -99,7 +99,7 @@ public static void main(String[] args) throws Exception {
9999

100100
// Redefine a class and create some garbage
101101
// Since there are no methods running, the previous version is never added to the
102-
// previous_version_list and the flag _has_previous_versions should stay false
102+
// previous_version_list and the flag _should_clean_previous_versions should stay false
103103
RedefineClassHelper.redefineClass(RedefinePreviousVersions_B.class, newB);
104104

105105
for (int i = 0; i < 10 ; i++) {
@@ -119,7 +119,7 @@ public void run() {
119119
}
120120

121121
// Since a method of newRunning is running, this class should be added to the previous_version_list
122-
// of Running, and _has_previous_versions should return true at class unloading.
122+
// of Running, and _should_clean_previous_versions should return true at class unloading.
123123
RedefineClassHelper.redefineClass(RedefinePreviousVersions_Running.class, newRunning);
124124

125125
for (int i = 0; i < 10 ; i++) {
Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,113 @@
1+
/*
2+
* Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
3+
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
4+
*
5+
* This code is free software; you can redistribute it and/or modify it
6+
* under the terms of the GNU General Public License version 2 only, as
7+
* published by the Free Software Foundation.
8+
*
9+
* This code is distributed in the hope that it will be useful, but WITHOUT
10+
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
11+
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
12+
* version 2 for more details (a copy is included in the LICENSE file that
13+
* accompanied this code).
14+
*
15+
* You should have received a copy of the GNU General Public License version
16+
* 2 along with this work; if not, write to the Free Software Foundation,
17+
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
18+
*
19+
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
20+
* or visit www.oracle.com if you need additional information or have any
21+
* questions.
22+
*/
23+
24+
/*
25+
* @test
26+
* @bug 8306929
27+
* @summary Verify clean_previous_versions when run with JFR and CDS
28+
* @requires vm.jvmti
29+
* @requires vm.cds
30+
* @requires vm.hasJFR
31+
* @requires vm.opt.final.ClassUnloading
32+
* @requires vm.flagless
33+
* @library /test/lib
34+
* @run driver RedefineSharedClassJFR xshare-off
35+
* @run driver RedefineSharedClassJFR xshare-on
36+
*/
37+
import java.util.ArrayList;
38+
import java.util.List;
39+
40+
import jdk.test.lib.Platform;
41+
import jdk.test.lib.process.ProcessTools;
42+
import jdk.test.lib.process.OutputAnalyzer;
43+
44+
import jtreg.SkippedException;
45+
46+
public class RedefineSharedClassJFR {
47+
48+
private static final String SHOULD_CLEAN_TRUE = "Class unloading: should_clean_previous_versions = true";
49+
private static final String SHOULD_CLEAN_FALSE = "Class unloading: should_clean_previous_versions = false";
50+
private static final String SCRATCH_CLASS_ADDED_SHARED = "scratch class added; class is shared";
51+
private static final String SCRATCH_CLASS_ADDED_ON_STACK = "scratch class added; one of its methods is on_stack.";
52+
53+
public static void main(String[] args) throws Exception {
54+
// Skip test if default archive is supported.
55+
if (!Platform.isDefaultCDSArchiveSupported()) {
56+
throw new SkippedException("Supported platform");
57+
}
58+
59+
// Test is run with JFR which will transform a number of classes. Depending
60+
// on if the test is run with or without CDS the output will be different,
61+
// due to the fact that shared classes can never be cleaned out after retranform.
62+
if (args.length > 0) {
63+
// When run with an argument the class is used as driver and should parse
64+
// the output to verify it is correct given the command line.
65+
List<String> baseCommand = List.of(
66+
"-XX:StartFlightRecording",
67+
"-Xlog:redefine+class+iklass+add=trace,redefine+class+iklass+purge=trace",
68+
"RedefineSharedClassJFR");
69+
70+
if (args[0].equals("xshare-off")) {
71+
// First case is with -Xshare:off. In this case no classes are shared
72+
// and we should be able to clean out the retransformed classes. Verify
73+
// that the cleaning is done when the GC is triggered.
74+
List<String> offCommand = new ArrayList<>();
75+
offCommand.add("-Xshare:off");
76+
offCommand.addAll(baseCommand);
77+
ProcessBuilder pb = ProcessTools.createJavaProcessBuilder(offCommand);
78+
new OutputAnalyzer(pb.start())
79+
.shouldContain(SHOULD_CLEAN_TRUE)
80+
.shouldNotContain(SHOULD_CLEAN_FALSE)
81+
// We expect at least one of the transformed classes to be in use, if
82+
// not the above check that should_clean_previous should be true will also
83+
// fail. This check is to show what is expected.
84+
.shouldContain(SCRATCH_CLASS_ADDED_ON_STACK)
85+
.shouldNotContain(SCRATCH_CLASS_ADDED_SHARED)
86+
.shouldHaveExitValue(0);
87+
return;
88+
} else if (args[0].equals("xshare-on")) {
89+
// With -Xshare:on, the shared classes can never be cleaned out. Check the
90+
// logs to verify we don't try to clean when we know it is not needed.
91+
List<String> onCommand = new ArrayList<>();
92+
onCommand.add("-Xshare:on");
93+
onCommand.addAll(baseCommand);
94+
ProcessBuilder pb = ProcessTools.createJavaProcessBuilder(onCommand);
95+
new OutputAnalyzer(pb.start())
96+
.shouldContain(SHOULD_CLEAN_FALSE)
97+
.shouldNotContain(SHOULD_CLEAN_TRUE)
98+
.shouldContain(SCRATCH_CLASS_ADDED_SHARED)
99+
// If the below line occurs, then should_clean_previous_versions will be
100+
// true and the above shouldContain will trigger. This check is to
101+
// show the intention that we don't expect any non-shared transformed
102+
// classes to be in use.
103+
.shouldNotContain(SCRATCH_CLASS_ADDED_ON_STACK)
104+
.shouldHaveExitValue(0);
105+
return;
106+
}
107+
}
108+
109+
// When run without any argument this class acts as test and we do a system GC
110+
// to trigger cleaning and get the output we want to check.
111+
System.gc();
112+
}
113+
}

0 commit comments

Comments
 (0)