Lock-based vs lock-free MethodCounters initialization

[Jiangli Zhou]

Hi Vladimir,

I worked on MethodCounters allocation and planned to look into the 
possible memory leak at some point, but haven't gotten to it. Thanks for 
looking into this! The possible contention of a MethodCounters 
allocation only happens when the method is being executed the first 
time.  I did some quick profiling and determined that it was rare. If 
that was the case, metspace fragmentation due to deallocations of 
MethodCounters followed by failed CAS would also be uncommon. The 
lock-free based solution seems to be the safer and more efficient one to 
go for.

I see David and Mikael already responded to your question already. Both 
of them provided very good information. Maybe it's worth to do some 
profiling first?

Thanks,

Jiangli

On 05/12/2014 04:19 AM, 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?
>
> 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