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

[Coleen Phillimore]

On Wed, 7 Apr 2021 04:49:02 GMT, David Holmes <dholmes 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.
>
> 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.

Only the ServiceThread calls this code, so they won't be removed again.  This is modeled after the ObjectMonitor code that does the same thing.  All the threads have to hit a handshake at a safepoint polling place, which they cannot do while reading the list.  After all the threads have hit the handshake, no thread can see the old entries on the list.
I thought that's what I said in the comment.  What should I say to make this clearer?

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

Thanks, I'll add acquire/release and change the names to match the implementation details.

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

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