On Mon, 29 Aug 2022 12:24:33 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: > > David's comments Looks good overall and seems to be equivalent to the old code. Just a couple of nits. 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(); // reacquires SD lock It's confusing to have two similarly named functions (purge_deleted_entries and purge_entries_from_table). Maybe the two functions should be combined into a single `purge()` function that performs the two steps? src/hotspot/share/classfile/protectionDomainCache.hpp line 35: > 33: // The amount of different protection domains used is typically > magnitudes smaller > 34: // than the number of system dictionary entries (loaded classes). > 35: class ProtectionDomainCacheTable : public AllStatic { How about adding a comment to say what this table maps from/to? Something like: // The ProtectionDomainCacheTable maps all java.security.ProtectionDomain objects that are // registered by DictionaryEntry::add_protection_domain() to a unique WeakHandle. ------------- Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.org/jdk/pull/10043