8291736: find_method_handle_intrinsic leaks Method* #9983

coleenp · 2022-08-23T15:29:58Z

As part of the SymbolPropertyTable conversion, we noticed that threads could race to add the Method entry to the table, and the loser wasn't deleted. This change locks the InvokeMethodTable_lock through the Method creation so that it's not leaked. See bug for details, but this was performance tested with our general suite of performance tests to show no significant differences.
Also tested with tier1-3, and previously 4-7 with SymbolPropertyTable conversion patch.

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-8291736: find_method_handle_intrinsic leaks Method*

Reviewers

Harold Seigel (@hseigel - Reviewer) ⚠️ Review applies to 73f02463
Ioi Lam (@iklam - Reviewer) ⚠️ Review applies to 73f02463
David Holmes (@dholmes-ora - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9983

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

Using diff file

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

bridgekeeper · 2022-08-23T15:31:55Z

👋 Welcome back coleenp! 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 · 2022-08-23T15:34:44Z

@coleenp The following label will be automatically applied to this pull request:

hotspot-runtime

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

mlbridge · 2022-08-23T15:38:48Z

Webrevs

hseigel

The change looks good.
Thanks, Harold

openjdk · 2022-08-24T12:45:13Z

@coleenp 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:

8291736: find_method_handle_intrinsic leaks Method*

Reviewed-by: hseigel, iklam, dholmes

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 129 new commits pushed to the master branch:

5757e21: 8292385: assert(ctrl == kit.control()) failed: Control flow was added although the intrinsic bailed out
3993a1f: 8292067: Convert test/sun/management/jmxremote/bootstrap shell tests to java version
83a3408: 8293315: Add back logging for Placeholders
b6477fd: 8293288: bootcycle build failure after JDK-8173605
0c6094e: 8293188: x86_64: Introduce stubGenerator_x86_64.hpp
2baeebb: 8293006: sun/tools/jhsdb/JStackStressTest.java fails with "UnalignedAddressException: 8baadbabe"
da99e3e: 8289400: Improve com/sun/jdi/TestScaffold error reporting
77e21c5: 8290529: C2: assert(BoolTest(btest).is_canonical()) failure
fcc0cf9: 8292375: Convert ProtectionDomainCacheTable to ResourceHashtable
6fc58b8: 8293207: Add assert to JVM_ReferenceRefersTo to clarify its API
... and 119 more: https://git.openjdk.org/jdk/compare/d24b7b7026cf85f1aecf44f60819762872cfd5c1...master

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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

coleenp · 2022-08-24T15:05:14Z

Thanks, Harold!

iklam

This looks fine. Now we are holding a lock across the call to make_method_handle_intrinsic, but that doesn't seem to do anything complicated (luckily it's not calling back into Java), so we should be OK.

src/hotspot/share/classfile/systemDictionary.cpp

dholmes-ora · 2022-08-25T00:08:10Z

src/hotspot/share/classfile/systemDictionary.cpp

  const int iid_as_int = vmIntrinsics::as_int(iid);
  assert(MethodHandles::is_signature_polymorphic(iid) &&
         MethodHandles::is_signature_polymorphic_intrinsic(iid) &&
         iid != vmIntrinsics::_invokeGeneric,
         "must be a known MH intrinsic iid=%d: %s", iid_as_int, vmIntrinsics::name_at(iid));

-  Method** met;
+  MutexLocker ml(THREAD, InvokeMethodTable_lock);


I don't think you can hold the Mutex across the entire method as the exception code below will go to Java.

This call does not go to Java. The other table does - the invoke_method_type_table. This just creates an intrinsic method and an adapter to go with it from the AdapterHandlerLibrary.

Line 2087 can throw an exception. If you want to hold the mutex across the entire body you need to throw the exception after releasing the lock.

Sorry, I didn't read what you actually wrote (just what I thought you wrote). Line 2080 can also throw an exception for metaspace allocation failure. I don't believe that we always do this outside a lock. This is a subtle interaction that only you noticed. I'm not sure what to do with the metaspace allocation failure. Moving throwing the other exception outside the lock uglifies the logic but isn't hard to do.

