RFR: 8313332: Simplify lazy jmethodID cache in InstanceKlass [v2]

[Coleen Phillimore]

On Wed, 3 Apr 2024 12:42:30 GMT, Coleen Phillimore <coleenp at 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