RFR: 8306843: JVMTI tag map extremely slow after JDK-8292741 [v2]

[Ioi Lam]

On Fri, 5 May 2023 12:07:20 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> The ResourceHashtable conversion for JDK-8292741 didn't add the resizing code.  The old hashtable code was tuned for resizing in anticipation of large hashtables for JVMTI tags.  This patch ports over the old hashtable resizing code.  It also adds a ResourceHashtable::put_fast() function that prepends to the bucket list, which is also reclaims the performance of the old hashtable for this test with 10M tags.  The ResourceHashtable put function is really a put_if_absent. This can be cleaned up in a future change.  Also, the remove function needed a lambda to destroy the WeakHandle, since resizing requires copying entries.
>> 
>> Tested with JVMTI and JDI tests locally, and tier1-4 tests.
>
> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Remove return variable from remove lambda, fix formatting.

I can't comment on the JVMTI changes, but the changes in the hashtable code seems OK to me.

src/hotspot/share/classfile/stringTable.cpp line 638:

> 636:  public:
> 637:   size_t _errors;
> 638:   VerifyCompStrings() : _table(unsigned(_items_count / 8) + 1, 0 /* do not resize */), _errors(0) {}

Shouldn't this use a regular ResourceHashtable instead?

src/hotspot/share/utilities/resizeableResourceHash.hpp line 91:

> 89:   // Calculate next "good" hashtable size based on requested count
> 90:   int calculate_resize(bool use_large_table_sizes) const {
> 91:     const int resize_factor = 2;     // by how much we will resize using current number of entries

Does this function depend on the template parameters? If not, I think it can be made a static function -- you may need to pass `BASE::number_of_entries()` in as a parameter.

src/hotspot/share/utilities/resourceHash.hpp line 147:

> 145:   */
> 146:   bool put_fast(K const& key, V const& value) {
> 147:     unsigned hv = HASH(key);

I think `put_fast` is not clear enough. Maybe `put_must_be_absent()` or something more concise.

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

PR Review: https://git.openjdk.org/jdk/pull/13818#pullrequestreview-1416091781
PR Review Comment: https://git.openjdk.org/jdk/pull/13818#discussion_r1187009635
PR Review Comment: https://git.openjdk.org/jdk/pull/13818#discussion_r1187005281
PR Review Comment: https://git.openjdk.org/jdk/pull/13818#discussion_r1187009805