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

[David Holmes]

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