This is a subtle interaction that only you noticed.

You mean in this review? The original code clearly knew this had to be done outside the lock:

http://hg.openjdk.java.net/jdk7/jdk7/hotspot/rev/e5b0439ef4ae#l33.106

Now perhaps the current code is actually safe (as @iklam suggests) because it can't actually invoke any Java code? But that is something we would need to establish. In any case it seems an anti-pattern to allow any kind of use of CHECK from inside a locked region when we "know" we always have to throw exceptions outside of locked regions.

The problem with the original code was the ranking and multi-purpose use of the SystemDictionary_lock. Creating the adapters takes out a lock whose ranking is above SystemDictionary_lock. Also the tables (and lock for the tables) were shared with code that could call Java code for calling Java to create MethodType, so that code had to release the lock. Leaking the java.lang.invoke.MethodType is ok because it'll be GC'd.

We've leaked the Method* and adapter for many years now, maybe it's fine to keep leaking it and I should just close this and not try to fix it.

The case of throwing exceptions inside Mutex locked regions is something I thought we have all over the source code. I couldn't find any explicit calls, but the pattern of doing metaspace allocation inside of a MutexLocker is in enough places that adding an assert(!thread->owns_locks(), "must release all locks when possibly throwing exceptions"); during in the TRAPS version of Metaspace::allocate() fails immediately.

But it may be that the OOM for Metaspace allocation failure doesn't call into Java for constructing the object and this part is fine.

Right, as per our side-bar discussion, while the general rule is "no throwing exceptions while holding a VM mutex/monitor" it seems that it is safe to only throw OutOfMemoryError because Universe::gen_out_of_memory_error doesn't execute any Java code. It either uses one of the pre-allocated OOME instances (if available) and fills in the stacktrace, or else (or in case of secondary OOM) it uses the pre-allocated stackless singleton instance. This should be documented somewhere of course (and perhaps even programmatically verified somehow NoJavaCodeVerifier? :) )

Also whilst the lower-level code makes it somewhat clear that the only exception that can arise is the metaspace OOME, it is not at all clear just by looking at a callsite like:

methodHandle m = Method::make_method_handle_intrinsic(iid, signature, CHECK_NULL);

so perhaps we even need additional macros like CHECK_OOM to verify only OOME can be thrown? (Not in this PR.)

I added a comment above the make_method_handle_intrinsic call about why it's ok to call inside a locked region, and agree that we need better checking for the ability to run Java code from all paths with possible exceptions. I'm not sure how to do that yet but we should file an RFE.

dholmes-ora · 2022-08-25T00:11:45Z

Why not just use created to determine whether to delete the method to be thrown away?

coleenp · 2022-08-25T11:30:54Z

Why not just use created to determine whether to delete the method to be thrown away?

I had a version of this that crashed because we create an adapter and a native wrapper so removing the Method* through add_to_deallocate_list lead to a dangling pointer from the code cache.
This seems simpler.

To elaborate further, this code creates a native wrapper nmethod for the Method created in make_method_handle_intrinsic, which has the Metadata for Method* in the nmethod. Removing the leaked Method (and associated ConstantPool, as each of these creates their own constant pool), also requires some way of removing the nmethod as well. Given that this code is owned by the GC, adding non-gc support for doing this seems like a very bad idea.

dholmes-ora

No further comments from me.

coleenp · 2022-09-06T13:58:58Z

Thanks David.
/integrate

openjdk · 2022-09-06T14:01:24Z

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

b2067e6: 8291725: Leftover marks when VM shutdown aborts bitmap clearing make mixed gc fail
6a1e98c: 8293213: G1: Remove redundant assertion in G1RemSet::clean_card_before_refine
a92c1ff: 8287912: GTK L&F : Background of tree icons are red
272745b: 8293340: Remove unused _code in {Zero,Template}InterpreterGenerator
26f2a97: 8290561: Coalesce incubator-module warnings for single-file source-code programs
8e22f2b: 8293361: GHA: dump config.log in case of configure failure
b17758a: 8247283: Enable the new conformant preprocessor option in Visual Studio
1bed23a: 8293353: [BACKOUT] G1: Remove redundant is-marking-active checks in C1 barrier
4955835: 8282434: Leading combining diacritic character in string renders incorrectly
ef20ffe: 8293159: Use try-with-resources in X11FontManager.registerFontDir
... and 150 more: https://git.openjdk.org/jdk/compare/d24b7b7026cf85f1aecf44f60819762872cfd5c1...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-09-06T14:01:39Z

