On Fri, 13 Jun 2025 19:39:39 GMT, Daniel D. Daugherty <dcu...@openjdk.org>
wrote:
>> Coleen Phillimore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix formatting errors.
>
> 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?
I don't want it tunable. I think it's fairly unlikely to grow. Even if it
does, I don't think it would cause a performance problem. I could name 32 as a
constant globally in classLoaderData.cpp in case we ever find we need to change
the initial 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/
It did change. Thank you for noticing it.
> 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.,/
ok.
> 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/
I thought the convention was that we were supposed to call it `null` in the
comments and `nullptr` in the code.
> 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`?
yes.
> 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).
yes moved.
> 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.
fixed. The spaces match the coding style. I fixed the others that you pointed
out below.
> src/hotspot/share/oops/instanceKlass.cpp line 2402:
>
>> 2400: return jmeths;
>> 2401: }
>> 2402:
>
> nit spacing: delete extra blank line?
fixed.
> src/hotspot/share/oops/instanceKlass.cpp line 2404:
>
>> 2402:
>> 2403:
>> 2404: jmethodID InstanceKlass::get_jmethod_id(Method* method) {
>
> Should `method` be `const`?
It's really unusual in our source code to pass const Metadata pointers because
of the history of the code. We should probably start doing that. I'll change
this to const and see if there's a fall out.
> 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/
I fixed the idnum+1 => idnum + 1 in this function.
> 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.
I thought the build checked this. thanks for noticing.
> 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`.
I think the rules are comments and strings say `null` and code is `nullptr`.
> 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?
I moved 'created' to where it's used but grow_hint and clean_hint are in the
order of the parameter list.
> 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.
much better
> 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/
clearing is better.
> 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?
Okay, I added one when we increment the jmethod_id_counter.
// Update jmethodID global counter.
_jmethodID_counter++;
guarantee(_jmethodID_counter != 0, "must never go back to zero");
I think this will detect wraparound.
> 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`?
I can't remember. There may have been another lock held while this one was
(which is why we added MUTEX_DEFL to help with that). I'll check.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2150302295
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2150302945
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2150308058
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2150309308
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2150312028
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2150319536
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2150320490
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2150321823
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2150326543
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2150338172
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2150340203
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2150344720
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2150354189
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2150356106
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2150358453
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2150366952
PR Review Comment: https://git.openjdk.org/jdk/pull/25267#discussion_r2150372817
