8268406: Deallocate jmethodID native memory

[coleenp]

This change uses a ConcurrentHashTable to associate Method* with jmethodID, instead of an indirection. JNI is deprecated in favor of using Panama to call methods, so I don't think we're concerned about JNI performance going forward. JVMTI uses a lot of jmethodIDs but there aren't any performance tests for JVMTI, but running vmTestbase/nsk/jvmti with in product build with and without this change had no difference in time.

The purpose of this change is to remove the memory leak when you unload classes: we were leaving the jmethodID memory just in case JVMTI code still had references to that jmethodID and instead of crashing, should get nullptr. With this change, if JVMTI looks up a jmethodID, we've removed it from the table and will return nullptr. Redefinition and the InstanceKlass::_jmethod_method_ids is somewhat complicated. When a method becomes "obsolete" in redefinition, which means that the code in the method is changed, afterward creating a jmethodID from an "obsolete" method will create a new entry in the InstanceKlass table. This mechanism increases the method_idnum to do this. In the future maybe we could throw NoSuchMethodError if you try to create a jmethodID out of an obsolete method and remove all this code. But that's not in this change.

Tested with tier1-4, 5-7.

---

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-8268406: Deallocate jmethodID native memory (Enhancement - P4)

Reviewers

• Daniel D. Daugherty (@dcubed-ojdk - Reviewer) Review applies to cc0d6dda
• David Holmes (@dholmes-ora - Reviewer)
• Serguei Spitsyn (@sspitsyn - Reviewer)
• Erik Österlund (@fisk - Reviewer) Review applies to 66eb4269
• Axel Boldt-Christmas (@xmas92 - Reviewer) Review applies to 66eb4269

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25267

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 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]

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

8268406: Deallocate jmethodID native memory

Reviewed-by: dholmes, sspitsyn, dcubed, eosterlund, aboldtch

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

• 8ea544c: 8360287: JFR: PlatformTracer class should be loaded lazily
• 334683e: 8358645: Access violation in ThreadsSMRSupport::print_info_on during thread dump
• 20e0055: 8344165: Trace exceptions with a complete call-stack
• ... and 200 more: https://git.openjdk.org/jdk/compare/0582bd290d5a8b6344ae7ada36492cc2f33df050...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.

[openjdk]

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

Webrevs

• 11: Full - Incremental (8ef3f190)
• 10: Full - Incremental (cc0d6dda)
• 09: Full - Incremental (66eb4269)
• 08: Full - Incremental (1c1924c1)
• 07: Full - Incremental (50c036f4)
• 06: Full - Incremental (8339a6b5)
• 05: Full - Incremental (fd9bf502)
• 04: Full - Incremental (333d344c)
• 03: Full - Incremental (41d49607)
• 02: Full - Incremental (067046bf)
• 01: Full - Incremental (c2f2f42c)
• 00: Full (57321e75)

[coleenp]

Also, this was @fisk 's idea.

[dcubed-ojdk]

JNI is deprecated in favor of using Panama to call methods, so I don't think we're concerned about performance going forward.

s/concerned about performance/concerned about JNI performance/

[dcubed-ojdk]

In the future maybe we could throw NoSuchMethodError if you try to create a jmethodID out of an obsolete method and remove all this code. But that's not in this change.

Hmmm... I'm rusty with redefinition, but I think there are legitimate scenarios where is "okay"
to call an obsolete method. I believe it has to do with an in-progress call to a method being
redefined and you might get the obsolete method or the new method depending on timing.

[coleenp]

There is a way to create a jmethodID from an obsolete method and I thought we had tests that did this. I'm not finding them right now. The old and obsolete methods can still be running after a redefinition if they were running during the redefinition. The JIT deoptimizes them but they will be run in the interpreter. New invocations of the method will choose the new version of the method always. Always unless there's a bug that we don't know about. We have fixed a few old method invocations in the past coming from various places in the JVM but we fixed the "last" one fairly recently.
But technically, one could create a jmethodID to an obsolete method although it's not easy to do. I was musing above but not committed to breaking compatibility if there is code that requires this.

[coleenp]

Dan, thank you for your first pass. I've tried to address the things you pointed out. I think I've used 'null' correctly in comments and strings though.

[dcubed-ojdk]

Thumbs up with the latest version!

Thanks for fixing all the nits.

[dholmes-ora]

This looks good. I'm still digesting it.

Thanks

[dholmes-ora]

Technically you don't need acquire semantics here as this value is not used to then access other data. But I see this is the only getter API available.

[coleenp]

Yes, this does need an acquire getter outside the lock. It's better to not have a non-acquire version to be possibly used by accident.

[dholmes-ora]

Acquire is only ever needed outside the lock. I don't like there only being acquire/release available generally speaking because it just confuses what memory operations are being synchronized.

[sspitsyn]

This is really great optimization and refactoring!
Looks pretty good to me but I'd like to make one more pass through the changes.

[xmas92]

Looks good. Thanks.

Feels like JmethodIdCreation_lock might not be needed now but it is also easier to reason about things when we serialize the modifications.

[fisk]

the cls parameter is never actually used. So while it is supposed to refer to the class you have the static method jMethodID for, there is no requirement that it actually does, and could even be null.

Not passing in the cls parameter, would be a clear user error though, right? And one that would have crashed before, because if you racingly execute bytecodes of a class that is being unloaded, things would blow up one way or another. To me it seems like the user should just pass in the class as intended and then all is good.

[coleenp]

Thank you for reviewing from the GC and class unloading perspective, Axel and Erik.

[coleenp]

Thank you Serguei and Dan for reviewing.

[dholmes-ora]

the cls parameter is never actually used. So while it is supposed to refer to the class you have the static method jMethodID for, there is no requirement that it actually does, and could even be null.

