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

[Coleen Phillimore]

Yes, it looks good to me.
Thanks,
Coleen

On 11/3/15 8:23 AM, Mikael Gerdin wrote:
> Hi Coleen,
> are you ok with the changes in webrev.2?
>
> /Mikael
>
> On 2015-10-28 08:38, Mikael Gerdin wrote:
>> 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
>>>
>>
>