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