[GR-15869] Hash code improvement.

PullRequest: truffleruby/842

Author: aardvark179

CHANGELOG.md

@@ -17,6 +17,10 @@ Compatibility
 * `String#-@` now performs string deduplication (#1608).
 * `Hash#merge` now preserves the key order from the original hash for merged values (#1650).
 
+Changes:
+
+* Hash code calculation has been improved to reduce hash collisions for `Hash` and other cases.
+
 # 1.0 RC 17
 # 19.0.0, May 2019
 

spec/ruby/core/hash/hash_spec.rb

@@ -11,6 +11,14 @@
     { 0=>2, 11=>1 }.hash.should == { 11=>1, 0=>2 }.hash
   end
 
+  it "returns a value in which element values do not cancel each other out" do
+    { a: 2, b: 2 }.hash.should_not == { a: 7, b: 7 }.hash
+  end
+
+  it "returns a value in which element keys and values do not cancel each other out" do
+    { :a => :a }.hash.should_not == { :b => :b }.hash
+  end
+
   it "generates a hash for recursive hash structures" do
     h = {}
     h[:a] = h

src/main/java/org/truffleruby/core/VMPrimitiveNodes.java

@@ -67,6 +67,7 @@
 import org.truffleruby.language.control.ExitException;
 import org.truffleruby.language.control.RaiseException;
 import org.truffleruby.language.control.ThrowException;
+import org.truffleruby.language.dispatch.CallDispatchHeadNode;
 import org.truffleruby.language.methods.InternalMethod;
 import org.truffleruby.language.methods.LookupMethodNode;
 import org.truffleruby.language.methods.LookupMethodNodeGen;
@@ -444,4 +445,79 @@ public DynamicObject negativeCount(int count) {
 
     }
 
+    @Primitive(name = "vm_hash_start", needsSelf = false)
+    public abstract static class VMHashStart extends PrimitiveArrayArgumentsNode {
+
+        @Specialization
+        public long startHash(long salt) {
+            return getContext().getHashing(this).start(salt);
+        }
+
+        @Specialization(guards = "isRubyBignum(salt)")
+        public long startHashBigNum(DynamicObject salt) {
+            return getContext().getHashing(this).start(Layouts.BIGNUM.getValue(salt).hashCode());
+        }
+
+        @Specialization(guards = "!isRubyNumber(salt)")
+        public Object startHashNotNumber(Object salt,
+                                 @Cached("createPrivate()") CallDispatchHeadNode coerceToIntNode,
+                                 @Cached("createBinaryProfile()") ConditionProfile isIntegerProfile,
+                                 @Cached("createBinaryProfile()") ConditionProfile isLongProfile,
+                                 @Cached("createBinaryProfile()") ConditionProfile isBignumProfile) {
+            Object result = coerceToIntNode.call(coreLibrary().getTruffleTypeModule(), "coerce_to_int", salt);
+            if (isIntegerProfile.profile(result instanceof Integer)) {
+                return getContext().getHashing(this).start((int) result);
+            } else if (isLongProfile.profile(result instanceof Long)) {
+                return getContext().getHashing(this).start((long) result);
+            } else if (isBignumProfile.profile(Layouts.BIGNUM.isBignum(result))) {
+                return getContext().getHashing(this).start(Layouts.BIGNUM.getValue((DynamicObject) result).hashCode());
+            } else {
+                throw new UnsupportedOperationException();
+            }
+
+        }
+    }
+
+    @Primitive(name = "vm_hash_update", needsSelf = false)
+    public abstract static class VMHashUpdate extends PrimitiveArrayArgumentsNode {
+
+        @Specialization
+        public long updateHash(long hash, long value) {
+            return Hashing.update(hash, value);
+        }
+
+        @Specialization(guards = "isRubyBignum(value)")
+        public long updateHash(long hash, DynamicObject value) {
+            return Hashing.update(hash, Layouts.BIGNUM.getValue(value).hashCode());
+        }
+
+
+        @Specialization(guards = "!isRubyNumber(value)")
+        public Object updateHash(long hash, Object value,
+                                 @Cached("createPrivate()") CallDispatchHeadNode coerceToIntNode,
+                                 @Cached("createBinaryProfile()") ConditionProfile isIntegerProfile,
+                                 @Cached("createBinaryProfile()") ConditionProfile isLongProfile,
+                                 @Cached("createBinaryProfile()") ConditionProfile isBignumProfile) {
+            Object result = coerceToIntNode.call(coreLibrary().getTruffleTypeModule(), "coerce_to_int", value);
+            if (isIntegerProfile.profile(result instanceof Integer)) {
+                return Hashing.update(hash, (int) result);
+            } else if (isLongProfile.profile(result instanceof Long)) {
+                return Hashing.update(hash, (long) result);
+            } else if (isBignumProfile.profile(Layouts.BIGNUM.isBignum(result))) {
+                return Hashing.update(hash, Layouts.BIGNUM.getValue((DynamicObject) result).hashCode());
+            } else {
+                throw new UnsupportedOperationException();
+            }
+
+        }
+    }
+
+    @Primitive(name = "vm_hash_end", needsSelf = false)
+    public abstract static class VMHashEnd extends PrimitiveArrayArgumentsNode {
+
+        @Specialization
+        public long endHash(long hash) {
+            return Hashing.end(hash);
+        }
+    }
 }

src/main/java/org/truffleruby/core/numeric/IntegerNodes.java

@@ -1467,24 +1467,6 @@ public DynamicObject toS(DynamicObject value, int base) {
 
     }
 
-    @Primitive(name = "integer_memhash")
-    public static abstract class IntegerMemhashNode extends PrimitiveArrayArgumentsNode {
-
-        private static final int CLASS_SALT = 94974697; // random number, stops hashes for similar
-                                                        // values but different classes being the
-                                                        // same, static because we want
-                                                        // deterministic hashes
-
-        @Specialization
-        public long memhashLongLong(long a, long b) {
-            long h = getContext().getHashing(this).start(CLASS_SALT);
-            h = Hashing.update(h, a);
-            h = Hashing.update(h, b);
-            return Hashing.end(h);
-        }
-
-    }
-
     @Primitive(name = "integer_fits_into_int")
     public static abstract class FixnumFitsIntoIntNode extends PrimitiveArrayArgumentsNode {
 

src/main/ruby/core/complex.rb

@@ -334,8 +334,19 @@ def to_s
     result
   end
 
+  # Random number for hash codes. Stops hashes for similar values in
+  # different classes from clashing, but defined as a constant so
+  # that hashes will be deterministic.
+
+  CLASS_SALT = 0x37f7c8ee
+
+  private_constant :CLASS_SALT
+
   def hash
-    Truffle.invoke_primitive :integer_memhash, @real.hash, @imag.hash
+    val = Truffle.invoke_primitive :vm_hash_start, CLASS_SALT
+    val = Truffle.invoke_primitive :vm_hash_update, val, @real.hash
+    val = Truffle.invoke_primitive :vm_hash_update, val, @imag.hash
+    Truffle.invoke_primitive :vm_hash_end, val
   end
 
   def inspect

src/main/ruby/core/hash.rb

@@ -345,16 +345,27 @@ def to_h
     end
   end
 
+  # Random number for hash codes. Stops hashes for similar values in
+  # different classes from clashing, but defined as a constant so
+  # that hashes will be deterministic.
+
+  CLASS_SALT = 0xC5F7D8
+
+  private_constant :CLASS_SALT
+
   def hash
-    val = size
+    val = Truffle.invoke_primitive :vm_hash_start, CLASS_SALT
+    val = Truffle.invoke_primitive :vm_hash_update, val, size
     Thread.detect_outermost_recursion self do
       each_pair do |key,value|
-        val ^= key.hash
-        val ^= value.hash
+        entry_val = Truffle.invoke_primitive :vm_hash_start, key.hash
+        entry_val = Truffle.invoke_primitive :vm_hash_update, entry_val, value.hash
+        # We have to combine these with xor as the hash must not depend on hash order.
+        val ^= Truffle.invoke_primitive :vm_hash_end, entry_val
       end
     end
 
-    val
+    Truffle.invoke_primitive :vm_hash_end, val
   end
 
   def delete_if(&block)

src/main/ruby/core/range.rb

@@ -182,14 +182,20 @@ def first(n=undefined)
     super
   end
 
+  # Random number for hash codes. Stops hashes for similar values in
+  # different classes from clashing, but defined as a constant so
+  # that hashes will be deterministic.
+
+  CLASS_SALT = 0xb36df84d
+
+  private_constant :CLASS_SALT
+
   def hash
-    excl = exclude_end? ? 1 : 0
-    hash = excl
-    hash ^= self.begin.hash << 1
-    hash ^= self.end.hash << 9
-    hash ^= excl << 24;
-    # Are we throwing away too much here for a good hash value distribution?
-    hash & Truffle::Platform::LONG_MAX
+    val = Truffle.invoke_primitive(:vm_hash_start, CLASS_SALT)
+    val = Truffle.invoke_primitive(:vm_hash_update, val, exclude_end? ? 1 : 0)
+    val = Truffle.invoke_primitive(:vm_hash_update, val, self.begin.hash)
+    val = Truffle.invoke_primitive(:vm_hash_update, val, self.end.hash)
+    Truffle.invoke_primitive(:vm_hash_end, val)
   end
 
   def include?(value)

src/main/ruby/core/rational.rb

@@ -206,8 +206,19 @@ def floor(precision = 0)
     end
   end
 
+  # Random number for hash codes. Stops hashes for similar values in
+  # different classes from clashing, but defined as a constant so
+  # that hashes will be deterministic.
+
+  CLASS_SALT = 0x3c654ce9
+
+  private_constant :CLASS_SALT
+
   def hash
-    @numerator.hash ^ @denominator.hash
+    val = Truffle.invoke_primitive :vm_hash_start, CLASS_SALT
+    val = Truffle.invoke_primitive :vm_hash_update, val, @numerator.hash
+    val = Truffle.invoke_primitive :vm_hash_update, val, @denominator.hash
+    Truffle.invoke_primitive :vm_hash_end, val
   end
 
   def inspect

src/main/ruby/core/struct.rb

@@ -266,12 +266,21 @@ def each_pair
     self
   end
 
+  # Random number for hash codes. Stops hashes for similar values in
+  # different classes from clashing, but defined as a constant so
+  # that hashes will be deterministic.
+
+  CLASS_SALT = 0xa1982d79
+
+  private_constant :CLASS_SALT
+
   def hash
-    hash_val = size
-    return _attrs.size if Thread.detect_outermost_recursion self do
-      _attrs.each { |var| hash_val ^= instance_variable_get(:"@#{var}").hash }
+    val = Truffle.invoke_primitive(:vm_hash_start, CLASS_SALT)
+    val = Truffle.invoke_primitive(:vm_hash_update, val, size)
+    return val if Thread.detect_outermost_recursion self do
+      _attrs.each { |var| Truffle.invoke_primitive(:vm_hash_update, val, instance_variable_get(:"@#{var}").hash) }
     end
-    hash_val
+    Truffle.invoke_primitive(:vm_hash_end, val)
   end
 
   def length
@@ -355,20 +364,25 @@ def self._specialize(attrs)
       hashes << "#{vars[-1]}.hash"
     end
 
+    hash_calculation = hashes.map do |calc|
+      "hash = Truffle.invoke_primitive :vm_hash_update, hash, #{calc}"
+    end.join("\n")
+
     code = <<-CODE
       def initialize(#{args.join(", ")})
         #{assigns.join(';')}
         self
       end
 
       def hash
-        hash = #{hashes.size}
+        hash = Truffle.invoke_primitive :vm_hash_start, CLASS_SALT
+        hash = Truffle.invoke_primitive :vm_hash_update, hash, #{hashes.size}
 
         return hash if Thread.detect_outermost_recursion(self) do
-          hash = hash ^ #{hashes.join(' ^ ')}
+          #{hash_calculation}
         end
 
-        hash
+        Truffle.invoke_primitive :vm_hash_end, hash
       end
 
       def to_a

src/main/ruby/core/time.rb

@@ -112,8 +112,19 @@ def zone
     end
   end
 
+  # Random number for hash codes. Stops hashes for similar values in
+  # different classes from clashing, but defined as a constant so
+  # that hashes will be deterministic.
+
+  CLASS_SALT = 0xf39684d6
+
+  private_constant :CLASS_SALT
+
   def hash
-    tv_sec ^ tv_nsec
+    val = Truffle.invoke_primitive :vm_hash_start, CLASS_SALT
+    val = Truffle.invoke_primitive :vm_hash_update, val, tv_sec
+    val = Truffle.invoke_primitive :vm_hash_update, val, tv_nsec
+    Truffle.invoke_primitive :vm_hash_end, val
   end
 
   def eql?(other)