On Wed, 18 Jun 2025 12:46:56 GMT, Coleen Phillimore <cole...@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
