Commit 2fedce2d authored by Kenton Varda's avatar Kenton Varda

Detect and report when a HashMap suffers from excessive collisions.

And fix a bug detected by this.
parent 181cc66a
...@@ -896,12 +896,24 @@ uint Type::hashCode() const { ...@@ -896,12 +896,24 @@ uint Type::hashCode() const {
case schema::Type::FLOAT64: case schema::Type::FLOAT64:
case schema::Type::TEXT: case schema::Type::TEXT:
case schema::Type::DATA: case schema::Type::DATA:
return kj::hashCode(baseType, listDepth); 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::STRUCT:
case schema::Type::ENUM: case schema::Type::ENUM:
case schema::Type::INTERFACE: case schema::Type::INTERFACE:
return kj::hashCode(schema, listDepth); 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: case schema::Type::LIST:
KJ_UNREACHABLE; KJ_UNREACHABLE;
...@@ -911,7 +923,7 @@ uint Type::hashCode() const { ...@@ -911,7 +923,7 @@ uint Type::hashCode() const {
// both branches compile to the same instructions and can optimize it away. // both branches compile to the same instructions and can optimize it away.
uint16_t val = scopeId != 0 || isImplicitParam ? uint16_t val = scopeId != 0 || isImplicitParam ?
paramIndex : static_cast<uint16_t>(anyPointerKind); paramIndex : static_cast<uint16_t>(anyPointerKind);
return kj::hashCode(val, isImplicitParam, scopeId); return kj::hashCode(val, isImplicitParam, scopeId, listDepth);
} }
} }
......
...@@ -123,7 +123,7 @@ private: ...@@ -123,7 +123,7 @@ private:
return e.key == key; return e.key == key;
} }
template <typename KeyLike> template <typename KeyLike>
inline bool hashCode(KeyLike&& key) const { inline auto hashCode(KeyLike&& key) const {
return kj::hashCode(key); return kj::hashCode(key);
} }
}; };
...@@ -252,7 +252,7 @@ public: ...@@ -252,7 +252,7 @@ public:
template <typename T, typename U> template <typename T, typename U>
inline bool matches(T& a, U& b) const { return a == b; } inline bool matches(T& a, U& b) const { return a == b; }
template <typename KeyLike> template <typename KeyLike>
inline bool hashCode(KeyLike&& key) const { inline auto hashCode(KeyLike&& key) const {
return kj::hashCode(key); return kj::hashCode(key);
} }
}; };
......
...@@ -161,18 +161,32 @@ kj::Array<HashBucket> rehash(kj::ArrayPtr<const HashBucket> oldBuckets, size_t t ...@@ -161,18 +161,32 @@ kj::Array<HashBucket> rehash(kj::ArrayPtr<const HashBucket> oldBuckets, size_t t
auto newBuckets = kj::heapArray<HashBucket>(size); auto newBuckets = kj::heapArray<HashBucket>(size);
memset(newBuckets.begin(), 0, sizeof(HashBucket) * size); memset(newBuckets.begin(), 0, sizeof(HashBucket) * size);
uint entryCount = 0;
uint collisionCount = 0;
for (auto& oldBucket: oldBuckets) { for (auto& oldBucket: oldBuckets) {
if (oldBucket.isOccupied()) { if (oldBucket.isOccupied()) {
++entryCount;
for (uint i = oldBucket.hash % newBuckets.size();; i = probeHash(newBuckets, i)) { for (uint i = oldBucket.hash % newBuckets.size();; i = probeHash(newBuckets, i)) {
auto& newBucket = newBuckets[i]; auto& newBucket = newBuckets[i];
if (newBucket.isEmpty()) { if (newBucket.isEmpty()) {
newBucket = oldBucket; newBucket = oldBucket;
break; 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; return newBuckets;
} }
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment