8308762: Metaspace leak with Instrumentation.retransform #14780

jpbempel · 2023-07-06T05:18:01Z

Fix a small leak in constant pool merging during retransformation of a class. If this class has a catch block with Throwable, the class Throwable is pre-resolved in the constant pool, while all the other classes are in a unresolved state. So the constant pool merging process was considering the entry with pre-resolved class as different compared to the destination and create a new entry. We now try to consider it as equal specially for Methodref/Fieldref.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8308762: Metaspace leak with Instrumentation.retransform (Bug - P4)

Reviewers

David Holmes (@dholmes-ora - Reviewer)
Coleen Phillimore (@coleenp - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14780/head:pull/14780
$ git checkout pull/14780

Update a local copy of the PR:
$ git checkout pull/14780
$ git pull https://git.openjdk.org/jdk.git pull/14780/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14780

View PR using the GUI difftool:
$ git pr show -t 14780

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14780.diff

Webrev

Link to Webrev Comment

Fix a small leak in constant pool merging during retransformation of a class. If this class has a catch block with Throwable, the class Throwable is pre-resolved in the constant pool, while all the other classes are in a unresolved state. So the constant pool merging process was considering the entry with pre-resolved class as different compared to the destination and create a new entry. We now try to consider it as equal specially for Methodref/Fieldref.

bridgekeeper · 2023-07-06T05:18:47Z

👋 Welcome back jpbempel! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-07-06T05:20:49Z

@jpbempel The following labels will be automatically applied to this pull request:

hotspot
serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-07-06T05:25:14Z

Webrevs

dholmes-ora

Sorry I'm having a lot of trouble trying to understand the fix in the context of the problem description as outlined in the bug report. The issue pertained only to the treatment of Throwable due to it being pre-resolved by the verifier, but your fix is looking at Field and MethodRefs ??

There are also remaining comments about resolved and unresolved class entries deliberately not being considered the same.

dholmes-ora · 2023-07-06T06:23:40Z

src/hotspot/share/oops/constantPool.cpp

+    return false;  // not a mismatch; not our special case
+  }
+
+  char *s1 = klass_name_at(index1)->as_C_string();


as_C_string needs an active ResourceMark - not sure where it is coming from even in the existing code. There should at least be a comment that an active ResourceMark is needed.

dholmes-ora · 2023-07-06T06:28:35Z

src/hotspot/share/oops/constantPool.cpp

@@ -1276,6 +1276,33 @@ void ConstantPool::unreference_symbols() {
  }
 }

+// Returns true if the current mismatch is due to a resolved/unresolved
+// class pair. Otherwise, returns false.
+bool ConstantPool::is_unresolved_class_mismatch(int index1,


Has this been moved verbatim from jvmtiRedefineClasses.cpp?

There are a couple of style nits with that existing code that don't fit this file:

parameters should line up ie. const under int

no comment on the closing brace of the method body.

Has this been moved verbatim from jvmtiRedefineClasses.cpp?

yes

There are a couple of style nits with that existing code that don't fit this file:

parameters should line up ie. const under int

no comment on the closing brace of the method body.

Will adjust accordingly

jpbempel · 2023-07-06T11:02:48Z

Sorry I'm having a lot of trouble trying to understand the fix in the context of the problem description as outlined in the bug report. The issue pertained only to the treatment of Throwable due to it being pre-resolved by the verifier, but your fix is looking at Field and MethodRefs ??

For the Klass in itself, there is this method is_unresolved_class_mismatch that compares it correctly if entry differs by resolved and unresolved state. Except that for MethodRef and FieldRef, the Klass here is also compared but strictly without taking into account the already resolved state (in this case of the Throwable). So that's why I am adding the call to is_unresolved_class_mismatch for those cases.

There are also remaining comments about resolved and unresolved class entries deliberately not being considered the same.

Sorry I don't understand what you mean, can you elaborate please?

coleenp

I wonder if compare_entry_to can just handle this case directly and not have this function.

coleenp · 2023-07-06T13:26:00Z

src/hotspot/share/oops/constantPool.cpp

+  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
+  }


klass_name_at() returns a Symbol* that's in the SymbolTable so this never needed to do a strcmp. It can test for equality.

dholmes-ora · 2023-07-07T00:34:59Z

src/hotspot/share/oops/constantPool.cpp

    bool match = compare_entry_to(recur1, cp2, recur2);
+    match |= is_unresolved_class_mismatch(recur1, cp2, recur2);


This is changing the definition of a "match" such that in all cases an unresolved class entry is considered equal to a resolved class entry - but only for these Field and MethodRefs. I don't see how this connects back to the original problem. Nor do I see why this is alway a correct thing to do.

I agree that it may be not the right place to call is_unresolved_class_mismatch method because we capture all cases calling compare_entry_to. For a JVM_CONSTANT_Class, compare_entry_to is recovered by the call to is_unresolved_class_mismatch in VM_RedefineClasses::merge_constant_pools. The idea is to do the same for other reference to the class in FieldRefs and (Interface)MethodRefs because otherwise the entry does not match with pre-resolved Throwable class.

dholmes-ora · 2023-07-07T00:46:09Z

Sorry I don't understand what you mean, can you elaborate please?

In ConstantPool::compare_entry_to it states:

  if (t1 != t2) {
    // Not the same entry type so there is nothing else to check. Note
    // that this style of checking will consider resolved/unresolved
    // class pairs as different.
    // From the ConstantPool* API point of view, this is correct
    // behavior. See VM_RedefineClasses::merge_constant_pools() to see how this
    // plays out in the context of ConstantPool* merging.
    return false;
  }

and in VM_RedefineClasses::merge_constant_pools() it says

 // Pass 0:
    // The old_cp is copied to *merge_cp_p; this means that any code
    // using old_cp does not have to change. This work looks like a
    // perfect fit for ConstantPool*::copy_cp_to(), but we need to
    // handle one special case:
    // - revert JVM_CONSTANT_Class to JVM_CONSTANT_UnresolvedClass
// This will make verification happy.

these comments both pertain to the problem at hand: the merge reverts the resolved class entry to be unresolved; unresolved entries are not considered matches for resolved ones, hence we grow the constant pool.

Your fix is addressing the second part of the issue by forcing a match, but as per my other comment, it is not clear to me how the placement of that fix addresses the issue originally reported with the catch block, nor is it obvious to me that we should always consider resolved and unresolved class entries to be equal.

coleenp · 2023-07-11T01:03:29Z

ConstantPool merging is still really complicated and JVM_CONSTANT_Class/UnresolvedClass (and UnresolvedClassInError) may have changed to not track with what RedefineClasses does. If I'm reading this correctly, we are always calling find_matching_entry() and compare_entries_to() between the merged_cp and scratch_cp. Since we unresolve classes in merge_cp, and scratch_cp hasn't resolved anything yet, how are we getting this mismatch? Can you post a stack trace for us?

jpbempel · 2023-07-12T14:01:12Z

these comments both pertain to the problem at hand: the merge reverts the resolved class entry to be unresolved; unresolved entries are not considered matches for resolved ones, hence we grow the constant pool.

And If if I am reading this correctly from VM_RedefineClasses::merge_constant_pools:

      bool match = scratch_cp->compare_entry_to(scratch_i, *merge_cp_p, scratch_i);
      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;
      }

In a case of JVM_CONSTANT_Class/JVM_CONSTANT_UnresolvedClass this is handled specially with is_unresolved_class_mismatch.

But the same problem rises again when dealing with JVM_CONSTANT_Methodref and al. because the unresolved state contains also unresolved class for the merged_cp while for scratch_cp it is a resolved one.
The idea behind my fix proposal is to do the same as for JVM_CONSTANT_Class.
But I can understand your concern that introducing this in a method that could be used outside of the code of class redefintion is worrisome.
I can try to find another way to do the same but specifically in the context of class redefinition.

