RFR: 8292375: Convert ProtectionDomainCacheTable to ResourceHashtable [v3]

[Coleen Phillimore]

On Mon, 29 Aug 2022 00:12:36 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   include mutexLocker.hpp for minimal build.
>
> src/hotspot/share/classfile/protectionDomainCache.cpp line 42:
> 
>> 40: #include "utilities/resourceHash.hpp"
>> 41: 
>> 42: unsigned int ProtectionDomainCacheTable::compute_hash(const WeakHandle& protection_domain) {
> 
> Why are we now using `WeakHandle` everywhere?

WeakHandle is the object we're storing as in the hashtable.  It also turns out to be the key.

> src/hotspot/share/classfile/protectionDomainCache.cpp line 43:
> 
>> 41: 
>> 42: unsigned int ProtectionDomainCacheTable::compute_hash(const WeakHandle& protection_domain) {
>> 43:   return (unsigned int)(protection_domain.resolve()->identity_hash());
> 
> And if it is a `WeakHandle` can't `resolve` now return NULL?

compute_hash() is always called on a live WeakHandle so will never return null.

> src/hotspot/share/classfile/protectionDomainCache.cpp line 50:
> 
>> 48: }
>> 49: 
>> 50: ResourceHashtable<WeakHandle, WeakHandle, 1009, ResourceObj::C_HEAP, mtClass,
> 
> I am struggling to understand what the key and values types are in this hashtable ???

WeakHandle is the key and the value in this hashtable.  We need to match the WeakHandle in the input by value (see equals function), but we also want to return the WeakHandle to store in to the pd_set list in the DictionaryEntry.

> src/hotspot/share/classfile/protectionDomainCache.cpp line 162:
> 
>> 160:   // Purge any deleted entries outside of the SystemDictionary_lock.
>> 161:   purge_deleted_entries();
>> 162:   int oops_removed = purge_entries_from_table();
> 
> Maybe add comment
> 
> `int oops_removed = purge_entries_from_table(); // reacquires SD lock`
> 
> otherwise it gives the false impression this is done lock-free.

Ok.

> src/hotspot/share/classfile/protectionDomainCache.cpp line 168:
> 
>> 166: }
>> 167: 
>> 168: void ProtectionDomainCacheTable::print_on(outputStream* st) {
> 
> It is a little disconcerting that `print_on` can no longer be a `const` function!

It's static, so it can't.

> src/hotspot/share/classfile/protectionDomainCache.cpp line 186:
> 
>> 184: 
>> 185: // The object_no_keepalive() call peeks at the phantomly reachable oop without
>> 186: // keeping it alive. This is okay to do in the VM thread state if it is not
> 
> You don't call `object_no_keepalive()` any more

This one (not the one I removed), is called by dictionary.cpp - the pd_set is a linked list of ProtectionDomainEntry, where we don't keep the WeakHandle alive when looking at the value.

> src/hotspot/share/classfile/protectionDomainCache.cpp line 202:
> 
>> 200:     // Keep entry alive
>> 201:     (void)wk->resolve();
>> 202:     return *wk;
> 
> Can't this be factored out of the if-else as `wk` is always the right entry to resolve and return?

Yes, that works.

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

PR: https://git.openjdk.org/jdk/pull/10043