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

[Igor Veresov]

Looks good.

igor

On 1/11/11 3:48 PM, Vladimir Kozlov wrote:
> 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.
>>