RFR: 8268406: Deallocate jmethodID native memory [v10]

[Daniel D. Daugherty]

On Fri, 20 Jun 2025 20:41:21 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:
> 
>   Fix the test

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

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

> 2479: 
> 2480: // Make a jmethodID for all methods in this class.
> 2481: // This makes getting all method ids much, much faster with classes with more than 8

Perhaps:

// Make a jmethodID for all methods in this class. This makes getting
// all method ids much, much faster with classes with more than 8


for a better looking flow.

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

> 4277: // This nulls out jmethodIDs for all obsolete methods in the previous version of the 'klass'.
> 4278: // These obsolete methods only exist in the previous version and we're about to delete the memory for them.
> 4279: // The jmethodID for these are deallocated when we unload the class, so this doesn't remove them from the table.

nit typo: s/jmethodID/jmethodIDs/

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

> 188:   // - multiple redefined versions may share jmethodID slots and if a method
> 189:   //   has already been rewired to a newer version we could be clearing reference
> 190:   //   to a still existing method instance.

Perhaps:

  //   has already been rewired to a newer version we could be clearing he
  //   reference to a still existing method instance.

src/hotspot/share/runtime/mutexLocker.cpp line 236:

> 234:   MUTEX_DEFN(Notification_lock               , PaddedMonitor, service);          // used for notification thread operations
> 235: 
> 236:   MUTEX_DEFN(JmethodIdCreation_lock          , PaddedMutex  , nosafepoint-1);    // used for creating jmethodIDs locks HandshakeState_lock

Perhaps:

  MUTEX_DEFN(JmethodIdCreation_lock          , PaddedMutex  , nosafepoint-1);    // used for creating jmethodIDs, can lock HandshakeState_lock which is at nosafepoint

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

Marked as reviewed by dcubed (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/25267#pullrequestreview-2958999974
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2167214224
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2167231725
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2167249362
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2167284891