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

David Holmes david.holmes at oracle.com
Fri May 27 03:39:31 UTC 2016


Hi Kim,

Updated webrev:

http://cr.openjdk.java.net/~dholmes/8154750/webrev.v2/

More inline ...

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));
> }

Changed.

> This also seems like it might be generally useful.  Would it make
> sense to add it to OrderAccess?
>
> ------------------------------------------------------------------------------
> 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.)

Added the additional asserts directly in those functions. However given 
the CLDG functions loop calling the CLD functions I am a little bit 
concerned about the redundancy and overhead. I'll leave it to the CLD to 
say if they have any concerns here.

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

This was not what I initially thought it was (which was the destructor 
code). The remove_class function is safe from an OrderAccess perspective 
- and I've added a comment to that effect. However you have hit on a 
general concurrency problem. I don't see anything that considers what 
happens if one of the lock-free iteration methods encounters a class 
that is concurrently being removed. Even if this is safe by construction 
(ie these two things can't happen at the same time) there is no 
commentary to that effect. So this is either a bug or needs an 
explanatory comment. Again I'll let the CLD folk comment on that.

Thanks,
David


More information about the hotspot-runtime-dev mailing list