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

[David Holmes]

On 27/05/2016 3:30 PM, Stefan Karlsson wrote:
> On 2016-05-27 00:36, David Holmes wrote:
>> Hi Kim,
>>
>> Thanks for looking at this.
>>
>> 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. ??
>>
>>> 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.
>>
>>> ------------------------------------------------------------------------------
>>>
>>> src/share/vm/classfile/classLoaderData.cpp
>>>  197 void ClassLoaderData::modules_do(void f(ModuleEntry*)) {
>>>  209 void ClassLoaderData::packages_do(void f(PackageEntry*)) {
>>>
>>> Based on the RFR, I'm guessing these are only called at a safepoint or
>>> while holding Module_lock?  Maybe add assert_locked_or_safepoint to
>>> these too?
>>>
>>> (Maybe these are only called from the corresponding
>>> ClassLoaderDataGraph functions, which already have such an assertion.
>>> But more local assertions in the CLD functions would make things
>>> easier to understand.)
>>
>> AFAICS yes they are only called from the CLDG functions. I don't like
>> adding redundant assertions but okay. (Even debug builds have time
>> constraints!)
>>
>>> ------------------------------------------------------------------------------
>>>
>>> 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.
>
> It used to be the case that Klasses were deleted from the_klasses list
> while other threads iterated over it. Coleen removed those instances with:
> http://mail.openjdk.java.net/pipermail/hotspot-dev/2014-March/013283.html
>
> If we still cut Klasses out of the CLD::_klasses list outside of a GC
> safepoint, then that's a bug.

Thanks Stefan! So if I read this all correctly CLD::remove_class doesn't 
need the lock and we should be able to assert it is called at a safepoint.

David
-----


> Thanks,
> StefanK
>
>> Thanks,
>> David
>>
>