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

[Vladimir Kozlov]

You are right, I added store_store barriers and make these fields volatile:

http://cr.openjdk.java.net/~kvn/7011386/webrev

Vladimir

Tom Rodriguez wrote:
> 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.
>