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

Kim Barrett kim.barrett at oracle.com
Thu Jun 2 06:28:16 UTC 2016


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



More information about the hotspot-runtime-dev mailing list