On Fri, 16 May 2025 12:18:42 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.
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'
nit typo: please add an ending period to the comment.
src/hotspot/share/oops/jmethodIDTable.cpp line 29:
> 27: #include "memory/resourceArea.hpp"
> 28: #include "oops/method.hpp"
> 29: #include "oops/jmethodIDTable.hpp"
Please swap these two #includes into sort order.
src/hotspot/share/oops/jmethodIDTable.cpp line 35:
> 33: #include "utilities/macros.hpp"
> 34:
> 35: // Save (jmethod, Method*) in a hashtable to lookup Method
nit typo: please add an ending period to the comment.
src/hotspot/share/oops/jmethodIDTable.cpp line 36:
> 34:
> 35: // Save (jmethod, Method*) in a hashtable to lookup Method
> 36: // The CHT is for performance because it is has lock free lookup.
typo: s/because it is has lock free/because it has lock free/
src/hotspot/share/oops/jmethodIDTable.cpp line 73:
> 71: // 2^24 is max size
> 72: const size_t end_size = 24;
> 73: // If a chain gets to 32 something might be wrong
nit typo: please add an ending period to the comment.
src/hotspot/share/oops/jmethodIDTable.cpp line 98:
> 96:
> 97: static JmethodEntry* get_jmethod_entry(jmethodID mid) {
> 98: assert(mid != nullptr, "JNI method id should not be null");
Perhaps: s/null/nullptr/
I can't remember if assert failure text output is okay to be `null`.
src/hotspot/share/oops/jmethodIDTable.cpp line 104:
> 102: JmethodEntry* result = nullptr;
> 103: auto get = [&] (JmethodEntry* value) {
> 104: // function called if value is found so is never null
Perhaps: s/null/nullptr/
src/hotspot/share/oops/jmethodIDTable.cpp line 109:
> 107: bool needs_rehashing = false;
> 108: _jmethod_id_table->get(current, lookup, get, &needs_rehashing);
> 109: assert (!needs_rehashing, "should never need rehashing");
nit spacing: s/assert (/assert(/
src/hotspot/share/oops/jmethodIDTable.cpp line 129:
> 127: }
> 128:
> 129: // Add a method id to the jmethod_ids
nit typo: please add an ending period to the comment.
src/hotspot/share/oops/jmethodIDTable.cpp line 131:
> 129: // Add a method id to the jmethod_ids
> 130: jmethodID JmethodIDTable::make_jmethod_id(Method* m) {
> 131: bool grow_hint, clean_hint, created;
nit: sort local variables?
src/hotspot/share/oops/jmethodIDTable.cpp line 144:
> 142: log_debug(jmethod)("Inserted jmethod id " UINT64_FORMAT_X,
> _jmethodID_counter);
> 143:
> 144: // Resize table if it needs to grow. The _jmethod_id_table has a good
> distribution
nit typo: please add an ending period to the comment.
src/hotspot/share/oops/jmethodIDTable.cpp line 158:
> 156: JmethodEntry* result;
> 157: auto get = [&] (JmethodEntry* value) {
> 158: // function called if value is found so is never null
Perhaps: s/null/nullptr/
src/hotspot/share/oops/jmethodIDTable.cpp line 162:
> 160: };
> 161: bool removed = _jmethod_id_table->remove(current, lookup, get);
> 162: assert(removed, "should be");
"should be" or "must be"?
src/hotspot/share/oops/jmethodIDTable.cpp line 169:
> 167: assert_locked_or_safepoint(JmethodIdCreation_lock);
> 168: JmethodEntry* result = get_jmethod_entry(jmid);
> 169: // change to table to point to the new method
Perhaps: // Change to table entry to point to the new method.
src/hotspot/share/oops/jmethodIDTable.cpp line 180:
> 178: // We need to make sure that jmethodID actually resolves to this method
> 179: // - multiple redefined versions may share jmethodID slots and if a
> method
> 180: // has already been rewired to a newer version we could be removing
> reference
typo?: s/could be removing reference/could be clearing a reference/
src/hotspot/share/oops/jmethodIDTable.hpp line 31:
> 29: #include "memory/allocation.hpp"
> 30:
> 31: // Class for associating Method with jmethodID
nit typo: please add an ending period to the comment.
src/hotspot/share/oops/jmethodIDTable.hpp line 38:
> 36: static void initialize();
> 37:
> 38: // Given a Method return a jmethodID
nit typo: please add an ending period to the comment.
src/hotspot/share/oops/jmethodIDTable.hpp line 41:
> 39: static jmethodID make_jmethod_id(Method* m);
> 40:
> 41: // Given a jmethodID, return a Method
nit typo: please add an ending period to the comment.
src/hotspot/share/oops/jmethodIDTable.hpp line 45:
> 43:
> 44: // Class unloading support, remove the associations from the tables.
> Stale jmethodID will
> 45: // not be found and return null
nit typo: please add an ending period to the comment.
Perhaps: s/null/nullptr/
src/hotspot/share/oops/method.cpp line 2063:
> 2061:
> 2062: // jmethodID handling
> 2063: // jmethodIDs are 64-bit integers that will never run out and are
> mapped in a table
Should we have a `guarantee` or `assert` somewhere that the counter never wraps?
src/hotspot/share/oops/method.cpp line 2065:
> 2063: // jmethodIDs are 64-bit integers that will never run out and are
> mapped in a table
> 2064: // to their Method and vice versa. If JNI code has access to stale
> jmethodID, this
> 2065: // wastes no memory but the Method* returned is null
Perhaps: s/null/nullptr/
src/hotspot/share/oops/method.cpp line 2076:
> 2074: assert(jmid != nullptr, "must be created");
> 2075:
> 2076: // Add to growable array in CLD
nit typo: please add an ending period to the comment.
src/hotspot/share/oops/method.cpp line 2088:
> 2086: // Get the Method out of the table given the method id.
> 2087: Method* Method::resolve_jmethod_id(jmethodID mid) {
> 2088: assert(mid != nullptr, "JNI method id should not be null");
Perhaps: s/null/nullptr/
src/hotspot/share/oops/method.hpp line 709:
> 707: // Use resolve_jmethod_id() in situations where the caller is expected
> 708: // to provide a valid jmethodID; the only sanity checks are in asserts;
> 709: // result guaranteed not to be null.
Perhaps: s/null/nullptr/
src/hotspot/share/oops/method.hpp line 713:
> 711:
> 712: // Use checked_resolve_jmethod_id() in situations where the caller
> 713: // should provide a valid jmethodID, but might not. Null is returned
Perhaps: s/Null/Nullptr/
src/hotspot/share/prims/jvmtiEnv.cpp line 2773:
> 2771: int skipped = 0; // skip overpass methods
> 2772:
> 2773: // Make jmethodIDs for all non-overpass methods
nit typo: please add an ending period to the comment.
src/hotspot/share/prims/jvmtiEnv.cpp line 2789:
> 2787: }
> 2788: jmethodID id = m->find_jmethod_id_or_null();
> 2789: assert (id != nullptr, "should be created above");
nit spacing: s/assert (/assert(/
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.
Interesting. Why change from `nosafepoint-2` to `nosafepoint-1`?
-------------
PR Review: https://git.openjdk.org/jdk/pull/25267#pullrequestreview-2926153300
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2145955175
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2145963559
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2145960811
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2145967055
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2145984911
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2145989746
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2145993097
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2145994055
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2146005043
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2145996873
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2145997702
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2145998880
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2146002943
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2146006426
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2146007219
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2146007725
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2146008289
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2146011312
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2146013418
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2146029756
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2146014609
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2146018654
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2146016370
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2146017327
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2146029142
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2146019950
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2146022181
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2146024577
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2146025893
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2146026406
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2146026860
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2146029258
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2146037800
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2146031309
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2146039166
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2146040503
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2146045442
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2146047075
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2146049138
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2146050371
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2146052800
