RFR: 8268406: Deallocate jmethodID native memory [v7]

[David Holmes]

On Wed, 18 Jun 2025 12:46:56 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> 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.
>
> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Add a basic gtest.

Still working my way through this ...

Just to be sure I have the correct mental picture of this, things work as follow:

- a `jMethodID` is just a unique integer index to represent a `Method` for use with JNI
- when we allocate a new `jMethodID` to a `Method` we 
  - add that mapping to the global table
  - add an entry to the `ClassLoaderData`'s `jMethodID` "list"
- each `instanceklass` also maintains a "cache" of its own `jMethodID` mappings (some of which need updating on method redefinition)
- when a class is unloaded we deallocate the cache, deallocate the CLD list, and remove the table entries
- when a JNI API is presented with a `jMethodID` by the caller, it validates it by looking it up in the table, to see if the mapping exists

The cache is created under a lock, and `jMethodID`s are created and added to the cache (and the table, and the CLD list) under the same lock. But access to the cache is lock-free using acquire/release.

The cache is also destroyed under the same lock, and the table entries removed under that same lock, and the CLD list deallocated.

Is the above correct? What details have I missed?

Thanks

src/hotspot/share/oops/instanceKlass.cpp line 2395:

> 2393: 
> 2394: // Allocate the jmethodID cache.
> 2395: static jmethodID* create_jmethod_id_cache(size_t size) {

Why isn't this used at line 2439 to create the (initial?) cache?

src/hotspot/share/oops/jmethodIDTable.cpp line 40:

> 38: static uint64_t _jmethodID_counter = 0;
> 39: // Tracks the number of jmethodID entries in the _jmethod_id_table.
> 40: // Incremented on insert, decremented on remove. Use to track if we need to resize the table.

Suggestion:

// Incremented on insert, decremented on remove. Used to track if we need to resize the table.

src/hotspot/share/oops/jmethodIDTable.cpp line 141:

> 139:   // Update jmethodID global counter.
> 140:   _jmethodID_counter++;
> 141:   guarantee(_jmethodID_counter != 0, "must never go back to zero");

Again a guarantee is not needed here as the only possible way this could trigger is if we initialize it incorrectly./

-------------

PR Review: https://git.openjdk.org/jdk/pull/25267#pullrequestreview-2940758855
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2155747747
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2155550987
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2155763677