RFR: 8313332: Simplify lazy jmethodID cache in InstanceKlass

[Serguei Spitsyn]

On Fri, 29 Mar 2024 15:25:48 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).

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