Not passing in the cls parameter, would be a clear user error though, right? And one that would have crashed before, because if you racingly execute bytecodes of a class that is being unloaded, things would blow up one way or another. To me it seems like the user should just pass in the class as intended and then all is good.

The point is to try and make things more robust when the user does the unexpected. As Coleen stated we are trying to handle cases where JNI code looks up a jMethodID in one place, stashes it away and then uses it elsewhere with no guarantee the class is being kept alive. I agree you would think they would have, and pass in, the original jclass reference, but the fact we don't actually use that value is not hard to determine and JNI code can take advantage of that - treating the jMethodId as an effective raw pointer to a method.

[dholmes-ora]

The jmethodID table is so that jmethodID isn't a stale pointer itself and doesn't require us to hold a stale pointer, but whether it can return a stale Method* (now and before this change) is something we should figure out how it should work.

That's fine. I (per my response to Thomas) thought the new approach also closed the door on unsafe usage of the jMethodID, but that is not the case. It probably does close the door quite a bit in relation to current approach though.

[dcubed-ojdk]

I've made another pass through this PR. Just a few more nits.

[dcubed-ojdk]

Another way to look at the safety of the new mechanism is to
consider the use of jmethodIDs from the AsyncGetCallTrace API:

src/hotspot/share/prims/forte.cpp:

108 2025.06.25 12:09:14 $ cat /tmp/fred
// Forte Analyzer AsyncGetCallTrace() entry point. Currently supported
// on Linux X86, Solaris SPARC and Solaris X86.
//
// Async-safe version of GetCallTrace being called from a signal handler
// when a LWP gets interrupted by SIGPROF but the stack traces are filled
// with different content (see below).
//
// This function must only be called when JVM/TI
// CLASS_LOAD events have been enabled since agent startup. The enabled
// event will cause the jmethodIDs to be allocated at class load time.
// The jmethodIDs cannot be allocated in a signal handler because locks
// cannot be grabbed in a signal handler safely.
//
// void (*AsyncGetCallTrace)(ASGCT_CallTrace *trace, jint depth, void* ucontext)
//
// Called by the profiler to obtain the current method call stack trace for
// a given thread. The thread is identified by the env_id field in the
// ASGCT_CallTrace structure. The profiler agent should allocate a ASGCT_CallTrace
// structure with enough memory for the requested stack depth. The VM fills in
// the frames buffer and the num_frames field.
//
// Arguments:
//
//   trace    - trace data structure to be filled by the VM.
//   depth    - depth of the call stack trace.
//   ucontext - ucontext_t of the LWP
//
// ASGCT_CallTrace:
//   typedef struct {
//       JNIEnv *env_id;
//       jint num_frames;
//       ASGCT_CallFrame *frames;
//   } ASGCT_CallTrace;
//
// Fields:
//   env_id     - ID of thread which executed this trace.
//   num_frames - number of frames in the trace.
//                (< 0 indicates the frame is not walkable).
//   frames     - the ASGCT_CallFrames that make up this trace. Callee followed by callers.
//
//  ASGCT_CallFrame:
//    typedef struct {
//        jint lineno;
//        jmethodID method_id;
//    } ASGCT_CallFrame;
//
//  Fields:
//    1) For Java frame (interpreted and compiled),
//       lineno    - bci of the method being executed or -1 if bci is not available
//       method_id - jmethodID of the method being executed
//    2) For native method
//       lineno    - (-3)
//       method_id - jmethodID of the method being executed

The AsyncGetCallTrace API returns jmethodIDs without explicitly
associated jclass references. The AsyncGetCallTrace API relies on
JVM/TI CLASS_LOAD events to have been enabled since agent startup for
jmethodID creation. Implicit in that requirement is the agent keeping a
jclass reference for each CLASS_LOAD event so that the class cannot be
unloaded while the data returned by AsyncGetCallTrace is processed.

[dcubed-ojdk]

Ping @jbachorik for discussions about the safety of jmethodIDs...

[dcubed-ojdk]

Thanks for making the wording tweaks.

[coleenp]

// This function must only be called when JVM/TI
// CLASS_LOAD events have been enabled since agent startup. The enabled
// event will cause the jmethodIDs to be allocated at class load time.
// The jmethodIDs cannot be allocated in a signal handler because locks
// cannot be grabbed in a signal handler safely.

I do not see any code that creates jmethodIDs for all the methods in a ClassLoad JVMTI event. ACGCT does call

trace->frames[count].method_id = method->find_jmethod_id_or_null();

so it won't create jmethodIDs from a signal handler. Keeping them live for the frames depends on when the native caller of ASGCT uses the trace that is returned. If the frames are still on the call stack, the methods cannot be unloaded because they're still on the call stack. If the frames returned by ASGCT are stored somewhere else, and accessed later, they could be null. I don't see any code that makes this work tbh. Most methods do not have jmethodIDs.

[dholmes-ora]

Still fine

[coleenp]

Thank you for the reviews, Serguei, David, Dan, Axel and Erik. And all the discussion, and the idea itself.
/integrate

[openjdk]

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

• aa26ced: 8360474: Add missing include guards for some HotSpot headers
• 20e983a: 8360487: Remove unnecessary List.indexOf key from AbstractMidiDevice.TransmitterList.remove
• 97ec9d3: 8360533: ContainerRuntimeVersionTestUtils fromVersionString fails with some docker versions
• ... and 205 more: https://git.openjdk.org/jdk/compare/0582bd290d5a8b6344ae7ada36492cc2f33df050...master

Your commit was automatically rebased without conflicts.

[openjdk]

@coleenp Pushed as commit d8f9b18.

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