jpbempel · 2023-07-12T14:13:59Z

ConstantPool merging is still really complicated and JVM_CONSTANT_Class/UnresolvedClass (and UnresolvedClassInError) may have changed to not track with what RedefineClasses does. If I'm reading this correctly, we are always calling find_matching_entry() and compare_entries_to() between the merged_cp and scratch_cp. Since we unresolve classes in merge_cp, and scratch_cp hasn't resolved anything yet, how are we getting this mismatch? Can you post a stack trace for us?

I have try to explain in the comment to David above. Tell me if there is something that is still not clear.

coleenp · 2023-07-12T18:57:14Z

Hi, I now see why there's a resolved class in the scratch constant pool (ie the constant pool for the new class bytes). It looks like javac has preresolved it for us, which you said but I didn't know why. The merged constant pool always has an unresolved class copy of that class entry, because we create the merged constant pool with all unresolved classes.

I now believe that ConstantPool::compare_entry_to should always compare class and resolved class as equivalent. Since the klass_name_at() function is the same call for both unresolved and resolved class, you could change the tag at the beginning of compare_entry_to() to JVM_CONSTANT_UnresolvedClass if it's resolved, like we do for error classes. Then remove the is_unresolved_class_mismatch function entirely (the comment at that call might be useful to explain why you're changing the tag in compare_entry_to). This code is only used for RedefineClasses, but even if it wasn't this comparison is the right thing to do. For special measure, you could #if INCLUDE_JVMTI around this and the operand functions that call it or file an RFE to do so for further cleanup (compare_entry_to, find_matching_entry, find_matching_operand, compare_operand_to).

dholmes-ora · 2023-07-12T21:34:35Z

It looks like javac has preresolved it for us, which you said but I didn't know why.

The preresolving is done by the verifier. https://bugs.openjdk.org/browse/JDK-8308762?focusedCommentId=14584707&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14584707

coleenp · 2023-07-13T13:54:21Z

I see. I was looking at jcod output but that doesn't distinguish resolved vs. unresolved class. At any rate, the verifier will re-resolve the class when it verifies the merged constant pool. It's still safe to consider these types as same.

coleenp · 2023-07-13T14:34:38Z

Also there is a nice test harness for class redefinition in the test/hotspot/jtreg/serviceability/jvmti/RedefineClasses tests that you might be able to use to add a test for this.

remove is_unresolved_class_mismatch

openjdk · 2023-07-18T16:42:36Z

@jpbempel Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

jpbempel · 2023-07-18T16:49:32Z

Thanks @coleenp for the hints about fixing this issue.
Please review again the new changes that include:

jtreg test that reproduces the issue
revert the unresolved class state if the current entry is resolved
remove is_unresolved_class_mismatch method as useless now.

Thanks
(sorry to the force push, old habit)

sspitsyn · 2023-07-19T09:59:04Z

src/hotspot/share/oops/constantPool.cpp

+    // to ensure proper comparison.
+    t1 = JVM_CONSTANT_UnresolvedClass;
+  }
+


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?

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.

coleenp

Sorry I didn't see this update. The change looks good to me. You could do the cleanup that Serguei suggests with t2 and removing JVM_CONSTANT_Class case. I thought maybe they should be left in case we want to generalize this compare_entry_to function someday, that's why I didn't suggest removing it.

coleenp · 2023-07-27T19:48:02Z

src/hotspot/share/oops/constantPool.cpp

+    // to ensure proper comparison.
+    t1 = JVM_CONSTANT_UnresolvedClass;
+  }
+


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.

openjdk · 2023-07-27T19:51:35Z

@jpbempel This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8308762: Metaspace leak with Instrumentation.retransform

Reviewed-by: dholmes, coleenp

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 8 new commits pushed to the master branch:

