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

Coleen Phillimore coleen.phillimore at oracle.com
Tue Oct 27 14:54:59 UTC 2015

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::allocationtype template parameter to ResourceHashtable to allow a user to specify C-heap allocation using ResourceObj::CHeap. Currently if one tries to do { ResourceMark rm; ResourceHash* hash = new (ResourceObj::CHEAP) ResourceHash(); hash->put(...) } hash->get(...) The get()-call will crash because the allocation of the hash table nodes does not respect the CHEAP 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 primitivehash() function which is the default one: 36 template unsigned primitivehash(const K& k) { 37 unsigned hash = (unsigned)((uintptrt)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::memberindex 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/:hotspotall)

Thanks! /Mikael

More information about the hotspot-dev mailing list