RFR: 8268406: Deallocate jmethodID native memory [v4]

[Serguei Spitsyn]

On Tue, 17 Jun 2025 13:59:54 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:
> 
>   Use load_acquire only in the places that need it.

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

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

> 2410: }
> 2411: 
> 2412: // Lookup or create a jmethodID

Nit: Add dot at the end.

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

> 2470:       // Allocate a larger one and copy entries to the new one.
> 2471:       // They've already been updated to point to new methods where applicable (i.e., not obsolete).
> 2472:       jmethodID* new_cache = create_jmethod_id_cache(size);

Nice micro-refactoring!

src/hotspot/share/oops/jmethodIDTable.hpp line 48:

> 46:   static void remove(jmethodID mid);
> 47: 
> 48:   // RedefineClasses support

Nit: Add a dot at the end of the comment for consistency with other comments.

src/hotspot/share/oops/method.hpp line 718:

> 716: 
> 717:   static void change_method_associated_with_jmethod_id(jmethodID old_jmid_ptr, Method* new_method);
> 718:   static bool validate_method_id(jmethodID mid);

Nit: I'd suggest to name it `validate_jmethod_id` for consistency.

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

PR Review: https://git.openjdk.org/jdk/pull/25267#pullrequestreview-2937302206
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2153333668
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2153332839
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2153323790
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2153315538