RFR: 8313332: Simplify lazy jmethodID cache in InstanceKlass

[Coleen Phillimore]

On Wed, 3 Apr 2024 02:41:06 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> This change simplifies the code that grows the jmethodID cache in InstanceKlass.  Instead of lazily, when there's a rare request for a jmethodID for an obsolete method, the jmethodID cache is grown during the RedefineClasses safepoint.  The InstanceKlass's jmethodID cache is lazily allocated when there's a jmethodID allocated, so not every InstanceKlass has a cache, but the growth now only happens in a safepoint.  This code will become racy with the potential change for deallocating jmethodIDs.
>> 
>> Tested with tier1-4, vmTestbase/nsk/jvmti java/lang/instrument tests (in case they're not in tier1-4).
>
> 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.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/18549#discussion_r1549664452