Fix catastrophic hash collisions in FreqTable::make_hash and vecHasher

barry.hpp

@@ -31,7 +31,7 @@
 /* Versioning */
 #define BARRY_VERSION_MAYOR 0
 #define BARRY_VERSION_MINOR 2
-#define BARRY_VERSION_PATCH 1
+#define BARRY_VERSION_PATCH 2
 #define BARRY_VERSION BARRY_VERSION_MAYOR ## . ## BARRY_VERSION_MINOR ## . ## BARRY_VERSION_PATCH
 
 static const int barry_version_major = BARRY_VERSION_MAYOR;
@@ -396,19 +396,40 @@ struct vecHasher
 
     std::size_t operator()(std::vector< T > const&  dat) const noexcept
     {
-
+
+        // Well-defined for empty input (the loop below reads dat[0u]).
+        if (dat.empty())
+            return 0u;
+
         std::hash< T > hasher;
-        std::size_t hash = hasher(dat[0u]);
-
+
+        // Finalizer from splitmix64: std::hash of arithmetic types is
+        // usually the identity (or a bit-cast for doubles, where small
+        // integers leave the low ~50 bits zero). Without this avalanche
+        // step the low bits of the combined hash carry almost no entropy,
+        // and unordered containers that mask the hash with a power-of-two
+        // bucket count put (nearly) all keys in a single bucket, turning
+        // lookups into O(n). The mixing runs in an explicit 64-bit
+        // accumulator so the avalanche is identical where size_t is 32-bit
+        // (the constants and shifts are 64-bit).
+        auto mix64 = [](std::uint64_t z) -> std::uint64_t {
+            z += 0x9e3779b97f4a7c15ULL;
+            z = (z ^ (z >> 30)) * 0xbf58476d1ce4e5b9ULL;
+            z = (z ^ (z >> 27)) * 0x94d049bb133111ebULL;
+            return z ^ (z >> 31);
+        };
+
+        std::uint64_t hash = mix64(hasher(dat[0u]));
+
         // ^ makes bitwise XOR
         // 0x9e3779b9 is a 32 bit constant (comes from the golden ratio)
         // << is a shift operator, something like lhs * 2^(rhs)
         if (dat.size() > 1u)
             for (size_t i = 1u; i < dat.size(); ++i)
-                hash ^= hasher(dat[i]) + 0x9e3779b9 + (hash<<6) + (hash>>2);
-        
-        return hash;
-        
+                hash ^= mix64(hasher(dat[i])) + 0x9e3779b9 + (hash<<6) + (hash>>2);
+
+        return static_cast< std::size_t >(hash);
+
     }
 
 };
