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

Thomas Schatzl thomas.schatzl at oracle.com
Mon Oct 19 08:10:32 UTC 2015


Hi,

On Fri, 2015-10-16 at 10:30 +0200, 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.
> 
> 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/

  - resourceHash.?pp: if you change copyrights, please change them to
the correct year :)

  - resourceHash.cpp: missing include reference to allocation.hpp (for
AllStatic), debug.hpp (for assert)

I do not think I need a re-review for these changes.

Seems okay to me otherwise.

Thanks,
  Thomas



More information about the hotspot-dev mailing list