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

[David Holmes]

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