On Fri, 29 Mar 2024 15:25:48 GMT, Coleen Phillimore <cole...@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 2277:
> 2275: jmethodID InstanceKlass::get_jmethod_id(const methodHandle& method_h) {
> 2276: Method* method = method_h();
> 2277: int idnum = method_h->method_idnum();
Nit: Can use `method` instead of `method_h()`.
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;
}
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18549#discussion_r1548840596
PR Review Comment: https://git.openjdk.org/jdk/pull/18549#discussion_r1548839930
