Bug #11130: Re: [ruby-changes:38376] glass:r50457 (trunk): * enum.c (enum_to_a): Use size to set array capa when possible.

• Status: Closed
• Assignee: Glass_saga (Masaki Matsushita)
• Target version: -
• ruby -v: r50457
• Backport: 2.0.0: DONTNEED, 2.1: DONTNEED, 2.2: DONTNEED

[kou (Kouhei Sutou)]

須藤です。

+    if (NIL_P(size) || size == Qundef) {
+	ary = rb_ary_new();
+    }
+    else {
+	ary = rb_ary_new_capa(NUM2LONG(size));
+    }

を

    if (FIXNUM_P(size)) {
	ary = rb_ary_new_capa(NUM2LONG(size));
    }
    else {
	ary = rb_ary_new();
    }

とかsizeが返す値が数値じゃなかったらこれまでと同じ挙動にする
ようにしてもらえないでしょうか？

これまでは

class NonIntegerSizeEnum
  include Enumerable

  def initialize(n)
    @n = n
  end

  def each
    @n.times { |i| yield i }
  end

  def size
    :size
  end
end

NonIntegerSizeEnum.new(100).to_a

というコードが動いていたんですが、この変更の後からは

/tmp/b.rb:17:in `to_a': no implicit conversion of Symbol into Integer (TypeError)

というエラーがでるようになってしまって困っています。

[usa (Usaku NAKAMURA)]

• Status changed from Open to Assigned
• Assignee set to Glass_saga (Masaki Matsushita)
• ruby -v set to r50457
• Backport changed from 2.0.0: UNKNOWN, 2.1: UNKNOWN, 2.2: UNKNOWN to 2.0.0: DONTNEED, 2.1: DONTNEED, 2.2: DONTNEED

[usa (Usaku NAKAMURA)]

• Description updated (diff)

[nobu (Nobuyoshi Nakada)]

• Description updated (diff)

[matz (Yukihiro Matsumoto)]

そもそもsizeがnilでも整数でもない値を返すのはどうにもバグっぽいので、新しい挙動でバグが発見されたと考えそうな気がしますが、sizeに手を付けずこの挙動を維持してほしい(後方互換性の維持以外の)理由ってのがあれば教えてください。

Matz.

[kou (Kouhei Sutou)]

実際のケースはgtk3 gemという中のコードで複数の子ウィジェットを持つウィジェットオブジェクトで発生しました。このGTK+のウィジェットではeachで子ウィジェットを繰り返し、sizeでは[width, height]という配列を返していました。GTK+のウィジェットの文脈ではsizeが横幅と縦幅を返すのはおかしいことではないのですが、この場合はEnumerableを使わないほうがよさそうでしょうか。

• 失敗するテスト: https://github.com/ruby-gnome2/ruby-gnome2/blob/master/gtk3/test/test-gtk-container.rb#L42
• 失敗したテストの結果: https://travis-ci.org/ruby-gnome2/ruby-gnome2/jobs/61960137#L1328

[knu (Akinori MUSHA)]

最適化のヒントとして使えるなら使うというのを超えて、ドキュメントなしに互換性を壊してしまったらバグではないでしょうか。
sizeがINFINITYを返すときも、eachを呼ぶことなくRangeErrorが発生します。（そのようなコードが足を撃たんとしている蓋然性は高いですが）

• Status changed from Assigned to Closed

Applied in changeset r50477.

---

• enum.c (enum_to_a): fix incompatibility introduced in r50457.
  [Bug #11130]

• test/ruby/test_enum.rb: test for above.

[Glass_saga (Masaki Matsushita)]

互換性を壊すのは私の本意ではないので、以前と同じ挙動になるよう修正しました。
ご迷惑をおかけしました。

[usa (Usaku NAKAMURA)]

そもそも、本件、githubのpull request以外のどこかで議論はあったのでしょうか？
sizeという、多義性のある(と実際に確認された)メソッドに依拠するのはそもそも危険なのでは、という気がしないでもないのですが、lengthやcountでなくsizeが対象として選ばれた経緯はなんでしょう？

[Glass_saga (Masaki Matsushita)]

Enumerable#countは、実際にイテレーションを回してみて回った数を数える実装となっています。
pull request ( https://github.com/ruby/ruby/pull/444 )の狙いはEnumerable#to_aの高速化なので、countは不向きです。

Enumerator#sizeがFixnumを返す場合に限っては、それに依拠して配列のサイズを決めてしまって問題ないのではないかと思います。

[mame (Yusuke Endoh)]

• Status changed from Closed to Assigned

議論は #9118 にあります。

そこでも指摘していますが、この最適化は def size; to_a.size; end という手抜きな size 実装で無限再帰になります（そういう実装はいくつも実在します）。再入チェックを入れてもなお高速か、確かめる必要があるでしょう。

まずは revert に一票です。

--
Yusuke Endoh [e-mail]

[usa (Usaku NAKAMURA)]

• Related to Feature #9118: In Enumerable#to_a, use size to set array capa when possible added

[usa (Usaku NAKAMURA)]

Masaki Matsushita wrote:

Enumerable#countは、実際にイテレーションを回してみて回った数を数える実装となっています。
pull request ( https://github.com/ruby/ruby/pull/444 )の狙いはEnumerable#to_aの高速化なので、countは不向きです。

Enumerable#countでなく自前の実装があるならそれを使い、そうでないならないものとみなせ、というくらいの意図でした。

Enumerator#sizeがFixnumを返す場合に限っては、それに依拠して配列のサイズを決めてしまって問題ないのではないかと思います。

自前のコードを漁ってみたら、大昔に書いたRSSパーサの中のクラスがEnumerableをincludeしてsizeを別の意味で定義してたのですが、そいつは整数を返してました。
(RSSのノードを意味するクラスで、eachは子ノードを回すが、sizeは自分を文字列化したときの長さを返していた。)

Yusuke Endoh wrote:

議論は #9118 にあります。

おお、ありがとうございます。ははは、同じ内容だ。

まずは revert に一票です。

あちらでも結論が出てないですし、いったんrevertして、必要ならあちらで議論を再開するのがよさそうですね。

• Status changed from Assigned to Closed

Applied in changeset r50483.

---

• enum.c (enum_to_a): revert r50457.
  it requires recursion check.
  then, it doesn't make performance improvement.
  [Bug #11130] [Feature #9118]

[Glass_saga (Masaki Matsushita)]

再入チェックを入れてパフォーマンスを比較した結果、改善が見られなかったのでrevertしました。

再入チェック付の最適化:
ruby-dev bm_enum_to_a_sized.rb 19.46s user 1.29s system 99% cpu 20.760 total
ruby-dev bm_enum_to_a_sized.rb 19.46s user 1.29s system 99% cpu 20.754 total
ruby-dev bm_enum_to_a_sized.rb 19.68s user 1.31s system 99% cpu 21.006 total

trunk:
ruby-dev bm_enum_to_a_sized.rb 19.93s user 1.30s system 99% cpu 21.240 total
ruby-dev bm_enum_to_a_sized.rb 19.88s user 1.30s system 99% cpu 21.189 total
ruby-dev bm_enum_to_a_sized.rb 20.81s user 1.48s system 99% cpu 22.322 total