Merge pull request #901 from capnproto/fix-hash

Detect and report when a HashMap suffers from excessive collisions.

c++/src/capnp/schema.c++

@@ -896,12 +896,24 @@ uint Type::hashCode() const {
     case schema::Type::FLOAT64:
     case schema::Type::TEXT:
     case schema::Type::DATA:
+      if (listDepth == 0) {
+        // Make sure that hashCode(Type(baseType)) == hashCode(baseType), otherwise HashMap lookups
+        // keyed by `Type` won't work when the caller passes `baseType` as the key.
+        return kj::hashCode(baseType);
+      } else {
         return kj::hashCode(baseType, listDepth);
+      }
 
     case schema::Type::STRUCT:
     case schema::Type::ENUM:
     case schema::Type::INTERFACE:
+      if (listDepth == 0) {
+        // Make sure that hashCode(Type(schema)) == hashCode(schema), otherwise HashMap lookups
+        // keyed by `Type` won't work when the caller passes `schema` as the key.
+        return kj::hashCode(schema);
+      } else {
         return kj::hashCode(schema, listDepth);
+      }
 
     case schema::Type::LIST:
       KJ_UNREACHABLE;
@@ -911,7 +923,7 @@ uint Type::hashCode() const {
       // both branches compile to the same instructions and can optimize it away.
       uint16_t val = scopeId != 0 || isImplicitParam ?
           paramIndex : static_cast<uint16_t>(anyPointerKind);
-      return kj::hashCode(val, isImplicitParam, scopeId);
+      return kj::hashCode(val, isImplicitParam, scopeId, listDepth);
     }
   }
 

c++/src/kj/map.h

@@ -123,7 +123,7 @@ private:
       return e.key == key;
     }
     template <typename KeyLike>
-    inline bool hashCode(KeyLike&& key) const {
+    inline auto hashCode(KeyLike&& key) const {
       return kj::hashCode(key);
     }
   };
@@ -252,7 +252,7 @@ public:
   template <typename T, typename U>
   inline bool matches(T& a, U& b) const { return a == b; }
   template <typename KeyLike>
-  inline bool hashCode(KeyLike&& key) const {
+  inline auto hashCode(KeyLike&& key) const {
     return kj::hashCode(key);
   }
 };

c++/src/kj/table.c++

@@ -161,16 +161,30 @@ kj::Array<HashBucket> rehash(kj::ArrayPtr<const HashBucket> oldBuckets, size_t t
   auto newBuckets = kj::heapArray<HashBucket>(size);
   memset(newBuckets.begin(), 0, sizeof(HashBucket) * size);
 
+  uint entryCount = 0;
+  uint collisionCount = 0;
+
   for (auto& oldBucket: oldBuckets) {
     if (oldBucket.isOccupied()) {
+      ++entryCount;
       for (uint i = oldBucket.hash % newBuckets.size();; i = probeHash(newBuckets, i)) {
         auto& newBucket = newBuckets[i];
         if (newBucket.isEmpty()) {
           newBucket = oldBucket;
           break;
         }
+        ++collisionCount;
+      }
     }
   }
+
+  if (collisionCount > 16 + entryCount * 4) {
+    static bool warned = false;
+    if (!warned) {
+      KJ_LOG(WARNING, "detected excessive collisions in hash table; is your hash function OK?",
+          entryCount, collisionCount, kj::getStackTrace());
+      warned = true;
+    }
   }
 
   return newBuckets;