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

[Coleen Phillimore]

On 5/27/16 1:30 AM, 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.

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.

And add an assert that you're at a safepoint?

I'm still reviewing the other code.

Coleen

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