RFR: 8259242: vmTestbase/vm/mlvm/mixed/stress/java/findDeadlock/TestDescription.java timed out

[David Holmes]

On Tue, 6 Apr 2021 18:54:09 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

> This change makes the DictionaryEntry pd_set lock free, because when I set a low SafepointTimeout on the findDeadlock test, these threads were waiting on the ProtectionDomainSet_lock.  This lock is a singleton that's taken for every dictionary entry.
> 
> I don't know if this change will make the test never timeout again, because removing this lock caused us to hit a low SafepointTimeout on another _no_safepoint_check lock that is not so easy to eradictate.
> 
> Tested with tiers 1-3.  There's a test that exercises this code in runtime/Dictionary/ProtectionDomainCacheTest.java.

Hi Coleen,

Can you explain the synchronization protocol you are using here please - I don't understand how the handshake fits into it ??

Further comments below.

Thanks,
David

src/hotspot/share/classfile/dictionary.cpp line 422:

> 420: void Dictionary::clean_cached_protection_domains(GrowableArray<ProtectionDomainEntry*>* delete_list) {
> 421:   assert(Thread::current()->is_Java_thread(), "only called by JavaThread");
> 422:   assert_locked_or_safepoint(SystemDictionary_lock);

If the current thread must be a JavaThread then we cannot be at a safepoint and so the second assert can just check the lock is held.

src/hotspot/share/classfile/dictionary.hpp line 145:

> 143: 
> 144:   ProtectionDomainEntry* pd_set() const            { return Atomic::load(&_pd_set); }
> 145:   void set_pd_set(ProtectionDomainEntry* entry)    { Atomic::store(&_pd_set, entry); }

These probably need to be load_acquire and release_store, so that a lock-free traversal can proceed correctly with a locked addition/removal. With suitable name changes for the methods.

src/hotspot/share/classfile/protectionDomainCache.cpp line 89:

> 87:   // If there are any deleted entries, Handshake-all then they'll be
> 88:   // safe to remove since traversing the pd_set list does not stop for
> 89:   // safepoints and only JavaThreads will read the pd_set.

I do not understand what you are trying to do here. This is basically a no-op handshake with every thread? Does all the threads remain in the handshake until the last one is processed or do they proceed through one at a time? The former seems better suited for a safepoint operation. The latter means this doesn't work as they may have moved on to a new removal.

src/hotspot/share/classfile/protectionDomainCache.hpp line 115:

> 113: 
> 114:   ProtectionDomainEntry* next() { return Atomic::load(&_next); }
> 115:   void set_next(ProtectionDomainEntry* entry) { Atomic::store(&_next, entry); }

Again these may need to be load_acquire/release_store for correct lock-free traversal. With suitable name changes for the methods.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3362