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

[David Holmes]

On 27/05/2016 9:27 AM, Kim Barrett wrote:
>> 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.

Ah! you mean like when I first started on this and called 
load_ptr_acquire(_var) instead of load_ptr_acquire(&_var) :)

Okay that sounds like a good thing to prevent.

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.

So the bottom line there is, this is out of scope. Right? :) I really 
don't want to start touching other uses of this API.

Thanks,
David

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