Lock-based vs lock-free MethodCounters initialization

Tue May 13 09:37:52 UTC 2014

On Monday 12 May 2014 12.17.24 John Rose wrote:
> 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?

Unfortunately it's a bit hard to measure fragmentation and waste in metaspace.
We do have some trace events for reporting the amount of memory used, 
committed and reserved for metaspace. Currently we don't keep track of the 
amount of dark matter memory (deallocations which do not fit in the free list) 
and we don't have any good way to expose information about the contents block 
freelists (which is where memory returned by explicit deallocations end up).

/Mikael

> 
> — 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/oo
> >>> ps/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/oo
> >>> ps/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