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

Mikael Gerdin mikael.gerdin at oracle.com
Wed Oct 28 07:38:49 UTC 2015

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