Project

General

Profile

Bug #11130

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

Added by kou (Kouhei Sutou) over 4 years ago. Updated over 4 years ago.

Status:
Closed
Priority:
Normal
Target version:
-
[ruby-dev:48959]

Description

須藤です。

+    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)

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


Related issues

Related to Ruby master - Feature #9118: In Enumerable#to_a, use size to set array capa when possibleClosedActions

Associated revisions

Revision 95f54fb0
Added by glass over 4 years ago

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

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

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@50477 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 50477
Added by Glass_saga (Masaki Matsushita) over 4 years ago

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

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

Revision 50477
Added by glass over 4 years ago

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

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

Revision 50477
Added by glass over 4 years ago

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

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

Revision 50477
Added by glass over 4 years ago

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

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

Revision 50477
Added by glass over 4 years ago

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

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

Revision d77f4934
Added by glass over 4 years ago

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

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@50483 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 50483
Added by Glass_saga (Masaki Matsushita) over 4 years ago

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

Revision 50483
Added by glass over 4 years ago

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

Revision 50483
Added by glass over 4 years ago

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

Revision 50483
Added by glass over 4 years ago

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

Revision 50483
Added by glass over 4 years ago

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

History

Updated by usa (Usaku NAKAMURA) over 4 years ago

  • 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

Updated by usa (Usaku NAKAMURA) over 4 years ago

  • Description updated (diff)

Updated by nobu (Nobuyoshi Nakada) over 4 years ago

  • Description updated (diff)

Updated by matz (Yukihiro Matsumoto) over 4 years ago

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

Matz.

Updated by kou (Kouhei Sutou) over 4 years ago

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

Updated by knu (Akinori MUSHA) over 4 years ago

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

#7

Updated by Anonymous over 4 years ago

  • 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.

Updated by Glass_saga (Masaki Matsushita) over 4 years ago

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

Updated by usa (Usaku NAKAMURA) over 4 years ago

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

Updated by Glass_saga (Masaki Matsushita) over 4 years ago

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

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

Updated by mame (Yusuke Endoh) over 4 years ago

  • Status changed from Closed to Assigned

議論は #9118 にあります。

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

まずは revert に一票です。

--
Yusuke Endoh mame@ruby-lang.org

#12

Updated by usa (Usaku NAKAMURA) over 4 years ago

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

Updated by usa (Usaku NAKAMURA) over 4 years ago

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して、必要ならあちらで議論を再開するのがよさそうですね。

#14

Updated by Anonymous over 4 years ago

  • 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]

Updated by Glass_saga (Masaki Matsushita) over 4 years ago

再入チェックを入れてパフォーマンスを比較した結果、改善が見られなかったので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

Also available in: Atom PDF