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

[David Holmes]

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.

Thanks,
David