Lock-based vs lock-free MethodCounters initialization

[Mikael Gerdin]

Hi Vladimir,

On Monday 12 May 2014 15.19.04 Vladimir Ivanov wrote:
> Hi,
> 
> There's a possible memory leak when allocating MethodCounters
> concurrently [1], because there's no coordination between concurrent
> counters initialization actions. In constrast with MethodCounters,
> MethodData allocation is guarded by MethodData lock.
> 
> I'm thinking about proper fix and considering 2 options: lock-based
> (reuse MethodData_lock or introduce MethodCounters_lock) and lock-free
> (do a CAS on _method_counters field) [3].
> 
> The problem with lock-based approach is that there's a possible deadlock
> with pending list lock (see JDK-6988439 [4] for details), so I need to
> check whether PLL is held and abort initialization in case it is
> (similar to what Method::build_interpreter_method_data does before
> acquiring the lock [2]).
> 
> Lock-free solution doesn't suffer from deadlocks, but it can increase
> Metaspace fragmentation due to failed CAS updates and subsequent
> deallocations.
> 
> I'm in favor of lock-free solution, but not sure about how serious
> fragmentation issue is in NPG world. What do you think?

1. Are MethodCounters objects large enough to fit on the per-metaspace block 
free list? See:
MetaWord* BlockFreelist::get_block(size_t word_size) {
  (...)
  if (word_size < TreeChunk<Metablock, FreeList<Metablock> >::min_size()) {
    // Dark matter.  Too small for dictionary.
    return NULL;
  }

2. If the above is true, are all MethodCounters the same size? I know how you 
guys love your variable-sized objects :)


If this is not true then we could encounter some severe fragmentation in 
Metaspace. There are already several issues with fragmentation and waste and I 
would prefer if we could avoid adding to the problem.

/Mikael


> 
> Thanks!
> 
> Best regards,
> Vladimir Ivanov
> 
> [1]
> http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/tip/src/share/vm/oops/m
> ethod.cpp#l384
> 
> MethodCounters* Method::build_method_counters(Method* m, TRAPS) {
>    methodHandle mh(m);
>    ClassLoaderData* loader_data = mh->method_holder()->class_loader_data();
>    MethodCounters* counters = MethodCounters::allocate(loader_data,
> CHECK_NULL);
>    if (mh->method_counters() == NULL) {
>      mh->set_method_counters(counters);
>    } else {
>      MetadataFactory::free_metadata(loader_data, counters);
>    }
>    return mh->method_counters();
> }
> 
> [2]
> http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/tip/src/share/vm/oops/m
> ethod.cpp#l360
> 
> void Method::build_interpreter_method_data(methodHandle method, TRAPS) {
>    // Do not profile method if current thread holds the pending list lock,
>    // which avoids deadlock for acquiring the MethodData_lock.
>    if (InstanceRefKlass::owns_pending_list_lock((JavaThread*)THREAD)) {
>      return;
>    }
> 
>    // Grab a lock here to prevent multiple
>    // MethodData*s from being created.
>    MutexLocker ml(MethodData_lock, THREAD);
>    if (method->method_data() == NULL) {
>      ClassLoaderData* loader_data =
> method->method_holder()->class_loader_data();
>      MethodData* method_data = MethodData::allocate(loader_data, method,
> CHECK);
>      method->set_method_data(method_data);
>      if (PrintMethodData && (Verbose || WizardMode)) {
>        ResourceMark rm(THREAD);
>        tty->print("build_interpreter_method_data for ");
>        method->print_name(tty);
>        tty->cr();
>        // At the end of the run, the MDO, full of data, will be dumped.
>      }
>    }
> }
> 
> [3] Possible lock-free solution:
> MethodCounters* Method::build_method_counters(Method* m, TRAPS) {
>    methodHandle mh(m);
>    if (mh->method_counters() != NULL)  return mh->method_counters();
>    ClassLoaderData* loader_data = mh->method_holder()->class_loader_data();
>    MethodCounters* counters = MethodCounters::allocate(loader_data,
> CHECK_NULL);
>    MethodCounters* old = (MethodCounters*)Atomic::cmpxchg_ptr(counters,
> &_method_counters, NULL);
>    if (old != NULL) {
>      MetadataFactory::free_metadata(loader_data, counters);
>    }
>    return mh->method_counters();
> }
> 
> [4] https://bugs.openjdk.java.net/browse/JDK-6988439