RFR: 8154750: Add missing OrderAccess operations to ClassLoaderData lock-free data structures

[Karen Kinnear]

David,

Thank you for fixing this and catching the error. My bad for suggesting we remove the StoreStore rather than
figuring out why someone had thought we needed it initially.

Question for you - do we need _klasses, _packages and _modules to be declared as volatile?

thanks,
Karen

> On Jun 2, 2016, at 2:48 AM, David Holmes <david.holmes at oracle.com> wrote:
> 
> Thanks Kim and Coleen!
> 
> David
> 
> On 2/06/2016 4:28 PM, Kim Barrett wrote:
>>> On Jun 1, 2016, at 9:02 PM, David Holmes <david.holmes at oracle.com> wrote:
>>> 
>>> Hi Coleen,
>>> 
>>> Thanks for reviewing this. I'm pulling all your comments into this end email.
>>> 
>>> On 1/06/2016 7:35 AM, Coleen Phillimore wrote:
>>>> Yes, this is true. Klasses can be added outside a safepoint to the
>>>> end but not deleted. Deletion is always at a safepoint. It wasn't
>>>> always at a safepoint...
>>>> 
>>>> Can you fix this comment before remove_class()?  This is wrong now. > It should say:
>>>> 
>>>> // Remove a klass from the _klasses list for scratch_class during
>>>> redefinition or parsed class in the case of an error.
>>> 
>>> Changed.
>>> 
>>>> And add an assert that you're at a safepoint?
>>> 
>>> Already added in webrev .v3
>>> 
>>> On 1/06/2016 7:57 AM, Coleen Phillimore wrote:
>>>> Can you put a comment in modules_do to describe why you don't need
>>>> load_ptr_acquire?
>>> 
>>> I hoped the assertions made it clear this was a locked or safepoint path - load_ptr_acquire is only needed on lock-free non-safepoint paths. I've added comments on the lock-free paths.
>>> 
>>>> in modules_do and here:
>>>> 
>>>> + // Check if _modules got allocated while we were waiting for this lock.
>>>> + if ((modules = _modules) == NULL) {
>>>> 
>>>> The racing thread has done a store_release so you don't need another
>>>> load_acquire after you get the lock?
>>> 
>>> You don't need a load_acquire because you have just acquired the lock that was released after the store (the fact it was a store_release is irrelevant in that case - it is the lock that provides acquire/release memory consistency).
>>> 
>>> On 1/06/2016 8:08 AM, Coleen Phillimore wrote:
>>>> This pattern also exists in
>>>> 
>>>> InstanceKlass::mask_for()
>>>> 
>>>> which is primarily called walking frames (could be in parallel) during a
>>>> safepoint.
>>>> 
>>>> Could you apply your change there also?
>>> 
>>> Only for you Coleen! :)
>>> 
>>> Updated webrev: http://cr.openjdk.java.net/~dholmes/8154750/webrev.v4/
>> 
>> Looks good.
>>