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

[Mikael Gerdin]

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