Lock-based vs lock-free MethodCounters initialization

[John Rose]

Lock-free should be our default if we think contention is rare, for reasons already given.

BUT, we should back up our intuition with a measurement, such as a pair of counters that track successful vs. failed CAS events.  We can then detect unexpectedly frequent failed CAS events and treat them as a possible bug.

Since we are noticing occasional fragmentation problems, we can also correlate them to those new counters.  If fragmentation is a risk, we need to detect it routinely and mechanically.  Mikael G., which metrics do we use to detect excessive holes in the metaspace heap?

— John

On May 12, 2014, at 11:56 AM, Vladimir Ivanov <vladimir.x.ivanov at oracle.com> wrote:

> 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
>>