On Mon, 22 Mar 2021 15:49:24 GMT, Coleen Phillimore <cole...@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.
Looks good overall. Just some small nits.
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?
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.
-------------
Changes requested by iklam (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3123
