RFR (M) 8055283: Expand ResourceHashtable with C_HEAP allocation, removal and some unit tests

[Coleen Phillimore]

Hi Mikael,    This looks fine.

I think it's #ifndef PRODUCT that you should use to wrap the unit test code.

Could you also put some string above it that it's a unit test?  Like in 
chunkedList.cpp?

/////////////// Unit tests ///////////////

thanks,
Coleen


On 10/27/15 7:40 AM, Erik Helin wrote:
> Hi Mikael,
>
> On 2015-10-16, Mikael Gerdin wrote:
>> Hi all,
>>
>> Here's an old favorite. I'm yet again in need of a simple-to-use hash table
>> in a place where resource allocation is impossible to use.
> Thanks for taking care of this!
>
>> The idea is to add a ResourceObj::allocation_type template parameter to
>> ResourceHashtable to allow a user to specify C-heap allocation using
>> ResourceObj::C_Heap.
>>
>> Currently if one tries to do
>> {
>> ResourceMark rm;
>> ResourceHash* hash = new (ResourceObj::C_HEAP) ResourceHash();
>> hash->put(...)
>> }
>> hash->get(...)
>>
>> The get()-call will crash because the allocation of the hash table nodes
>> does not respect the C_HEAP parameter.
>>
>> To sweeten the deal a little I add support for removing elements and also a
>> small bunch of unit tests to verify that the operations work as expected.
>>
>> I also discovered a small issue with the primitive_hash() function which is
>> the default one:
>>    36 template<typename K> unsigned primitive_hash(const K& k) {
>>    37   unsigned hash = (unsigned)((uintptr_t)k);
>>    38   return hash ^ (hash > 3); // just in case we're dealing with aligned
>> ptrs
>>    39 }
>>
>> It xors hash with the boolean expression (hash > 3) instead of what was
>> probably intended (hash >> 3) to deal with aligned pointers as the comment
>> suggests.
>> It appears that the only user of ResourceHash which doesn't specify its own
>> hash function is MethodFamily::_member_index so it would be nice if someone
>> could point me at any default method tests to verify that a proper hash
>> function doesn't break anything.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8055283
>> Webrev: http://cr.openjdk.java.net/~mgerdin/8055283/webrev.1/
> Please wrap all your test code in #ifdef ASSERT, we don't want it to be
> included in product builds. Then you can also get rid of the #ifdef
> ASSERT in the test.
>
> Modulo the above and Thomas's comments, the patch looks good to me. No
> need to re-review these changes.
>
> Thanks,
> Erik
>
>> Testing: JPRT
>> RBT (Kitchensnk, hotspot/test/:hotspot_all)
>>
>>
>> Thanks!
>> /Mikael