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

[Coleen Phillimore]

Thanks!  Looks good.
Coleen

On 6/1/16 9:02 PM, David Holmes 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/
>
> Thanks,
> David