python · jdemeyer · Sep 20, 2018
diff --git a/Include/pyhash.h b/Include/pyhash.h
@@ -12,8 +12,15 @@ PyAPI_FUNC(Py_hash_t) _Py_HashPointer(void*);
 PyAPI_FUNC(Py_hash_t) _Py_HashBytes(const void*, Py_ssize_t);
 #endif
 
-/* Prime multiplier used in string and various other hashes. */
-#define _PyHASH_MULTIPLIER 1000003UL  /* 0xf4243 */
+/* Multipliers used for various hashes.
+ * These should be large odd numbers. */
+#define _PyHASH_MULTIPLIER ((Py_uhash_t)1000003)
+#if SIZEOF_SIZE_T <= 4
+/* 3**41 truncated to platform bitsize */
+#define _PyHASH_MULTIPLIER2 ((Py_uhash_t)2069870691)
+#else
+#define _PyHASH_MULTIPLIER2 ((Py_uhash_t)18026252303461234787)
+#endif
 
 /* Parameters used for the numeric hash implementation.  See notes for
    _Py_HashDouble in Python/pyhash.c.  Numeric hashes are based on

diff --git a/Lib/test/test_tuple.py b/Lib/test/test_tuple.py
@@ -63,28 +63,56 @@ def f():
         self.assertEqual(list(tuple(f())), list(range(1000)))
 
     def test_hash(self):
-        # See SF bug 942952:  Weakness in tuple hash
-        # The hash should:
-        #      be non-commutative
-        #      should spread-out closely spaced values
-        #      should not exhibit cancellation in tuples like (x,(x,y))
-        #      should be distinct from element hashes:  hash(x)!=hash((x,))
-        # This test exercises those cases.
-        # For a pure random hash and N=50, the expected number of occupied
-        #      buckets when tossing 252,600 balls into 2**32 buckets
-        #      is 252,592.6, or about 7.4 expected collisions.  The
-        #      standard deviation is 2.73.  On a box with 64-bit hash
-        #      codes, no collisions are expected.  Here we accept no
-        #      more than 15 collisions.  Any worse and the hash function
-        #      is sorely suspect.
-
-        N=50
-        base = list(range(N))
-        xp = [(i, j) for i in base for j in base]
-        inps = base + [(i, j) for i in base for j in xp] + \
-                     [(i, j) for i in xp for j in base] + xp + list(zip(base))
-        collisions = len(inps) - len(set(map(hash, inps)))
-        self.assertTrue(collisions <= 15)
+        # We check for hash collisions between small integers, tuples
+        # of such integers and nested tuples of such integers.
+        #
+        # Earlier versions of the tuple hash algorithm had collisions
+        # reported at:
+        # - https://bugs.python.org/issue942952
+        # - https://bugs.python.org/issue34751
+        #
+        # For a pure random 32-bit hash and N = 345,130 test items, the
+        # expected number of collisions equals
+        #
+        # 2**(-32) * N(N-1)/2 = 13.9
+        #
+        # We allow up to 20 collisions, which suffices to make the test
+        # pass with 95% confidence. (Actually, due to the structure in
+        # the testsuite input, collisions are not independent. So this
+        # statistic is not really correct.)
+        #
+        # Also note that the hash of tuples is deterministic. So, if the
+        # test passes once on a given system, it will always pass.
+
+        # All numbers in the interval [-n, ..., n] except -1 because
+        # hash(-1) == hash(-2).
+        n = 5
+        A = [x for x in range(-n, n+1) if x != -1]
+
+        B = A + [(a,) for a in A]
+
+        L2 = [(a,b) for a in A for b in A]
+        L3 = L2 + [(a,b,c) for a in A for b in A for c in A]
+        L4 = L3 + [(a,b,c,d) for a in A for b in A for c in A for d in A]
+
+        # T = list of testcases. These consist of all (possibly nested
+        # at most 2 levels deep) tuples containing at most 4 items from
+        # the set A.
+        T = A
+        T += [(a,) for a in B + L4]
+        T += [(a,b) for a in L3 for b in B]
+        T += [(a,b) for a in L2 for b in L2]
+        T += [(a,b) for a in B for b in L3]
+        T += [(a,b,c) for a in B for b in B for c in L2]
+        T += [(a,b,c) for a in B for b in L2 for c in B]
+        T += [(a,b,c) for a in L2 for b in B for c in B]
+        T += [(a,b,c,d) for a in B for b in B for c in B for d in B]
+        self.assertEqual(len(T), 345130)
+
+        # Limit the hash to 32 bits to have a good test on 64-bit systems
+        hashes = [hash(x) % 2**32 for x in T]
+        collisions = len(T) - len(set(hashes))
+        self.assertLessEqual(collisions, 20)
 
     def test_repr(self):
         l0 = tuple()

diff --git a/Misc/NEWS.d/next/Core and Builtins/2018-09-20-15-41-58.bpo-34751.Yiv0pV.rst b/Misc/NEWS.d/next/Core and Builtins/2018-09-20-15-41-58.bpo-34751.Yiv0pV.rst
@@ -0,0 +1 @@
+Improved hash function for tuples to make collisions less likely.
diff --git a/Objects/tupleobject.c b/Objects/tupleobject.c
@@ -333,38 +333,56 @@ tuplerepr(PyTupleObject *v)
     return NULL;
 }
 
-/* The addend 82520, was selected from the range(0, 1000000) for
-   generating the greatest number of prime multipliers for tuples
-   up to length eight:
 
-     1082527, 1165049, 1082531, 1165057, 1247581, 1330103, 1082533,
-     1330111, 1412633, 1165069, 1247599, 1495177, 1577699
+/* Hash for tuples. The structure is that of a standard FNV-1a hash.
+   We are not using the usual FNV multiplier: testing has shown that
+   the many 0 bits tend to create sporadic collisions.
+
+   We compute the hash with unsigned variables for well-defined
+   overflow behavior.
 
    Tests have shown that it's not worth to cache the hash value, see
-   issue #9685.
+   https://bugs.python.org/issue9685
 */
