On Wed, 3 Apr 2024 12:42:30 GMT, Coleen Phillimore <cole...@openjdk.org> wrote:
>> src/hotspot/share/oops/instanceKlass.cpp line 2335:
>>
>>> 2333: jmethodID new_id = Method::make_jmethod_id(class_loader_data(),
>>> method);
>>> 2334: Atomic::release_store(&jmeths[idnum+1], new_id);
>>> 2335: return new_id;
>>
>> Nit: It feels like the function `InstanceKlass::get_jmethod_id()` can be
>> more simplified with a small restructuring:
>>
>> jmethodID update_jmethod_id(jmethodID* jmeths, Method* method, int idnum) {
>> // method_with_idnum
>> if (method->is_old() && !method->is_obsolete()) {
>> // The method passed in is old (but not obsolete), we need to use the
>> current version
>> method = method_with_idnum((int)idnum);
>> assert(method != nullptr, "old and but not obsolete, so should exist");
>> }
>> jmethodID new_id = Method::make_jmethod_id(class_loader_data(), method);
>> Atomic::release_store(&jmeths[idnum+1], new_id);
>> return new_id;
>> }
>>
>> jmethodID InstanceKlass::get_jmethod_id(const methodHandle& method_h) {
>> Method* method = method_h();
>> int idnum = method_h->method_idnum();
>> jmethodID* jmeths = methods_jmethod_ids_acquire();
>>
>> <... big comment ...>
>> MutexLocker ml(JmethodIdCreation_lock, Mutex::_no_safepoint_check_flag);
>> if (jmeths == nullptr) {
>> jmeths = methods_jmethod_ids_acquire();
>>
>> if (jmeths == nullptr) { // Still null?
>> 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;
>> jmethodID new_id = update_jmethod_id(jmeths, method, idnum);
>>
>> // publish jmeths
>> release_set_methods_jmethod_ids(jmeths);
>> return new_id;
>> }
>> }
>> jmethodID id = Atomic::load_acquire(&jmeths[idnum+1]);
>> if (id == nullptr) {
>> id = jmeths[idnum+1];
>>
>> if (id == nullptr) { // Still null?
>> jmethodID new_id = update_jmethod_id(jmeths, method, idnum);
>> return new_id;
>> }
>> }
>> return id;
>> }
>
> Yes this refactoring looks nice. Nice to have only one place that checks for
> !is_obsolete.
Thank you for the suggestion, I reran the jvmti tests locally.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18549#discussion_r1549733870