@coleenp Pushed as commit c05015b.

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

8291736: find_method_handle_intrinsic leaks Method*

73f0246

openjdk bot added the rfr Pull request is ready for review label Aug 23, 2022

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Aug 23, 2022

hseigel approved these changes Aug 24, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 24, 2022

iklam approved these changes Aug 24, 2022

View reviewed changes

dholmes-ora suggested changes Aug 25, 2022

View reviewed changes

coleenp added 3 commits August 25, 2022 11:31

Fix whitespace.

c12234d

Move one exception out of locked region.

f8e85da

Add comment

d0ead64

dholmes-ora approved these changes Sep 2, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Sep 6, 2022

openjdk bot closed this Sep 6, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 6, 2022

coleenp deleted the intrinsic-lock branch September 6, 2022 14:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8291736: find_method_handle_intrinsic leaks Method* #9983

8291736: find_method_handle_intrinsic leaks Method* #9983

coleenp commented Aug 23, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Aug 23, 2022

openjdk bot commented Aug 23, 2022

mlbridge bot commented Aug 23, 2022 •

edited

Loading

hseigel left a comment

openjdk bot commented Aug 24, 2022 •

edited

Loading

coleenp commented Aug 24, 2022

iklam left a comment

dholmes-ora Aug 25, 2022

coleenp Aug 25, 2022 •

edited

Loading

dholmes-ora Aug 25, 2022

coleenp Aug 26, 2022

dholmes-ora Aug 31, 2022

coleenp Aug 31, 2022 •

edited

Loading

dholmes-ora Sep 1, 2022

coleenp Sep 1, 2022

dholmes-ora commented Aug 25, 2022

coleenp commented Aug 25, 2022 •

edited

Loading

dholmes-ora left a comment

coleenp commented Sep 6, 2022

openjdk bot commented Sep 6, 2022

openjdk bot commented Sep 6, 2022

8291736: find_method_handle_intrinsic leaks Method* #9983

8291736: find_method_handle_intrinsic leaks Method* #9983

Conversation

coleenp commented Aug 23, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Aug 23, 2022

openjdk bot commented Aug 23, 2022

mlbridge bot commented Aug 23, 2022 • edited Loading

Webrevs

hseigel left a comment

Choose a reason for hiding this comment

openjdk bot commented Aug 24, 2022 • edited Loading

coleenp commented Aug 24, 2022

iklam left a comment

Choose a reason for hiding this comment

dholmes-ora Aug 25, 2022

Choose a reason for hiding this comment

coleenp Aug 25, 2022 • edited Loading

Choose a reason for hiding this comment

dholmes-ora Aug 25, 2022

Choose a reason for hiding this comment

coleenp Aug 26, 2022

Choose a reason for hiding this comment

dholmes-ora Aug 31, 2022

Choose a reason for hiding this comment

coleenp Aug 31, 2022 • edited Loading

Choose a reason for hiding this comment

dholmes-ora Sep 1, 2022

Choose a reason for hiding this comment

coleenp Sep 1, 2022

Choose a reason for hiding this comment

dholmes-ora commented Aug 25, 2022

coleenp commented Aug 25, 2022 • edited Loading

dholmes-ora left a comment

Choose a reason for hiding this comment

coleenp commented Sep 6, 2022

openjdk bot commented Sep 6, 2022

openjdk bot commented Sep 6, 2022

coleenp commented Aug 23, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Aug 23, 2022 •

edited

Loading

openjdk bot commented Aug 24, 2022 •

edited

Loading

coleenp Aug 25, 2022 •

edited

Loading

coleenp Aug 31, 2022 •

edited

Loading

coleenp commented Aug 25, 2022 •

edited

Loading