On Mon, 22 Mar 2021 21:42:51 GMT, Ioi Lam <ik...@openjdk.org> wrote:
>> From CR:
>> The useful/general BasicHashtable uses a block allocation scheme to
>> reportedly reduce fragmentation. When the StringTable and SymbolTable used
>> to use this hashtable, performance benefits were reportedly observed because
>> of the block allocation scheme. Since these tables were moved to the
>> concurrent hashtables, the tables left that use the block allocation scheme
>> are:
>>
>> AdapterHandlerLibrary, ResolutionError, LoaderConstraints, Leak profiler
>> bitset table and Placeholders. 3 of these tables are very small and never
>> needed block allocation to prevent fragmentation at least. Also there are 3
>> KVHashtables, which are built from BasicHashtable. 2 are used during dumping
>> and 1 is ID2KlassTable which appears small.
>>
>> ModuleEntry, PackageEntry, Dictionary, G1RootSet for nmethods, and
>> JvmtiTagMap tables didn't use the block allocation scheme.
>>
>> Removing this removes 7 pointers per table, and for each ClassLoaderData,
>> which has 3 tables, removes 21 pointers.
>>
>> This change was performance tested on linux and windows.
>>
>> It was also tested with tier1-6.
>
> src/hotspot/share/utilities/hashtable.cpp line 54:
>
>> 52: BasicHashtableEntry<F>* entry = ::new (NEW_C_HEAP_ARRAY(char,
>> this->entry_size(), F))
>> 53:
>> BasicHashtableEntry<F>(hashValue);
>> 54: assert(_entry_size % HeapWordSize == 0, "");
>
> Is this assert still needed? What's its purpose?
Yeah, seems a bit strange. Of course the entry is a heapword size. I'll
remove the assert.
> src/hotspot/share/utilities/hashtable.cpp line 64:
>
>> 62:
>> 63: if (DumpSharedSpaces) {
>> 64: // Avoid random bits in structure padding so we can have
>> deterministic content in CDS archive
>
> Hmm, the sequence looks a little odd: the constructor initializes some
> fields, they are then zeroed here, and then initialized again below ....
>
> Actually, I think you can get rid of the `if (DumpSharedSpaces)` block for
> now. I am not sure if it's needed. Deterministic CDS archive is broken anyway
> (https://bugs.openjdk.java.net/browse/JDK-8253495).
>
> That way you can get rid of the entry->set_xxx below as well.
Yes, I'd like to make the constructors initialize the fields, but didn't know
what to do about this block zeroing code. Would you have to add it back with
deterministic GC?
-------------
PR: https://git.openjdk.java.net/jdk/pull/3123
