Skip rb_eql() for identical elements in Array#eql?

Array#== walks both element buffers and only calls rb_equal() for a
pair whose VALUEs differ. Array#eql? called rb_ary_elt() twice and
rb_eql() once for every element, even though rb_eql() answers true
for identical VALUEs before doing anything else. For arrays of
Fixnums, Symbols, and nil, which are identical whenever they are
equal, that made eql? several times slower than ==.

recursive_eql() now walks the buffers the way recursive_equal() does.
A pair that is not identical still goes through rb_eql(), which can
run user code that resizes either array, so the length and both
buffers are re-read after every such call. Arrays whose lengths came
apart compare unequal, as they do under ==. Before, the shorter one
read as nil past its end, so the result depended on the remaining
elements of the longer one.

The new tests cover an element whose eql? empties both arrays,
shortens one of them, or grows both past their original capacity
mid-compare.

array.c

@@ -5538,12 +5538,24 @@ rb_ary_equal(VALUE ary1, VALUE ary2)
 static VALUE
 recursive_eql(VALUE ary1, VALUE ary2, int recur)
 {
-    rb_len_t i;
+    rb_len_t i, len1;
+    const VALUE *p1, *p2;
 
     if (recur) return Qtrue; /* Subtle! */
-    for (i=0; i<RARRAY_LEN(ary1); i++) {
-        if (!rb_eql(rb_ary_elt(ary1, i), rb_ary_elt(ary2, i)))
-            return Qfalse;
+
+    /* rb_eql() can resize either array and evacuate ptrs */
+    p1 = RARRAY_CONST_PTR(ary1);
+    p2 = RARRAY_CONST_PTR(ary2);
+    len1 = RARRAY_LEN(ary1);
+
+    for (i = 0; i < len1; i++) {
+        if (p1[i] != p2[i]) {
+            if (!rb_eql(p1[i], p2[i])) return Qfalse;
+            len1 = RARRAY_LEN(ary1);
+            if (len1 != RARRAY_LEN(ary2)) return Qfalse;
+            p1 = RARRAY_CONST_PTR(ary1);
+            p2 = RARRAY_CONST_PTR(ary2);
+        }
     }
     return Qtrue;
 }

benchmark/array_eql.yml

@@ -0,0 +1,13 @@
+prelude: |
+  small_ary1 = [1, :a, nil, 2]
+  small_ary2 = [1, :a, nil, 2]
+  large_ary1 = Array.new(1000) { |i| i }
+  large_ary2 = Array.new(1000) { |i| i }
+  str_ary1 = Array.new(100) { |i| i.to_s }
+  str_ary2 = Array.new(100) { |i| i.to_s }
+
+benchmark:
+  small_ary.eql?: small_ary1.eql?(small_ary2)
+  large_ary.==: large_ary1 == large_ary2
+  large_ary.eql?: large_ary1.eql?(large_ary2)
+  str_ary.eql?: str_ary1.eql?(str_ary2)

test/ruby/test_array.rb

@@ -3254,6 +3254,87 @@ class TestArray < Test::Unit::TestCase
     assert_equal($test_equal_resize_a, $test_equal_resize_b)
   end
 
+  def test_eql_resize
+    a = Array.new(3, &:to_s)
+    b = a.dup
+    o = Object.new
+    o.define_singleton_method(:eql?) do |_|
+      a.clear
+      b.clear
+      true
+    end
+    a[1] = o
+    assert_send([a, :eql?, b])
+
+    a = [0, nil, nil]
+    b = [0, 1, nil]
+    o = Object.new
+    o.define_singleton_method(:eql?) do |_|
+      b.pop
+      true
+    end
+    a[1] = o
+    assert_not_send([a, :eql?, b])
+  end
+
+  def test_eql_grow
+    a = Array.new(3, &:to_s)
+    b = a.dup
+    extra = Array.new(100, &:to_s)
+    o = Object.new
+    o.define_singleton_method(:eql?) do |_|
+      a.concat(extra)
+      b.concat(extra)
+      true
+    end
+    a[1] = o
+    assert_send([a, :eql?, b])
+
+    a = Array.new(3, &:to_s)
+    b = a.dup
+    o = Object.new
+    o.define_singleton_method(:eql?) do |_|
+      a.concat(extra)
+      b.concat(extra.map(&:dup).tap { |e| e[-1] = :different })
+      true
+    end
+    a[1] = o
+    assert_not_send([a, :eql?, b])
+  end
+
+  def test_eql_replace
+    # Unlike resizing small arrays, whose elements are embedded in the
+    # array objects, replacing arrays too long to embed frees the
+    # buffers being compared, so reading them after eql? returns would
+    # read freed memory.
+    ints = ->(n) { Array.new(n) { |i| i } }
+    compare = ->(new_a, new_b) do
+      a = ints[200]
+      b = ints[200]
+      o = Object.new
+      o.define_singleton_method(:eql?) do |_|
+        a.replace(new_a)
+        b.replace(new_b) if new_b
+        true
+      end
+      a[100] = o
+      b[100] = Object.new
+      a.eql?(b)
+    end
+
+    # shorter than the index being compared
+    assert_equal(true, compare[ints[50], ints[50]])
+    # shorter, and equal or different after the index being compared
+    assert_equal(true, compare[ints[150], ints[150]])
+    assert_equal(false, compare[ints[150], ints[150].tap { |e| e[125] = -1 }])
+    # longer, and equal or different beyond the original length
+    assert_equal(true, compare[ints[400], ints[400]])
+    assert_equal(false, compare[ints[400], ints[400].tap { |e| e[300] = -1 }])
+    # only one of them replaced
+    assert_equal(false, compare[ints[150], nil])
+    assert_equal(false, compare[ints[400], nil])
+  end
+
   def test_flatten_error
     a = []
     f = [].freeze