RFR: 8158854: Ensure release_store is paired with load_acquire in lock-free code

[Zhengyu Gu]

Hi David,

The changes look good to me.

Just a minor comment:

I saw you made InstanceKlass::_array_klasses pointer "volatile", but not 
some other places. I know that probably it has not effort, but should we 
make all these pointers "volatile" just for consistency?

Thanks,

-Zhengyu



On 08/22/2016 12:16 AM, David Holmes wrote:
> I went to push this and realized I hadn't hg add'ed the new
>
> src/share/vm/oops/arrayKlass.inline.hpp
>
> which is also missing from the webrev (but now updated in place).
>
> Thanks,
> David
>
> On 19/08/2016 12:02 PM, Daniel D. Daugherty wrote:
>> On 8/17/16 8:50 PM, David Holmes wrote:
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8158854
>>>
>>> webrev: http://cr.openjdk.java.net/~dholmes/8158854/webrev/
>>
>> src/share/vm/classfile/classLoader.hpp
>>     No comments.
>>
>> src/share/vm/classfile/verifier.cpp
>>     No comments.
>>
>> src/share/vm/oops/arrayKlass.hpp
>>     No comments.
>>
>> src/share/vm/oops/instanceKlass.cpp
>>     No comments.
>>
>> src/share/vm/oops/instanceKlass.hpp
>>     No comments.
>>
>> src/share/vm/oops/instanceKlass.inline.hpp
>>     No comments.
>>
>> src/share/vm/oops/objArrayKlass.cpp
>>     No comments.
>>
>> src/share/vm/oops/typeArrayKlass.cpp
>>     No comments.
>>
>> src/share/vm/runtime/vmStructs.cpp
>>     No comments.
>>
>> Thumbs up.
>>
>> Dan
>>
>>
>>>
>>>
>>> Generally speaking release_store should be paired with load_acquire to
>>> ensure correct memory visibility and ordering in lock-free code (often
>>> the read path is what is lock-free). So based on some observations
>>> from earlier bug fixes this bug was intended to examine the use of
>>> release_store and see if we have the appropriate load_acquire as well.
>>> The bug report lists all of the cases that were examined - some clear
>>> cut correct, some complex correct, some fixed here and some split out
>>> into separate issues.
>>>
>>> Here's a summary of the actual changes in the webrev:
>>>
>>>  src/share/vm/classfile/classLoader.hpp
>>>
>>> - next() accessor needs to use load_acquire.
>>>
>>> ---
>>>
>>> src/share/vm/classfile/verifier.cpp
>>>
>>> - load of _verify_byte_codes_fn needs to load_acquire to pair with use
>>> of release_store
>>> - release_store of _is_new_verify_byte_codes_fn is not needed
>>>
>>> ---
>>>
>>> src/share/vm/oops/arrayKlass.hpp
>>> src/share/vm/oops/instanceKlass.cpp
>>> src/share/vm/oops/instanceKlass.hpp
>>> src/share/vm/oops/instanceKlass.inline.hpp
>>> src/share/vm/oops/objArrayKlass.cpp
>>> src/share/vm/oops/typeArrayKlass.cpp
>>>
>>> The logic for storing dimensions values was using a storeStore barrier
>>> between the lower and higher dimensions. This is converted to use a
>>> release-store setter for higher-dimension, with paired load-acquire
>>> accessor. Plus the accessed fields are declared volatile.
>>>
>>> The methods_jmethod_ids_acquire() and its paired
>>> release_set_methods_jmethod_ids(), are moved to the .inline.hpp file
>>> where they belong.
>>>
>>> ---
>>>
>>> src/share/vm/runtime/vmStructs.cpp
>>>
>>> Updated declaration for _array_klasses  now it is volatile.
>>>
>>> ---
>>>
>>> Thanks,
>>> David
>>