8412479: 8316229: Enhance class initialization logging
c04c9ea: 8316627: JViewport Test headless failure
5cacf21: 8316156: ByteArrayInputStream.transferTo causes MaxDirectMemorySize overflow
3461c7b: 8316532: Native library copying in BuildMicrobenchmark.gmk cause dups on macOS
3301fb1: 8315869: UseHeavyMonitors not used
54028e7: 8316562: serviceability/sa/jmap-hprof/JMapHProfLargeHeapTest.java times out after JDK-8314829
7c991cc: 8296246: Update Unicode Data Files to Version 15.1.0
a021dbc: 8316149: Open source several Swing JTree JViewport KeyboardManager tests

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@dholmes-ora, @coleenp) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

coleenp

I have a suggestion for your test and copyright fix.

coleenp · 2023-07-28T12:17:39Z

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

@@ -0,0 +1,127 @@
+/*
+ * Copyright (c) 2016, 2023, Oracle and/or its affiliates. All rights reserved.


Please remove the 2016 copyright on the new test.

coleenp · 2023-07-28T12:20:47Z

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

+                t.printStackTrace();
+            }
+        }
+    }


If you look in that directory, there's an even simpler way to redefine classes without the Transformer and agent code. It looks like this: RedefineClassHelper.redefineClass(RedefineRunningMethods_B.class, evenNewerB);

coleenp · 2023-07-28T12:47:21Z

src/hotspot/share/prims/jvmtiRedefineClasses.cpp

-        // 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;


I'm sorry for the piecemeal review. There's another comment that mentions this comment that should be removed. It starts with this:

// The find_matching_entry() call above could fail to find a match

unresolved t2 too, cleanup JVM_CONSTANT_Class useless case

jpbempel · 2023-08-03T09:40:36Z

@coleenp I have made the changes: cleanup code and rewrite the unit test

jpbempel · 2023-08-23T21:24:04Z

Hey @coleenp could you review my last changes?

jpbempel · 2023-09-19T12:46:52Z

Please @coleenp review my last changes

dholmes-ora

Based on the previous discussion and comments this simple change seems quite reasonable to me.

One query on the test though.

Thanks

dholmes-ora · 2023-09-20T00:05:01Z

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

+ * @test
+ # @bug 8308762
+ * @library /test/lib
+ * @summary Test that redefinition of class containing Throwable refs does not leak constant pool


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?

Is it simply setting metaspace size small enough that the 500 iterations would exhaust metaspace if there were a leak?

yes exactly. with a leak you end up with OutOfMemoryError: metaspace running the test

coleenp

I'm sorry for the delay in reviewing this. This looks great!

dholmes-ora

Looks good. Thanks.

jpbempel · 2023-09-21T05:05:39Z

Thanks!
/integrate

openjdk · 2023-09-21T05:06:23Z

@jpbempel
Your change (at version 16bc7fa) is now ready to be sponsored by a Committer.

dholmes-ora · 2023-09-21T05:14:02Z

/sponsor

openjdk · 2023-09-21T05:16:11Z

Going to push as commit df4a25b.
Since your change was applied there have been 8 commits pushed to the master branch:

8412479: 8316229: Enhance class initialization logging
c04c9ea: 8316627: JViewport Test headless failure
5cacf21: 8316156: ByteArrayInputStream.transferTo causes MaxDirectMemorySize overflow
3461c7b: 8316532: Native library copying in BuildMicrobenchmark.gmk cause dups on macOS
3301fb1: 8315869: UseHeavyMonitors not used
54028e7: 8316562: serviceability/sa/jmap-hprof/JMapHProfLargeHeapTest.java times out after JDK-8314829
7c991cc: 8296246: Update Unicode Data Files to Version 15.1.0
a021dbc: 8316149: Open source several Swing JTree JViewport KeyboardManager tests

Your commit was automatically rebased without conflicts.

openjdk · 2023-09-21T05:16:24Z

@dholmes-ora @jpbempel Pushed as commit df4a25b.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

openjdk bot added the rfr Pull request is ready for review label Jul 6, 2023

openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Jul 6, 2023

dholmes-ora reviewed Jul 6, 2023

View reviewed changes

coleenp suggested changes Jul 6, 2023

View reviewed changes

dholmes-ora reviewed Jul 7, 2023

View reviewed changes

add jtreg test for leak

2ff1721

openjdk bot removed the rfr Pull request is ready for review label Jul 18, 2023

Revert resolved class to unresolved for comparison

c1a2d7c

remove is_unresolved_class_mismatch

jpbempel force-pushed the 8308762 branch from 05071a5 to c1a2d7c Compare July 18, 2023 16:41

openjdk bot added the rfr Pull request is ready for review label Jul 18, 2023

sspitsyn reviewed Jul 19, 2023

View reviewed changes

coleenp approved these changes Jul 27, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 27, 2023

coleenp suggested changes Jul 28, 2023

View reviewed changes

openjdk bot removed the ready Pull request is ready to be integrated label Jul 28, 2023

coleenp reviewed Jul 28, 2023

View reviewed changes

Rewrite unit test

57842b8

unresolved t2 too, cleanup JVM_CONSTANT_Class useless case

openjdk bot removed the rfr Pull request is ready for review label Aug 3, 2023

jpbempel added 2 commits August 3, 2023 09:57

remove now useless comment

3784e65

remove trailing whitespace

26cb41b

openjdk bot added the rfr Pull request is ready for review label Aug 3, 2023

dholmes-ora reviewed Sep 20, 2023

View reviewed changes

coleenp approved these changes Sep 20, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 20, 2023

Merge branch 'openjdk:master' into 8308762

16bc7fa

dholmes-ora approved these changes Sep 21, 2023

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Sep 21, 2023

openjdk bot added the integrated Pull request has been integrated label Sep 21, 2023

openjdk bot closed this Sep 21, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Sep 21, 2023

		bool match = compare_entry_to(recur1, cp2, recur2);
		match \|= is_unresolved_class_mismatch(recur1, cp2, recur2);

		@@ -0,0 +1,127 @@
		/*
		* Copyright (c) 2016, 2023, Oracle and/or its affiliates. All rights reserved.

8308762: Metaspace leak with Instrumentation.retransform #14780

8308762: Metaspace leak with Instrumentation.retransform #14780

Conversation

jpbempel commented Jul 6, 2023 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jul 6, 2023

openjdk bot commented Jul 6, 2023

mlbridge bot commented Jul 6, 2023 • edited

Webrevs

dholmes-ora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpbempel commented Jul 6, 2023

coleenp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dholmes-ora commented Jul 7, 2023

coleenp commented Jul 11, 2023

jpbempel commented Jul 12, 2023 • edited

jpbempel commented Jul 12, 2023

coleenp commented Jul 12, 2023

dholmes-ora commented Jul 12, 2023

coleenp commented Jul 13, 2023

coleenp commented Jul 13, 2023

openjdk bot commented Jul 18, 2023

jpbempel commented Jul 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coleenp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Jul 27, 2023 • edited

coleenp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpbempel commented Aug 3, 2023

jpbempel commented Aug 23, 2023

jpbempel commented Sep 19, 2023

dholmes-ora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coleenp left a comment

Choose a reason for hiding this comment

dholmes-ora left a comment

Choose a reason for hiding this comment

jpbempel commented Sep 21, 2023

openjdk bot commented Sep 21, 2023

dholmes-ora commented Sep 21, 2023

openjdk bot commented Sep 21, 2023

openjdk bot commented Sep 21, 2023

jpbempel commented Jul 6, 2023 •

edited by openjdk bot

mlbridge bot commented Jul 6, 2023 •

edited

jpbempel commented Jul 12, 2023 •

edited

openjdk bot commented Jul 27, 2023 •

edited