RFR: 8313332: Simplify lazy jmethodID cache in InstanceKlass [v2]

Wed Apr 3 22:09:11 UTC 2024

On Wed, 3 Apr 2024 13:25:36 GMT, Coleen Phillimore <coleenp 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).
>
> 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