RFR: 8290812: Add a test for ResourceHashtable [v7]

[Coleen Phillimore]

On Wed, 3 Aug 2022 05:15:42 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Expand to 5 tests and one that's simple.
>
> test/hotspot/gtest/utilities/test_resourceHash.cpp line 363:
> 
>> 361:         }
>> 362:     };
>> 363: };
> 
> It's a bit challenging to read 5 consecutive test cases that are subtly different. Maybe SimpleDeleter should be moved to a separate class, like ResourceHashtableSimpleDeleteTest. Then we can separate the test cases into a group of 2 and another group of 3.

I could do more splitting.

> test/hotspot/gtest/utilities/test_resourceHash.cpp line 429:
> 
>> 427:   _test_table.unlink(&deleter);
>> 428:   ASSERT_EQ(d->refcount(), d_orig_count) << "refcount should be as we started";
>> 429: }
> 
> `value_delete` and `value_remove` use the exact same table instance. It's unclear from the example why `value_delete` needs to manipulate the key's refcount explicitly, but the latter doesn't.

I was trying to say it's not strictly necessary.  When you have a remove function, you have a chance to delete the refcount outside the hashtable, but you have to do it in the do_entry function if you use unlink.   I'll try to make the test clearer.  I guess my comments are more ruminations rather than helpful.

> test/hotspot/gtest/utilities/test_resourceHash.cpp line 439:
> 
>> 437:     // refcount outside the hashtable functions.
>> 438:     s->increment_refcount();
>> 439:     _ptr_test_table.put(s, tv);
> 
> I am not sure why it's necessary to do s->increment_refcount() here and key->decrement_refcount() in PtrDeleter. We know that the symbol will be kept alive by the TestValue. These two inc/dec calls are done where we know that the TestValue is alive (after instantiation, and before deallocation). By removing these calls, we can reduce the chance of mishandling the refcount.

Yes, I was trying to show that you may have to manipulate the refcount in the Key if it's not kept alive by being the same in the Value. In this case it is.  Adding the SimpleDeleter to show that does a better job of that though.

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

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