RFR: 8292680: Convert Dictionary to ConcurrentHashTable [v4]

[Johan Sjölén]

On Wed, 24 Aug 2022 14:30:14 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> This change converts the dictionaries per ClassLoaderData from Old Hashtable to ConcurrentHashTables.  The CHT has a few extra fields (locks and such) but the Old Hashtable had TableRateStatistics so footprint is similar.  The Dictionaries require lock-free lookups because we lookup classes that we've already loaded whenever their names appear in the source code, which is pretty much all the time.  The Dictionaries retain the SystemDictionary_lock for writing.  Locking for adding entries gates other things like compiler dependencies and placeholder operations and loader constraint checking.  The one thing that we can do with the CHT is do a resize without going to a safepoint, so this can resize dictionaries during adding.  There's a test for this.
>> 
>> Entries in the dictionaries are never individually deleted.  When the class loader for the ClassLoaderData holding the dictionary is unloaded, the entire dictionary is deleted.  Note dictionary is an old name for hashtable of loaded classes.  We're keeping that name.
>> I added a test for printing dictionary statistics.
>> 
>> This has been tested with tier1-7, with tier1 on all Oracle platforms.  I've also performance tested this with our internal performance test suite with slight improvement but not statistically significant.
>
> Coleen Phillimore has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:
> 
>  - Merge branch 'master' into cht-dictionary
>  - Conditionalize scanning function call for safepoint.
>  - Use ConcurrentHashtable for Dictionary

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

> 409: // During class loading we may have cached a protection domain that has
> 410: // since been unreferenced, so this entry should be cleared.
> 411: void Dictionary::clean_cached_protection_domains(GrowableArray<ProtectionDomainEntry*>* delete_list) {

Only mention of `GrowableArray` is here and it's a pointer, so do we really need to include the `growableArray.hpp` header? A forward declaration should be sufficient. Not entirely sure, maybe the templates screw it up.

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

> 410: // since been unreferenced, so this entry should be cleared.
> 411: void Dictionary::clean_cached_protection_domains(GrowableArray<ProtectionDomainEntry*>* delete_list) {
> 412:   assert(JavaThread::current()->is_Java_thread(), "only called by JavaThread");

`JavaThread::current` already does this check, so just leave it as `Thread::current()`.

Edit, to be clear: JavaThread::current() will fail its assertion before `->is_Java_thread()` is ever called on this line.

src/hotspot/share/classfile/dictionary.hpp line 49:

> 47:   class Config {
> 48:    public:
> 49:     typedef DictionaryEntry* Value;

Nit: Is there a reason that we're mixing `using` and `typedef` (see line 55)?

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

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