On Wed, 3 Apr 2024 02:41:06 GMT, Serguei Spitsyn <[email protected]> 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
