On Wed, 3 Apr 2024 02:41:06 GMT, Serguei Spitsyn <sspit...@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