Lock-based vs lock-free MethodCounters initialization

[Vladimir Ivanov]

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?

Thanks!

Best regards,
Vladimir Ivanov

[1] 
http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/tip/src/share/vm/oops/method.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/method.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