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

[Stefan Karlsson]

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,
StefanK

> Thanks,
> David
>