-
 static Py_hash_t
 tuplehash(PyTupleObject *v)
 {
-    Py_uhash_t x;  /* Unsigned for defined overflow behavior. */
-    Py_hash_t y;
-    Py_ssize_t len = Py_SIZE(v);
-    PyObject **p;
-    Py_uhash_t mult = _PyHASH_MULTIPLIER;
-    x = 0x345678UL;
-    p = v->ob_item;
-    while (--len >= 0) {
-        y = PyObject_Hash(*p++);
-        if (y == -1)
+    /* Arbitrary starting value, any non-zero value would work.
+       The starting value is meant to:
+       - provide a class signature: this value should be specific to
+         tuples to make hashes of tuples different from other classes.
+       - encode the length of the tuple: thanks to the non-zero
+         starting value and a good choice of multiplier, the hashes of
+         all-zero tuples (0,) *  N are all different.
+    */
+    Py_uhash_t x = 3430008;
+    Py_uhash_t MUL = _PyHASH_MULTIPLIER2, ERR = -1;
+    Py_ssize_t i, len = Py_SIZE(v);
+    PyObject **item = v->ob_item;
+    for (i = 0; i < len; i++) {
+        Py_uhash_t y = PyObject_Hash(item[i]);
+        if (y == ERR) {
             return -1;
-        x = (x ^ y) * mult;
-        /* the cast might truncate len; that doesn't change hash stability */
-        mult += (Py_hash_t)(82520UL + len + len);
+        }
+        /* We modify the hash of the items slightly to solve two problems:
+           - it fixes hash collisions for nested tuples like (a, (b, c))
+           - it works around a weakness of the FNV hash algorithm due to
+             x ^ -2 == -x for odd x.
+             See https://bugs.python.org/issue34751 for details.
+           Note that this operation is a permutation, so it cannot introduce
+           additional collisions. */
+        y ^= 2 * y;
+
+        /* This is the FNV-1a hash operation */
+        x = (x ^ y) * MUL;
+    }
+    /* This final addition is only kept for historical reasons, it does
+       not serve any known purpose. */
+    x += 97531;
+
+    if (x == ERR) {
+        return -2;
     }
-    x += 97531UL;
-    if (x == (Py_uhash_t)-1)
-        x = -2;
     return x;
 }
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		Improved hash function for tuples to make collisions less likely.