On Wed, 3 Apr 2024 13:25:36 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).
>
> Coleen Phillimore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Refactoring suggested by Serguei.
Okay I've crawled thru the changes twice and I went back thru
the bug history for this code and added some notes and links
to the bug ID.
Modulo the nits that I flagged, I think the changes are fine. Making
cache growth only happen in the RedefineClasses safepoint is
definite improvement.
I see that you've run JVM/TI and JLI tests. You should also run JDI
tests. Basically for a low level fix like this that affects JVM/TI, you
should run Mach5 Tier[1-6].
src/hotspot/share/oops/instanceKlass.cpp line 2272:
> 2270: jmethodID InstanceKlass::update_jmethod_id(jmethodID* jmeths, Method*
> method, int idnum) {
> 2271: if (method->is_old() && !method->is_obsolete()) {
> 2272: // If the method passed in is old (but not obsolete), use the
> current version
nit: should end with a period.
src/hotspot/share/oops/instanceKlass.cpp line 2277:
> 2275: }
> 2276: jmethodID new_id = Method::make_jmethod_id(class_loader_data(),
> method);
> 2277: Atomic::release_store(&jmeths[idnum+1], new_id);
nit: spaces around operator `+`
src/hotspot/share/oops/instanceKlass.cpp line 2304:
> 2302: //
> 2303: // If the RedefineClasses() API has been used, then this cache grows
> 2304: // in the redefinition safepoint.
Much easier to reason about. Thanks for simplifying it.
src/hotspot/share/oops/instanceKlass.cpp line 2314:
> 2312: assert(size > (size_t)idnum, "should already have space");
> 2313: jmeths = NEW_C_HEAP_ARRAY(jmethodID, size+1, mtClass);
> 2314: memset(jmeths, 0, (size+1)*sizeof(jmethodID));
nit: spaces around operator `+` (two places)
nit: spaces around operator `*`
src/hotspot/share/oops/instanceKlass.cpp line 2325:
> 2323: }
> 2324:
> 2325: jmethodID id = Atomic::load_acquire(&jmeths[idnum+1]);
nit: spaces around operator `+`
src/hotspot/share/oops/instanceKlass.cpp line 2328:
> 2326: if (id == nullptr) {
> 2327: MutexLocker ml(JmethodIdCreation_lock,
> Mutex::_no_safepoint_check_flag);
> 2328: id = jmeths[idnum+1];
nit: spaces around operator `+`
src/hotspot/share/oops/instanceKlass.cpp line 2343:
> 2341: size_t size = idnum_allocated_count();
> 2342: size_t old_size = (size_t)cache[0];
> 2343: if (old_size < size+1) {
nit: spaces around operator `+`
src/hotspot/share/oops/instanceKlass.cpp line 2344:
> 2342: size_t old_size = (size_t)cache[0];
> 2343: if (old_size < size+1) {
> 2344: // allocate a larger one and copy entries to the new one.
nit typo: s/allocate/Allocate/
src/hotspot/share/oops/instanceKlass.cpp line 2345:
> 2343: if (old_size < size+1) {
> 2344: // allocate a larger one and copy entries to the new one.
> 2345: // They've already been updated to point to new methods where
> applicable (ie. not obsolete)
nit typo: s/ie./i.e.,/
Please add a period at the end of the sentence.
src/hotspot/share/oops/instanceKlass.cpp line 2347:
> 2345: // They've already been updated to point to new methods where
> applicable (ie. not obsolete)
> 2346: jmethodID* new_cache = NEW_C_HEAP_ARRAY(jmethodID, size+1,
> mtClass);
> 2347: memset(new_cache, 0, (size+1)*sizeof(jmethodID));
nit: spaces around operator `+` (two places)
nit: spaces around operator `*`
src/hotspot/share/oops/instanceKlass.cpp line 2348:
> 2346: jmethodID* new_cache = NEW_C_HEAP_ARRAY(jmethodID, size+1,
> mtClass);
> 2347: memset(new_cache, 0, (size+1)*sizeof(jmethodID));
> 2348: // cache size is stored in element[0], other elements offset by
> one
nit typo: s/cache/Cache/
Please add a period at the end.
src/hotspot/share/oops/instanceKlass.cpp line 2384:
> 2382: int idnum = method->method_idnum();
> 2383: jmethodID* jmeths = methods_jmethod_ids_acquire();
> 2384: return (jmeths != nullptr) ? jmeths[idnum+1] : nullptr;
nit: spaces around operator `+`
src/hotspot/share/oops/method.cpp line 2200:
> 2198:
> 2199: ResourceMark rm;
> 2200: log_info(jmethod)("Creating jmethodID for Method %s",
> m->external_name());
Hmmm... will this be too noisy for `info` level?
src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 4353:
> 4351: the_class->itable().initialize_itable();
> 4352:
> 4353: // Update jmethodID cache if present
Nit: should end with period.
-------------
Marked as reviewed by dcubed (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18549#pullrequestreview-1977965153
PR Review Comment: https://git.openjdk.org/jdk/pull/18549#discussion_r1550473408
PR Review Comment: https://git.openjdk.org/jdk/pull/18549#discussion_r1550476292
PR Review Comment: https://git.openjdk.org/jdk/pull/18549#discussion_r1550488626
PR Review Comment: https://git.openjdk.org/jdk/pull/18549#discussion_r1550492405
PR Review Comment: https://git.openjdk.org/jdk/pull/18549#discussion_r1550491163
PR Review Comment: https://git.openjdk.org/jdk/pull/18549#discussion_r1550492871
PR Review Comment: https://git.openjdk.org/jdk/pull/18549#discussion_r1550499076
PR Review Comment: https://git.openjdk.org/jdk/pull/18549#discussion_r1550499498
PR Review Comment: https://git.openjdk.org/jdk/pull/18549#discussion_r1550500350
PR Review Comment: https://git.openjdk.org/jdk/pull/18549#discussion_r1550501547
PR Review Comment: https://git.openjdk.org/jdk/pull/18549#discussion_r1550503608
PR Review Comment: https://git.openjdk.org/jdk/pull/18549#discussion_r1550505968
PR Review Comment: https://git.openjdk.org/jdk/pull/18549#discussion_r1550458475
PR Review Comment: https://git.openjdk.org/jdk/pull/18549#discussion_r1550449234
