8268406: Deallocate jmethodID native memory

Reviewed-by: dholmes, sspitsyn, dcubed, eosterlund, aboldtch

Author: coleenp

src/hotspot/share/classfile/classLoaderData.cpp

@@ -65,6 +65,7 @@
 #include "memory/resourceArea.hpp"
 #include "memory/universe.hpp"
 #include "oops/access.inline.hpp"
+#include "oops/jmethodIDTable.hpp"
 #include "oops/klass.inline.hpp"
 #include "oops/oop.inline.hpp"
 #include "oops/oopHandle.inline.hpp"
@@ -578,6 +579,33 @@ void ClassLoaderData::remove_class(Klass* scratch_class) {
   ShouldNotReachHere();   // should have found this class!!
 }
 
+void ClassLoaderData::add_jmethod_id(jmethodID mid) {
+  MutexLocker m1(metaspace_lock(), Mutex::_no_safepoint_check_flag);
+  if (_jmethod_ids == nullptr) {
+    _jmethod_ids = new (mtClass) GrowableArray<jmethodID>(32, mtClass);
+  }
+  _jmethod_ids->push(mid);
+}
+
+// Method::remove_jmethod_ids removes jmethodID entries from the table which
+// releases memory.
+// Because native code (e.g., JVMTI agent) holding jmethod_ids may access them
+// after the associated classes and class loader are unloaded, subsequent lookups
+// for these ids will return null since they are no longer found in the table.
+// The Java Native Interface Specification says "method ID
+// does not prevent the VM from unloading the class from which the ID has
+// been derived. After the class is unloaded, the method or field ID becomes
+// invalid".
+void ClassLoaderData::remove_jmethod_ids() {
+  MutexLocker ml(JmethodIdCreation_lock, Mutex::_no_safepoint_check_flag);
+  for (int i = 0; i < _jmethod_ids->length(); i++) {
+    jmethodID mid = _jmethod_ids->at(i);
+    JmethodIDTable::remove(mid);
+  }
+  delete _jmethod_ids;
+  _jmethod_ids = nullptr;
+}
+
 void ClassLoaderData::unload() {
   _unloading = true;
 
@@ -599,19 +627,8 @@ void ClassLoaderData::unload() {
   // after erroneous classes are released.
   classes_do(InstanceKlass::unload_class);
 
-  // Method::clear_jmethod_ids only sets the jmethod_ids to null without
-  // releasing the memory for related JNIMethodBlocks and JNIMethodBlockNodes.
-  // This is done intentionally because native code (e.g. JVMTI agent) holding
-  // jmethod_ids may access them after the associated classes and class loader
-  // are unloaded. The Java Native Interface Specification says "method ID
-  // does not prevent the VM from unloading the class from which the ID has
-  // been derived. After the class is unloaded, the method or field ID becomes
-  // invalid". In real world usages, the native code may rely on jmethod_ids
-  // being null after class unloading. Hence, it is unsafe to free the memory
-  // from the VM side without knowing when native code is going to stop using
-  // them.
   if (_jmethod_ids != nullptr) {
-    Method::clear_jmethod_ids(this);
+    remove_jmethod_ids();
   }
 }
 
@@ -1037,9 +1054,7 @@ void ClassLoaderData::print_on(outputStream* out) const {
     out->print_cr(" - dictionary          " INTPTR_FORMAT, p2i(_dictionary));
   }
   if (_jmethod_ids != nullptr) {
-    out->print   (" - jmethod count       ");
-    Method::print_jmethod_ids_count(this, out);
-    out->print_cr("");
+    out->print_cr(" - jmethod count       %d", _jmethod_ids->length());
   }
   out->print_cr(" - deallocate list     " INTPTR_FORMAT, p2i(_deallocate_list));
   out->print_cr(" - next CLD            " INTPTR_FORMAT, p2i(_next));

src/hotspot/share/classfile/classLoaderData.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012, 2024, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2012, 2025, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -53,7 +53,6 @@
 // and provides iterators for root tracing and other GC operations.
 
 class ClassLoaderDataGraph;
-class JNIMethodBlock;
 class ModuleEntry;
 class PackageEntry;
 class ModuleEntryTable;
@@ -143,10 +142,9 @@ class ClassLoaderData : public CHeapObj<mtClass> {
   ModuleEntry* _unnamed_module;          // This class loader's unnamed module.
   Dictionary*  _dictionary;              // The loaded InstanceKlasses, including initiated by this class loader
 
-  // These method IDs are created for the class loader and set to null when the
-  // class loader is unloaded.  They are rarely freed, only for redefine classes
-  // and if they lose a data race in InstanceKlass.
-  JNIMethodBlock*                  _jmethod_ids;
+  // These method IDs are created for the class loader and removed when the
+  // class loader is unloaded.
+  GrowableArray<jmethodID>*        _jmethod_ids;
 
   // Metadata to be deallocated when it's safe at class unloading, when
   // this class loader isn't unloaded itself.
@@ -316,8 +314,9 @@ class ClassLoaderData : public CHeapObj<mtClass> {
   void classes_do(KlassClosure* klass_closure);
   Klass* klasses() { return _klasses; }
 
-  JNIMethodBlock* jmethod_ids() const              { return _jmethod_ids; }
-  void set_jmethod_ids(JNIMethodBlock* new_block)  { _jmethod_ids = new_block; }
+  void add_jmethod_id(jmethodID id);
+  void remove_jmethod_ids();
+  GrowableArray<jmethodID>* jmethod_ids() const { return _jmethod_ids; }
 
   void print() const;
   void print_on(outputStream* out) const PRODUCT_RETURN;

src/hotspot/share/memory/universe.cpp

@@ -60,6 +60,7 @@
 #include "oops/compressedOops.hpp"
 #include "oops/instanceKlass.hpp"
 #include "oops/instanceMirrorKlass.hpp"
+#include "oops/jmethodIDTable.hpp"
 #include "oops/klass.inline.hpp"
 #include "oops/objArrayOop.inline.hpp"
 #include "oops/objLayout.hpp"
@@ -436,6 +437,9 @@ void Universe::genesis(TRAPS) {
 
     vmSymbols::initialize();
 
+    // Initialize table for matching jmethodID, before SystemDictionary.
+    JmethodIDTable::initialize();
+
     SystemDictionary::initialize(CHECK);
 
     // Create string constants
@@ -444,7 +448,6 @@ void Universe::genesis(TRAPS) {
     s = StringTable::intern("-2147483648", CHECK);
     _the_min_jint_string = OopHandle(vm_global(), s);
 
-
 #if INCLUDE_CDS
     if (CDSConfig::is_using_archive()) {
       // Verify shared interfaces array.

src/hotspot/share/nmt/memTag.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012, 2024, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2012, 2025, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -58,6 +58,7 @@
   f(mtMetaspace,      "Metaspace")                                                   \
   f(mtStringDedup,    "String Deduplication")                                        \
   f(mtObjectMonitor,  "Object Monitors")                                             \
+  f(mtJNI,            "JNI")                                                         \
   f(mtNone,           "Unknown")                                                     \
   //end
 

src/hotspot/share/oops/instanceKlass.cpp

@@ -2395,13 +2395,26 @@ jmethodID InstanceKlass::update_jmethod_id(jmethodID* jmeths, Method* method, in
   return new_id;
 }
 
+// Allocate the jmethodID cache.
+static jmethodID* create_jmethod_id_cache(size_t size) {
+  jmethodID* jmeths = NEW_C_HEAP_ARRAY(jmethodID, size + 1, mtClass);
+  memset(jmeths, 0, (size + 1) * sizeof(jmethodID));
+  // cache size is stored in element[0], other elements offset by one
+  jmeths[0] = (jmethodID)size;
+  return jmeths;
+}
+
+// When reading outside a lock, use this.
+jmethodID* InstanceKlass::methods_jmethod_ids_acquire() const {
+  return Atomic::load_acquire(&_methods_jmethod_ids);
+}
+
+void InstanceKlass::release_set_methods_jmethod_ids(jmethodID* jmeths) {
+  Atomic::release_store(&_methods_jmethod_ids, jmeths);
+}
+
 // Lookup or create a jmethodID.
-// This code is called by the VMThread and JavaThreads so the
-// locking has to be done very carefully to avoid deadlocks
-// and/or other cache consistency problems.
-//
-jmethodID InstanceKlass::get_jmethod_id(const methodHandle& method_h) {
-  Method* method = method_h();
+jmethodID InstanceKlass::get_jmethod_id(Method* method) {
   int idnum = method->method_idnum();
   jmethodID* jmeths = methods_jmethod_ids_acquire();
 
@@ -2422,15 +2435,12 @@ jmethodID InstanceKlass::get_jmethod_id(const methodHandle& method_h) {
 
   if (jmeths == nullptr) {
     MutexLocker ml(JmethodIdCreation_lock, Mutex::_no_safepoint_check_flag);
-    jmeths = methods_jmethod_ids_acquire();
+    jmeths = _methods_jmethod_ids;
     // Still null?
     if (jmeths == nullptr) {
       size_t size = idnum_allocated_count();
       assert(size > (size_t)idnum, "should already have space");
-      jmeths = NEW_C_HEAP_ARRAY(jmethodID, size + 1, mtClass);
-      memset(jmeths, 0, (size + 1) * sizeof(jmethodID));
-      // cache size is stored in element[0], other elements offset by one
-      jmeths[0] = (jmethodID)size;
+      jmeths = create_jmethod_id_cache(size);
       jmethodID new_id = update_jmethod_id(jmeths, method, idnum);
 
       // publish jmeths
@@ -2460,10 +2470,7 @@ void InstanceKlass::update_methods_jmethod_cache() {
     if (old_size < size + 1) {
       // Allocate a larger one and copy entries to the new one.
       // They've already been updated to point to new methods where applicable (i.e., not obsolete).
-      jmethodID* new_cache = NEW_C_HEAP_ARRAY(jmethodID, size + 1, mtClass);
-      memset(new_cache, 0, (size + 1) * sizeof(jmethodID));
-      // The cache size is stored in element[0]; the other elements are offset by one.
-      new_cache[0] = (jmethodID)size;
+      jmethodID* new_cache = create_jmethod_id_cache(size);
 
       for (int i = 1; i <= (int)old_size; i++) {
         new_cache[i] = cache[i];
@@ -2474,24 +2481,30 @@ void InstanceKlass::update_methods_jmethod_cache() {
   }
 }
 
-// Figure out how many jmethodIDs haven't been allocated, and make
-// sure space for them is pre-allocated.  This makes getting all
-// method ids much, much faster with classes with more than 8
+// Make a jmethodID for all methods in this class.  This makes getting all method
+// ids much, much faster with classes with more than 8
 // methods, and has a *substantial* effect on performance with jvmti
 // code that loads all jmethodIDs for all classes.
-void InstanceKlass::ensure_space_for_methodids(int start_offset) {
-  int new_jmeths = 0;
+void InstanceKlass::make_methods_jmethod_ids() {
+  MutexLocker ml(JmethodIdCreation_lock, Mutex::_no_safepoint_check_flag);
+  jmethodID* jmeths = _methods_jmethod_ids;
+  if (jmeths == nullptr) {
+    jmeths = create_jmethod_id_cache(idnum_allocated_count());
+    release_set_methods_jmethod_ids(jmeths);
+  }
+
   int length = methods()->length();
-  for (int index = start_offset; index < length; index++) {
+  for (int index = 0; index < length; index++) {
     Method* m = methods()->at(index);
-    jmethodID id = m->find_jmethod_id_or_null();
-    if (id == nullptr) {
-      new_jmeths++;
+    int idnum = m->method_idnum();
+    assert(!m->is_old(), "should not have old methods or I'm confused");
+    jmethodID id = Atomic::load_acquire(&jmeths[idnum + 1]);
+    if (!m->is_overpass() &&  // skip overpasses
+        id == nullptr) {
+      id = Method::make_jmethod_id(class_loader_data(), m);
+      Atomic::release_store(&jmeths[idnum + 1], id);
     }
   }
-  if (new_jmeths != 0) {
-    Method::ensure_jmethod_ids(class_loader_data(), new_jmeths);
-  }
 }
 
 // Lookup a jmethodID, null if not found.  Do no blocking, no allocations, no handles
@@ -2923,7 +2936,7 @@ void InstanceKlass::release_C_heap_structures(bool release_sub_metadata) {
   JNIid::deallocate(jni_ids());
   set_jni_ids(nullptr);
 
-  jmethodID* jmeths = methods_jmethod_ids_acquire();
+  jmethodID* jmeths = _methods_jmethod_ids;
   if (jmeths != nullptr) {
     release_set_methods_jmethod_ids(nullptr);
     FreeHeap(jmeths);
@@ -4275,17 +4288,14 @@ bool InstanceKlass::should_clean_previous_versions_and_reset() {
   return ret;
 }
 
-// This nulls out jmethodIDs for all methods in 'klass'
-// It needs to be called explicitly for all previous versions of a class because these may not be cleaned up
-// during class unloading.
-// We can not use the jmethodID cache associated with klass directly because the 'previous' versions
-// do not have the jmethodID cache filled in. Instead, we need to lookup jmethodID for each method and this
-// is expensive - O(n) for one jmethodID lookup. For all contained methods it is O(n^2).
-// The reason for expensive jmethodID lookup for each method is that there is no direct link between method and jmethodID.
-void InstanceKlass::clear_jmethod_ids(InstanceKlass* klass) {
+// This nulls out the jmethodID for all obsolete methods in the previous version of the 'klass'.
+// These obsolete methods only exist in the previous version and we're about to delete the memory for them.
+// The jmethodID for these are deallocated when we unload the class, so this doesn't remove them from the table.
+void InstanceKlass::clear_obsolete_jmethod_ids(InstanceKlass* klass) {
   Array<Method*>* method_refs = klass->methods();
   for (int k = 0; k < method_refs->length(); k++) {
     Method* method = method_refs->at(k);
+    // Only need to clear obsolete methods.
     if (method != nullptr && method->is_obsolete()) {
       method->clear_jmethod_id();
     }
@@ -4335,7 +4345,7 @@ void InstanceKlass::purge_previous_version_list() {
       // Unlink from previous version list.
       assert(pv_node->class_loader_data() == loader_data, "wrong loader_data");
       InstanceKlass* next = pv_node->previous_versions();
-      clear_jmethod_ids(pv_node); // jmethodID maintenance for the unloaded class
+      clear_obsolete_jmethod_ids(pv_node); // jmethodID maintenance for the unloaded class
       pv_node->link_previous_versions(nullptr);   // point next to null
       last->link_previous_versions(next);
       // Delete this node directly. Nothing is referring to it and we don't

src/hotspot/share/oops/instanceKlass.hpp

@@ -781,8 +781,8 @@ class InstanceKlass: public Klass {
                                     u2 method_index);
 
   // jmethodID support
-  jmethodID get_jmethod_id(const methodHandle& method_h);
-  void ensure_space_for_methodids(int start_offset = 0);
+  jmethodID get_jmethod_id(Method* method);
+  void make_methods_jmethod_ids();
   jmethodID jmethod_id_or_null(Method* method);
   void update_methods_jmethod_cache();
 
@@ -1056,10 +1056,10 @@ class InstanceKlass: public Klass {
     Atomic::store(&_init_thread, thread);
   }
 
-  inline jmethodID* methods_jmethod_ids_acquire() const;
-  inline void release_set_methods_jmethod_ids(jmethodID* jmeths);
-  // This nulls out jmethodIDs for all methods in 'klass'
-  static void clear_jmethod_ids(InstanceKlass* klass);
+  jmethodID* methods_jmethod_ids_acquire() const;
+  void release_set_methods_jmethod_ids(jmethodID* jmeths);
+  // This nulls out obsolete jmethodIDs for all methods in 'klass'.
+  static void clear_obsolete_jmethod_ids(InstanceKlass* klass);
   jmethodID update_jmethod_id(jmethodID* jmeths, Method* method, int idnum);
 
 public:

src/hotspot/share/oops/instanceKlass.inline.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2015, 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2015, 2025, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -71,14 +71,6 @@ inline void InstanceKlass::release_set_array_klasses(ObjArrayKlass* k) {
   Atomic::release_store(&_array_klasses, k);
 }
 
-inline jmethodID* InstanceKlass::methods_jmethod_ids_acquire() const {
-  return Atomic::load_acquire(&_methods_jmethod_ids);
-}
-
-inline void InstanceKlass::release_set_methods_jmethod_ids(jmethodID* jmeths) {
-  Atomic::release_store(&_methods_jmethod_ids, jmeths);
-}
-
 // The iteration over the oops in objects is a hot path in the GC code.
 // By force inlining the following functions, we get similar GC performance
 // as the previous macro based implementation.