RFR: 8292375: Convert ProtectionDomainCacheTable to ResourceHashtable [v3]

[David Holmes]

On Fri, 26 Aug 2022 17:07:54 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> Please review this simple conversion for the ProtectionDomainCacheTable from Old Hashtable to ResourceHashtable.  There are specific tests for this table in test/hotspot/jtreg/runtime/Dictionary and serviceability/dcmd/vm/DictionaryStatsTest.java.
>> Also tested with tier1-7.
>
> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   include mutexLocker.hpp for minimal build.

Hi Coleen,

Probably due to my lack of understanding of the existing code I found this conversion very hard to follow. A number of comments below.

Thanks.

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?

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?

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

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.

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!

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

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?

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

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