@@ -668,7 +689,7 @@ inline double vec_inner_prod(
  * - term k
  * 
  */
-template<typename T = double> 
+template<typename T = double>
 class FreqTable {
 private:
 
@@ -677,8 +698,56 @@ class FreqTable {
     size_t k = 0u;
     size_t n = 0u;
 
+    /**
+     * Rows whose hash collided with a row already in `index`
+     * (same 64-bit hash, different statistics). Keyed by the shared
+     * hash, holding the positions of the extra rows in `data`.
+     * Empty unless a true hash collision occurs, so it adds no
+     * per-row overhead in the common case.
+     */
+    std::unordered_map<size_t, std::vector<size_t>> collided;
+
     typename std::unordered_map<size_t, size_t>::iterator iter;
-
+
+    /**
+     * Whether the row stored at `pos` (position of its weight in
+     * `data`) holds exactly the statistics in `x`.
+     */
+    bool row_matches(size_t pos, const std::vector< T > & x) const
+    {
+        for (size_t j = 0u; j < k; ++j)
+            if (data[pos + 1u + j] != x[j])
+                return false;
+        return true;
+    }
+
+    /**
+     * Fallback for a true hash collision: the row at `pos` shares the
+     * hash `h` with `x` but holds different statistics. Looks `x` up in
+     * (or appends it to) the per-hash overflow chain.
+     */
+    size_t add_collided(size_t h, const std::vector< T > & x)
+    {
+
+        auto & chain = collided[h];
+        for (auto p : chain)
+        {
+            if (row_matches(p, x))
+            {
+                data[p] += 1.0;
+                return h;
+            }
+        }
+
+        chain.push_back(data.size());
+        data.push_back(1.0);
+        data.insert(data.end(), x.begin(), x.end());
+        n++;
+
+        return h;
+
+    }
+
 public:
     // size_t ncols;
     FreqTable() {};
@@ -739,16 +808,23 @@ inline size_t FreqTable<T>::add(
 
         if (x.size() != k)
             throw std::length_error(
-                "The value you are trying to add doesn't have the same lenght used in the database."
+                "The value you are trying to add doesn't have the same length used in the database."
                 );
-        
+
         #if __cplusplus > 201700L
         auto iter2 = index.try_emplace(h, data.size());
-        
+
         if (!iter2.second)
         {
-
-            data[(iter2.first)->second] += 1.0;
+
+            // The hash existed: make sure the row it points to actually
+            // holds the same statistics before counting it. Otherwise it
+            // is a hash collision and the row goes to the overflow chain.
+            size_t pos = (iter2.first)->second;
+            if (row_matches(pos, x))
+                data[pos] += 1.0;
+            else
+                return add_collided(h, x);
 
         }
         else
@@ -762,25 +838,31 @@ inline size_t FreqTable<T>::add(
 
         if (iter == index.end())
         {
-        
+
 
             index.insert({h, data.size()});
             data.push_back(1.0);
             data.insert(data.end(), x.begin(), x.end());
 
             n++;
-            
+
             return h;
 
         }
 
-        data[(*iter).second] += 1.0;
-
+        // The hash existed: make sure the row it points to actually
+        // holds the same statistics before counting it. Otherwise it
+        // is a hash collision and the row goes to the overflow chain.
+        if (row_matches((*iter).second, x))
+            data[(*iter).second] += 1.0;
+        else
+            return add_collided(h, x);
+
         #endif
-        
+
 
     }
-    
+
     return h;
 
 }
@@ -819,6 +901,7 @@ inline void FreqTable<T>::clear()
 {
 
     index.clear();
+    collided.clear();
     data.clear();
 
     n = 0u;
@@ -883,25 +966,47 @@ template<typename T>
 inline size_t FreqTable<T>::size() const noexcept
 {
 
-    return index.size();
+    // Not index.size(): rows stored in the collision overflow chain
+    // have no entry of their own in -index-.
+    return n;
 
 }
 
 template<typename T>
 inline size_t FreqTable<T>::make_hash(const std::vector< T > & x) const
 {
 
+    // Well-defined for empty input (the loop below reads x[0u]).
+    if (x.empty())
+        return 0u;
+
     std::hash< T > hasher;
-    std::size_t hash = hasher(x[0u]);
-
+
+    // Finalizer from splitmix64: std::hash of arithmetic types is usually
+    // the identity (or a bit-cast for doubles, where small integers leave
+    // the low ~50 bits zero). Without this avalanche step, the low bits of
+    // the combined hash carry almost no entropy, and unordered containers
+    // that mask the hash with a power-of-two bucket count put (nearly) all
+    // keys in a single bucket, turning lookups into O(n). The mixing runs
+    // in an explicit 64-bit accumulator so the avalanche is identical where
+    // size_t is 32-bit (the constants and shifts are 64-bit).
+    auto mix64 = [](std::uint64_t z) -> std::uint64_t {
+        z += 0x9e3779b97f4a7c15ULL;
+        z = (z ^ (z >> 30)) * 0xbf58476d1ce4e5b9ULL;
+        z = (z ^ (z >> 27)) * 0x94d049bb133111ebULL;
+        return z ^ (z >> 31);
+    };
+
+    std::uint64_t hash = mix64(hasher(x[0u]));
+
     // ^ makes bitwise XOR
     // 0x9e3779b9 is a 32 bit constant (comes from the golden ratio)
     // << is a shift operator, something like lhs * 2^(rhs)
     if (x.size() > 1u)
         for (size_t i = 1u; i < x.size(); ++i)
-            hash ^= hasher(x[i]) + 0x9e3779b9 + (hash<<6) + (hash>>2);
-    
-    return hash;
+            hash ^= mix64(hasher(x[i])) + 0x9e3779b9 + (hash<<6) + (hash>>2);
+
+    return static_cast< size_t >(hash);
 
 }