On Fri, 26 Aug 2022 17:07:54 GMT, Coleen Phillimore <cole...@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
