Request for review: 6922246: Deadlock during intensive class loading in parallel

[David Holmes]

Hi Coleen,

coleen phillimore said the following on 10/20/10 08:11:
> Summary: allocate methodData before taking out the MethodData_lock
> open webrev at http://cr.openjdk.java.net/~coleenp/6922246/
> bug link at http://bugs.sun.com/view_bug.do?bug_id=6922246
> 
> Thanks to Ramki for the diagnosis and suggestion for the fix.

This is a variation on the unsafe double-checked-locking idiom.

286   if (method->method_data() == NULL) {
291     methodDataOop method_data = oopFactory::new_methodData(method, 
CHECK);
292     MutexLocker ml(MethodData_lock, THREAD);
295     if (method->method_data() == NULL) {
296       method->set_method_data(method_data);

In general terms any stores associated with the method_data construction 
are not guaranteed to be visible by the time set_method_data is called. 
A second thread could then see a non-null method->method_data() but 
access uninitialized method_data values.

A general solution would need memory barriers before 286 and after 291 
to ensure correctness on all platforms. A simpler, though perhaps 
heavier solution would be to use a MutexUnlocker around the allocation:

MutexLocker ml(MethodData_lock, THREAD);
if (method->method_data() == NULL) {
   methodDataOop method_data = NULL;
   { // need new block for scope
     MutexUnLocker ml(MethodData_lock, THREAD);
     method_data = oopFactory::new_methodData(method, CHECK);
   }
   // someone might have beaten us to it
  if (method->method_data() == NULL) {
     method->set_method_data(method_data);
  }
}

A lock-less solution as Tom proposed would also require suitable 
memory-barriers.

David