RFR: 8259242: Remove ProtectionDomainSet_lock [v3]

[David Holmes]

On Thu, 8 Apr 2021 23:09:49 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> This change makes the DictionaryEntry pd_set reads 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.
>
> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Make ProtectionDomainEntry delete list global and set a threshold for deletions before handshaking. Fix if to when.

Hi Coleen,

Thanks for the additional explanations about the synchronization protocol that is being used. The use of the global handshake with the OM deletion had not really registered with me as a general technique that we would employ for safe-memory-reclamation. (I'm not sure if I find it cool or scary! :) )

So to summarise the protocol:
- the pd_set can only be set and appended to when the lock is held
- traversal of a pd_set is lock-free
- removal from a pd_set is a two phase process:
-   1. Under the lock move the entry to the global delete list
-   2. When appropriate the serviceThread will process the delete list, by first handshaking all threads, and then iterating through and deleting the entries

We know an entry cannot be in-use when deleted because that implies a traversal is in progress, but if a traversal is in progress then the handshake cannot have happened.

Is that correct?

I would like to see this high-level description as a block comment somewhere for clarity, as the individual pieces of code seem spread around and it is not easy (for me) to see the connections when just reading the code.

Thanks,
David

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

> 146:          "can only be called by a JavaThread or at safepoint");
> 147:   // This cannot safepoint while reading the protection domain set.
> 148:   NoSafepointVerifier nsv;

Does this implicitly also mean NoHandshakeVerifier? Or do we need a seperate NHV?

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

> 453:             prev->release_set_next(current->next_acquire());
> 454:           }
> 455:           delete_list->push(current);

Suggested comment before this:
// Mark current for deletion, but in the meantime it can still be traversed

It is important that current is not unlinked so that in-progress traversals do not break. Though to be honest I'm unclear exactly what triggers the clearing of an entry this way.

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

> 558: void DictionaryEntry::print_count(outputStream *st) {
> 559:   int count = 0;
> 560:   for (ProtectionDomainEntry* current = pd_set_acquire();  // accessed inside SD lock

Can the end-of-line comment get promoted to a full line comment before the loop - or even an assert that the lock is held.

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

> 114:     _delete_list = new (ResourceObj::C_HEAP, mtClass)
> 115:                        GrowableArray<ProtectionDomainEntry*>(20, mtClass);
> 116:   }

What is protecting this code to ensure the allocation can only happen once?

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

Marked as reviewed by dholmes (Reviewer).

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