Lock-based vs lock-free MethodCounters initialization

[Vladimir Ivanov]

Thanks, Jiangli & David.

Yes, I agree that more understanding of the behavior will be very 
helpful. I'll try to come up with a stress test and run some benchmarks 
to see how common is the contention during allocation.

Best regards,
Vladimir Ivanov

On 5/12/14 8:51 PM, Jiangli Zhou wrote:
> 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
>