Request for reviews (XS): 7011386: race in objArrayKlass::array_klass_impl

[Tom Rodriguez]

On Jan 11, 2011, at 11:15 AM, Vladimir Kozlov wrote:

> Tom,
> 
> Using oop_store() instead of oop_store_without_check()
> will generate barriers but it is overkill.

Those are gc barriers.  I was really referring to fences.  The code is a classic instance of the double check locking idiom for lazy init:

  objArrayKlassHandle ak (THREAD, this_oop->higher_dimension());
  if (ak.is_null()) {
    if (or_null)  return NULL;

    ResourceMark rm;
    JavaThread *jt = (JavaThread *)THREAD;
    {
      MutexLocker mc(Compile_lock, THREAD);   // for vtables
      // Ensure atomic creation of higher dimensions
      MutexLocker mu(MultiArray_lock, THREAD);

      // Check if another thread beat us
      ak = objArrayKlassHandle(THREAD, this_oop->higher_dimension());
      if( ak.is_null() ) {

So doesn't that mean that we need a fence of some kind?

tom

> I think marking receiver as volatile should be enough:
> 
>  void set_higher_dimension(klassOop k) volatile { oop_store_without_check((oop*) &_higher_dimension, (oop) k); }
>  void set_lower_dimension(klassOop k)  volatile { oop_store_without_check((oop*) &_lower_dimension, (oop) k); }
> 
> Thanks,
> Vladimir
> 
> Tom Rodriguez wrote:
>> That look good, though I keep wondering if we need a barrier in between or if those fields really should be volatile.  It seems like we're playing a little loose with the locking for these lazy values.
>> tom
>> On Jan 11, 2011, at 9:54 AM, Vladimir Kozlov wrote:
>>> http://cr.openjdk.java.net/~kvn/7011386/webrev
>>> 
>>> Fixed 7011386: race in objArrayKlass::array_klass_impl
>>> 
>>> Other threads may access _lower_dimension field before
>>> it is initialized by thread which holds the lock in
>>> objArrayKlass::array_klass_impl().
>>> 
>>> Move _lower_dimension field initialization before
>>> _higher_dimension.