New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8308762: Metaspace leak with Instrumentation.retransform #14780
Changes from all commits
bf523df
2ff1721
c1a2d7c
57842b8
3784e65
26cb41b
16bc7fa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1287,35 +1287,6 @@ int VM_RedefineClasses::find_new_operand_index(int old_index) { | |
} // end find_new_operand_index() | ||
|
||
|
||
// Returns true if the current mismatch is due to a resolved/unresolved | ||
// class pair. Otherwise, returns false. | ||
bool VM_RedefineClasses::is_unresolved_class_mismatch(const constantPoolHandle& cp1, | ||
int index1, const constantPoolHandle& cp2, int index2) { | ||
|
||
jbyte t1 = cp1->tag_at(index1).value(); | ||
if (t1 != JVM_CONSTANT_Class && t1 != JVM_CONSTANT_UnresolvedClass) { | ||
return false; // wrong entry type; not our special case | ||
} | ||
|
||
jbyte t2 = cp2->tag_at(index2).value(); | ||
if (t2 != JVM_CONSTANT_Class && t2 != JVM_CONSTANT_UnresolvedClass) { | ||
return false; // wrong entry type; not our special case | ||
} | ||
|
||
if (t1 == t2) { | ||
return false; // not a mismatch; not our special case | ||
} | ||
|
||
char *s1 = cp1->klass_name_at(index1)->as_C_string(); | ||
char *s2 = cp2->klass_name_at(index2)->as_C_string(); | ||
if (strcmp(s1, s2) != 0) { | ||
return false; // strings don't match; not our special case | ||
} | ||
|
||
return true; // made it through the gauntlet; this is our special case | ||
} // end is_unresolved_class_mismatch() | ||
|
||
|
||
// The bug 6214132 caused the verification to fail. | ||
// 1. What's done in RedefineClasses() before verification: | ||
// a) A reference to the class being redefined (_the_class) and a | ||
|
@@ -1705,14 +1676,6 @@ bool VM_RedefineClasses::merge_constant_pools(const constantPoolHandle& old_cp, | |
if (match) { | ||
// found a match at the same index so nothing more to do | ||
continue; | ||
} else if (is_unresolved_class_mismatch(scratch_cp, scratch_i, | ||
*merge_cp_p, scratch_i)) { | ||
// The mismatch in compare_entry_to() above is because of a | ||
// resolved versus unresolved class entry at the same index | ||
// with the same string value. Since Pass 0 reverted any | ||
// class entries to unresolved class entries in *merge_cp_p, | ||
// we go with the unresolved class entry. | ||
continue; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm sorry for the piecemeal review. There's another comment that mentions this comment that should be removed. It starts with this:
|
||
} | ||
|
||
int found_i = scratch_cp->find_matching_entry(scratch_i, *merge_cp_p); | ||
|
@@ -1726,13 +1689,6 @@ bool VM_RedefineClasses::merge_constant_pools(const constantPoolHandle& old_cp, | |
continue; | ||
} | ||
|
||
// The find_matching_entry() call above could fail to find a match | ||
// due to a resolved versus unresolved class or string entry situation | ||
// like we solved above with the is_unresolved_*_mismatch() calls. | ||
// However, we would have to call is_unresolved_*_mismatch() over | ||
// all of *merge_cp_p (potentially) and that doesn't seem to be | ||
// worth the time. | ||
|
||
// No match found so we have to append this entry and any unique | ||
// referenced entries to *merge_cp_p. | ||
append_entry(scratch_cp, scratch_i, merge_cp_p, merge_cp_length_p); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
/* | ||
* 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 8308762 | ||
* @library /test/lib | ||
* @summary Test that redefinition of class containing Throwable refs does not leak constant pool | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exactly how is this test tracking whether there is a leak or not? Is it simply setting metaspace size small enough that the 500 iterations would exhaust metaspace if there were a leak? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
yes exactly. with a leak you end up with |
||
* @requires vm.jvmti | ||
* @requires vm.flagless | ||
* @modules java.base/jdk.internal.misc | ||
* @modules java.instrument | ||
* java.compiler | ||
* @run main RedefineClassHelper | ||
* @run main/othervm/timeout=6000 -javaagent:redefineagent.jar -XX:MetaspaceSize=12m -XX:MaxMetaspaceSize=12m RedefineLeakThrowable | ||
*/ | ||
|
||
class Tester { | ||
void test() { | ||
try { | ||
int i = 42; | ||
} catch (Throwable t) { | ||
t.printStackTrace(); | ||
} | ||
} | ||
} | ||
|
||
public class RedefineLeakThrowable { | ||
|
||
static final String NEW_TESTER = | ||
"class Tester {" + | ||
" void test() {" + | ||
" try {" + | ||
" int i = 42;" + | ||
" } catch (Throwable t) {" + | ||
" t.printStackTrace();" + | ||
" }" + | ||
" }" + | ||
"}"; | ||
|
||
|
||
public static void main(String argv[]) throws Exception { | ||
for (int i = 0; i < 500; i++) { | ||
RedefineClassHelper.redefineClass(Tester.class, NEW_TESTER); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All consequences of this change are not clear to me yet.
The lines 1307-1314 become not needed anymore.
Also, should the same be done for t2 as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
t2 could be a resolved class, and can be compared with unresolved class because the function klass_name_at() works for both. It might be a good idea to change both of them though, although not necessary imo.
You're right 1307-1314 are not reached anymore. Neither is the ClassIndex case but not to remove as part of this change.