RFR: 8268406: Deallocate jmethodID native memory

Fri Jun 13 20:44:43 UTC 2025

On Fri, 16 May 2025 12:18:42 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.

Changes requested by dcubed (Reviewer).

src/hotspot/share/classfile/classLoaderData.cpp line 585:

> 583:   MutexLocker m1(metaspace_lock(), Mutex::_no_safepoint_check_flag);
> 584:   if (_jmethod_ids == nullptr) {
> 585:     _jmethod_ids = new (mtClass) GrowableArray<jmethodID>(32, mtClass);

Do you want the literal `32` to be a tunable value?

src/hotspot/share/classfile/classLoaderData.cpp line 590:

> 588: }
> 589: 
> 590: // Method::clear_jmethod_ids removes jmethodID entries from the table which

Perhaps the name changed during your development?
    s/clear_jmethod_ids/remove_jmethod_ids/

src/hotspot/share/classfile/classLoaderData.cpp line 592:

> 590: // Method::clear_jmethod_ids removes jmethodID entries from the table which
> 591: // releases memory.
> 592: // Because native code (e.g. JVMTI agent) holding jmethod_ids may access them

grammar: s/e.g./e.g.,/

src/hotspot/share/classfile/classLoaderData.cpp line 594:

> 592: // Because native code (e.g. JVMTI agent) holding jmethod_ids may access them
> 593: // after the associated classes and class loader are unloaded, subsequent lookups
> 594: // for these ids will return null since they are no longer found in the table.

Perhaps: s/null/nullptr/

src/hotspot/share/classfile/classLoaderData.hpp line 319:

> 317:   void add_jmethod_id(jmethodID id);
> 318:   void remove_jmethod_ids();
> 319:   GrowableArray<jmethodID>* jmethod_ids() { return _jmethod_ids; }

Should `jmethod_ids` still be `const`?

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

> 2392: }
> 2393: 
> 2394: // Lookup or create a jmethodID.

The comment on L2394 appears wrong for `create_jmethod_id_cache`.
Perhaps move it to L2404 (above get_jmethod_id() function).

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

> 2395: static jmethodID* create_jmethod_id_cache(size_t size) {
> 2396:   jmethodID* jmeths = NEW_C_HEAP_ARRAY(jmethodID, size+1, mtClass);
> 2397:   memset(jmeths, 0, (size+1)*sizeof(jmethodID));

nit spacing: s/size+1/size + 1/
on two lines.

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

> 2400:   return jmeths;
> 2401: }
> 2402: 

nit spacing: delete extra blank line?

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

> 2402: 
> 2403: 
> 2404: jmethodID InstanceKlass::get_jmethod_id(Method* method) {

Should `method` be `const`?

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

> 2458:     size_t size = idnum_allocated_count();
> 2459:     size_t old_size = (size_t)cache[0];
> 2460:     if (old_size < size+1) {

nit spacing: s/size+1/size + 1/

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

> 2459:     size_t old_size = (size_t)cache[0];
> 2460:     if (old_size < size+1) {
> 2461:       // allocate a larger one and copy entries to the new one.

nit typo: s/allocate/Allocate/

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

> 2460:     if (old_size < size+1) {
> 2461:       // allocate a larger one and copy entries to the new one.
> 2462:       // They've already been updated to point to new methods where applicable (ie. not obsolete)

nit typo: s/(ie. not obsolete)/(i.e., not obsolete)./

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

> 2493:         id == nullptr) {
> 2494:       id = Method::make_jmethod_id(class_loader_data(), m);
> 2495:       Atomic::release_store(&jmeths[idnum+1], id);

nit spacing: s/size+1/size + 1/

src/hotspot/share/oops/instanceKlass.hpp line 1057:

> 1055:   inline jmethodID* methods_jmethod_ids_acquire() const;
> 1056:   inline void release_set_methods_jmethod_ids(jmethodID* jmeths);
> 1057:   // This nulls out obsolete jmethodIDs for all methods in 'klass'