Bug #11130 closed
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 9 years ago.
Updated over 9 years ago.
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)
というエラーがでるようになってしまって困っています。
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
Description updated (diff )
Description updated (diff )
そもそもsizeがnilでも整数でもない値を返すのはどうにもバグっぽいので、新しい挙動でバグが発見されたと考えそうな気がしますが、sizeに手を付けずこの挙動を維持してほしい(後方互換性の維持以外の)理由ってのがあれば教えてください。
Matz.
実際のケースはgtk3 gemという中のコードで複数の子ウィジェットを持つウィジェットオブジェクトで発生しました。このGTK+のウィジェットではeachで子ウィジェットを繰り返し、sizeでは[width, height]という配列を返していました。GTK+のウィジェットの文脈ではsizeが横幅と縦幅を返すのはおかしいことではないのですが、この場合はEnumerableを使わないほうがよさそうでしょうか。
最適化のヒントとして使えるなら使うというのを超えて、ドキュメントなしに互換性を壊してしまったらバグではないでしょうか。
sizeがINFINITYを返すときも、eachを呼ぶことなくRangeErrorが発生します。(そのようなコードが足を撃たんとしている蓋然性は高いですが)
Status changed from Assigned to Closed
Applied in changeset r50477.
互換性を壊すのは私の本意ではないので、以前と同じ挙動になるよう修正しました。
ご迷惑をおかけしました。
そもそも、本件、githubのpull request以外のどこかで議論はあったのでしょうか?
sizeという、多義性のある(と実際に確認された)メソッドに依拠するのはそもそも危険なのでは、という気がしないでもないのですが、lengthやcountでなくsizeが対象として選ばれた経緯はなんでしょう?
Enumerable#countは、実際にイテレーションを回してみて回った数を数える実装となっています。
pull request ( https://github.com/ruby/ruby/pull/444 )の狙いはEnumerable#to_aの高速化なので、countは不向きです。
Enumerator#sizeがFixnumを返す場合に限っては、それに依拠して配列のサイズを決めてしまって問題ないのではないかと思います。
Status changed from Closed to Assigned
議論は #9118 にあります。
そこでも指摘していますが、この最適化は def size; to_a.size; end という手抜きな size 実装で無限再帰になります(そういう実装はいくつも実在します)。再入チェックを入れてもなお高速か、確かめる必要があるでしょう。
まずは revert に一票です。
--
Yusuke Endoh mame@ruby-lang.org
Related to Feature #9118 : In Enumerable#to_a, use size to set array capa when possible added
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 ]
再入チェックを入れてパフォーマンスを比較した結果、改善が見られなかったので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
Like 0
Like 0 Like 0 Like 0 Like 0 Like 0 Like 0 Like 0 Like 0 Like 0 Like 0 Like 0 Like 0 Like 0 Like 0 Like 0