On Mon, 16 Jun 2025 17:37:17 GMT, Coleen Phillimore <[email protected]> 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 formatting errors.
This looks good. I'm still digesting it.
Thanks
src/hotspot/share/oops/instanceKlass.cpp line 2480:
> 2478: void InstanceKlass::make_methods_jmethod_ids() {
> 2479: MutexLocker ml(JmethodIdCreation_lock,
> Mutex::_no_safepoint_check_flag);
> 2480: jmethodID* jmeths = methods_jmethod_ids_acquire();
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.
-------------
PR Review: https://git.openjdk.org/jdk/pull/25267#pullrequestreview-2933991535
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2151221096
