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

[Mikael Gerdin]

Hi Coleen,

On 2015-10-27 15:54, Coleen Phillimore wrote:
>
> Hi Mikael,    This looks fine.

Thanks.

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

I see, I looked at the block in jni.cpp and you are right.
It's a bit weird though since most of the actual tests use assert() 
which of course is ifdef ASSERT.
Anyway, I've changed it as per your request.

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

Sure, no problem.

Incremental webrev at:
http://cr.openjdk.java.net/~mgerdin/8055283/webrev.1_to_2/
Full webrev at:
http://cr.openjdk.java.net/~mgerdin/8055283/webrev.2

/Mikael


>
> 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
>