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

[David Holmes]

Hi Karen,

On 3/06/2016 12:32 AM, Karen Kinnear wrote:
> 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?

Good question. Normally I always make any variable accessed lock-free, 
volatile - "just to be on the safe side". I had overlooked it here but 
will fix it (seeing as how my push has failed to happen anyway).

Does it make any difference in this case? I think not but would have to 
defer to the compiler experts. Normally we apply 'volatile' to C 
variables to make sure the compiler doesn't do anything to them that 
might disrupt their correct use in lock-free environments when they 
might change without the compiler knowing. In this code the lock-free 
access is a load_ptr_acquire which already has such constraints built in 
to it.

Thanks,
David

> 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.
>>>
>