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

[Kim Barrett]

> On May 26, 2016, at 6:36 PM, David Holmes <david.holmes at oracle.com> wrote:
> On 27/05/2016 5:56 AM, Kim Barrett wrote:
>>> On May 24, 2016, at 4:03 AM, David Holmes <david.holmes at oracle.com> wrote:
>>> 
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8154750
>>> 
>>> webrev: http://cr.openjdk.java.net/~dholmes/8154750/webrev/
>> 
>> Thanks for fixing these uses of DCLP.
>> 
>> ------------------------------------------------------------------------------
>> src/share/vm/classfile/classLoaderData.cpp
>>  80 // helper function to avoid in-line casts
>>  81 template <typename T> static T load_ptr_acquire(volatile T* p) {
>>  82   return static_cast<T>(OrderAccess::load_ptr_acquire(p));
>>  83 }
>> 
>> For better type safety, maybe this should instead be
>> 
>> template<typename T> static T* load_ptr_acquire(T* volatile * p) {
>>  return static_cast<T*>(OrderAccess::load_ptr_acquire(p));
>> }
> 
> Can you explain to me what difference this makes please. The field types are all "Foo*" so I'm unclear what the difference is between writing this in a form where T is Foo* and T is Foo. ??

It would prevent one from inadvertently passing a Foo* rather than a Foo**, which would presently
compile but likely crash fairly quickly.  The existing uses in this file all appear to be correct (not
surprisingly), but accidents happen.  Even more important if this were moved to OrderAccess.

> 
>> This also seems like it might be generally useful.  Would it make
>> sense to add it to OrderAccess?
> 
> I tried and could not get it to work. As I said earlier in the thread it either failed to compile or else became self-referential and hit an infinite loop at runtime. Trying to deal with that seemed out-of-scope for what I was trying to achieve here. I was also worried about silently changing other uses of load_ptr_acquire.
> 
> That said, your formulation of it may work better than mine. So I will give it another try.

It might require renaming the existing load_ptr_acquire(void*), or an explicit conversion in the template.
I’ve always felt kind of squeamish about load_ptr_acquire(void*).  I think making it an implementation
detail (private, with a different name, e.g. load_ptr_acquire_impl) that this template (moved into
OrderAccess) uses would add convenience and safety.  Assuming doing so doesn’t break something
that is making assumptions about it.

That would require updating the existing definitions for the new name, and possibly updating at least
some callers (many callers probably now look like (Foo*)load_ptr_acquire(…), and the explicit cast
becomes redundant with this suggested change, so can be removed incrementally).  OTOH, there
aren’t all that many callers...

That’s obviously a substantial change, and clearly out of scope for this task.

>> src/share/vm/classfile/classLoaderData.cpp
>> 352 // This is called by InstanceKlass::deallocate_contents() to remove the
>> 353 // scratch_class for redefine classes.  We need a lock because there it may not
>> 354 // be called at a safepoint if there's an error.
>> 355 void ClassLoaderData::remove_class(Klass* scratch_class) {
>>     ... and modifies _klasses list ...
>> 
>> How are the unlocked iterators over the _klasses list protected from
>> the modifications performed here?  The description is suggestive that
>> they might not be (e.g. need lock because might not be at a
>> safepoint).
> 
> I'll have to check this but I think this deletion can only be done in a context where you can't be iterating the klasses. Lifecycle management is unclear to me (I'm not that familiar with the CLD code). May need the CLD experts to chime in.

I think that’s probably right, but I kept getting lost in code I’d never looked at before.