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

[Mikael Gerdin]

Hi Thomas,

On 2015-10-19 10:10, Thomas Schatzl wrote:
> Hi,
>
> On Fri, 2015-10-16 at 10:30 +0200, 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.
>>
>> 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/
>    - resourceHash.?pp: if you change copyrights, please change them to
> the correct year :)

Oh, but I actually did those changes last year :)

>
>    - resourceHash.cpp: missing include reference to allocation.hpp (for
> AllStatic), debug.hpp (for assert)
Will do.

>
> I do not think I need a re-review for these changes.
>
> Seems okay to me otherwise.

Thanks
/Mikael
>
> Thanks,
>    Thomas
>