On Fri, 5 May 2023 12:07:20 GMT, Coleen Phillimore <[email protected]> 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
