Forum: Ruby-dev [ruby-trunk - Feature #7368][Open] rb str each line()のパフォーマンス向上とリファクタリング

Issue #7368 has been reported by Glass_saga (Masaki Matsushita).

----------------------------------------
Feature #7368: rb_str_each_line()のパフォーマンス向上とリファクタリング
https://bugs.ruby-lang.org/issues/7368

Author: Glass_saga (Masaki Matsushita)
Status: Open
Priority: Normal
Assignee:
Category: core
Target version:


rb_str_each_line()でmemmem(3)を使う事を[ruby-dev:45344] [Feature 
#6129]で提案しましたが、
string.cからmemmem(3)を直接使わずに検索をrb_memsearch()にまとめた上で、
検索文字列と被検索文字列の両方がvalidなencodingである場合と、そうでない場合に関数を分けてリファクタリングしたpatchを作りました。
(どちらかがinvalidな場合には、rb_enc_codepoint_len()で舐めていき途中でArgumentErrorを投げる必要があるので、旧来の処理を使う必要があります)

このpatchには以下の利点があります。

* string.c側でmemmem(3)の有無を意識する必要がない

* 
これまで検索文字列がrb_default_rsである場合にはrb_str_each_line()側でmemchr(3)を用いた最適化が施されていた(trunkのstring.cの6166行以降)が、
  rb_memsearch()を使えば検索文字列の長さに合わせて最適化された検索をしてくれるので、このような特別扱いが不要になる

* 検索文字列と被検索文字列のencodingがvalidなら、検索文字列がrb_default_rsでない場合のパフォーマンスが向上する

* これまでrb_str_each_line()の冒頭で宣言されていた多数の変数を分けた関数毎に局所化できるので、以前よりは読みやすくなる

また、以下のコードでベンチマークを取りました。

require 'benchmark'

str = "hogehifuga\n" * 100_0000

Benchmark.bm do |x|
  x.report("default rs") do
    10.times do
      str.each_line {}
    end
  end

  x.report("not default rs") do
    10.times do
      str.each_line("hi") {}
    end
  end
end

trunk(r37670):
       user     system      total        real
default rs  2.060000   0.000000   2.060000 (  2.055412)
not default rs  3.700000   0.000000   3.700000 (  3.698057)

proposed:
       user     system      total        real
default rs  2.100000   0.000000   2.100000 (  2.095167)
not default rs  2.150000   0.000000   2.150000 (  2.153824)

検索文字列がrb_default_rsな場合のパフォーマンスが低下していない事、検索文字列がrb_default_rsでない場合にはパフォーマンスが向上している事が確認できます。

手元ではtest-allが通っていますが、それなりにコードを削った上に関数を新設しているので、patchをレビューして頂けると大変助かります。
よろしくお願いします。

Issue #7368 has been updated by naruse (Yui NARUSE).


validかinvalidかで区別する必要は無いし、それで十分でもありません。
防ぐべき誤マッチは文字境界をまたいだマッチであり、このパッチだとたぶん、
EUC-JPで「スト」に「好」がマッチするような場合は防げないんじゃないでしょうか。
（それっぽいのがないなーってとこまでしか見てないので、ちゃんと対策してたら申し訳ない）

行われるべき誤マッチ対策は rb_str_index にあるので、それを参考にしつつ直す必要があると思います。
----------------------------------------
Feature #7368: rb_str_each_line()のパフォーマンス向上とリファクタリング
https://bugs.ruby-lang.org/issues/7368#change-33019

Author: Glass_saga (Masaki Matsushita)
Status: Open
Priority: Normal
Assignee:
Category: core
Target version:


rb_str_each_line()でmemmem(3)を使う事を[ruby-dev:45344] [Feature 
#6129]で提案しましたが、
string.cからmemmem(3)を直接使わずに検索をrb_memsearch()にまとめた上で、
検索文字列と被検索文字列の両方がvalidなencodingである場合と、そうでない場合に関数を分けてリファクタリングしたpatchを作りました。
(どちらかがinvalidな場合には、rb_enc_codepoint_len()で舐めていき途中でArgumentErrorを投げる必要があるので、旧来の処理を使う必要があります)

このpatchには以下の利点があります。

* string.c側でmemmem(3)の有無を意識する必要がない

* 
これまで検索文字列がrb_default_rsである場合にはrb_str_each_line()側でmemchr(3)を用いた最適化が施されていた(trunkのstring.cの6166行以降)が、
  rb_memsearch()を使えば検索文字列の長さに合わせて最適化された検索をしてくれるので、このような特別扱いが不要になる

* 検索文字列と被検索文字列のencodingがvalidなら、検索文字列がrb_default_rsでない場合のパフォーマンスが向上する

* これまでrb_str_each_line()の冒頭で宣言されていた多数の変数を分けた関数毎に局所化できるので、以前よりは読みやすくなる

また、以下のコードでベンチマークを取りました。

require 'benchmark'

str = "hogehifuga\n" * 100_0000

Benchmark.bm do |x|
  x.report("default rs") do
    10.times do
      str.each_line {}
    end
  end

  x.report("not default rs") do
    10.times do
      str.each_line("hi") {}
    end
  end
end

trunk(r37670):
       user     system      total        real
default rs  2.060000   0.000000   2.060000 (  2.055412)
not default rs  3.700000   0.000000   3.700000 (  3.698057)

proposed:
       user     system      total        real
default rs  2.100000   0.000000   2.100000 (  2.095167)
not default rs  2.150000   0.000000   2.150000 (  2.153824)

検索文字列がrb_default_rsな場合のパフォーマンスが低下していない事、検索文字列がrb_default_rsでない場合にはパフォーマンスが向上している事が確認できます。

手元ではtest-allが通っていますが、それなりにコードを削った上に関数を新設しているので、patchをレビューして頂けると大変助かります。
よろしくお願いします。

Issue #7368 has been updated by Glass_saga (Masaki Matsushita).

File patch2.diff added

返信が遅くなってしまいました。
成瀬さん、レビュー頂きありがとうございます。

> このパッチだとたぶん、EUC-JPで「スト」に「好」がマッチするような場合は防げないんじゃないでしょうか。

rb_str_index()相当の誤マッチ対策をしてはいたのですが、誤マッチを検出した後の処理がまずかったので意味を為していませんでした。
この点は添付のpatchで修正しました。

> validかinvalidかで区別する必要は無いし、それで十分でもありません。

validとinvalidで区別しているのは、被検索文字列がinvalidである場合に途中でInvalidByteSequenceErrorがraiseされる事を期待しているテストがある為です。
rb_memsearch()は途中に不正なバイト列があっても素通りしてしまうので、検索の途中で例外を投げる事ができません。
従って、被検索文字列がinvalidな場合には従来のようにrb_enc_codepoint_len()で不正なバイト列でないかチェックしながら検索する必要があります。

誤マッチ検出後のバグを修正したpatchを添付します。
----------------------------------------
Feature #7368: rb_str_each_line()のパフォーマンス向上とリファクタリング
https://bugs.ruby-lang.org/issues/7368#change-33264

Author: Glass_saga (Masaki Matsushita)
Status: Open
Priority: Normal
Assignee:
Category: core
Target version:


rb_str_each_line()でmemmem(3)を使う事を[ruby-dev:45344] [Feature 
#6129]で提案しましたが、
string.cからmemmem(3)を直接使わずに検索をrb_memsearch()にまとめた上で、
検索文字列と被検索文字列の両方がvalidなencodingである場合と、そうでない場合に関数を分けてリファクタリングしたpatchを作りました。
(どちらかがinvalidな場合には、rb_enc_codepoint_len()で舐めていき途中でArgumentErrorを投げる必要があるので、旧来の処理を使う必要があります)

このpatchには以下の利点があります。

* string.c側でmemmem(3)の有無を意識する必要がない

* 
これまで検索文字列がrb_default_rsである場合にはrb_str_each_line()側でmemchr(3)を用いた最適化が施されていた(trunkのstring.cの6166行以降)が、
  rb_memsearch()を使えば検索文字列の長さに合わせて最適化された検索をしてくれるので、このような特別扱いが不要になる

* 検索文字列と被検索文字列のencodingがvalidなら、検索文字列がrb_default_rsでない場合のパフォーマンスが向上する

* これまでrb_str_each_line()の冒頭で宣言されていた多数の変数を分けた関数毎に局所化できるので、以前よりは読みやすくなる

また、以下のコードでベンチマークを取りました。

require 'benchmark'

str = "hogehifuga\n" * 100_0000

Benchmark.bm do |x|
  x.report("default rs") do
    10.times do
      str.each_line {}
    end
  end

  x.report("not default rs") do
    10.times do
      str.each_line("hi") {}
    end
  end
end

trunk(r37670):
       user     system      total        real
default rs  2.060000   0.000000   2.060000 (  2.055412)
not default rs  3.700000   0.000000   3.700000 (  3.698057)

proposed:
       user     system      total        real
default rs  2.100000   0.000000   2.100000 (  2.095167)
not default rs  2.150000   0.000000   2.150000 (  2.153824)

検索文字列がrb_default_rsな場合のパフォーマンスが低下していない事、検索文字列がrb_default_rsでない場合にはパフォーマンスが向上している事が確認できます。

手元ではtest-allが通っていますが、それなりにコードを削った上に関数を新設しているので、patchをレビューして頂けると大変助かります。
よろしくお願いします。

Issue #7368 has been updated by mame (Yusuke Endoh).

Status changed from Open to Assigned
Assignee set to naruse (Yui NARUSE)
Target version set to next minor


----------------------------------------
Feature #7368: rb_str_each_line()のパフォーマンス向上とリファクタリング
https://bugs.ruby-lang.org/issues/7368#change-33743

Author: Glass_saga (Masaki Matsushita)
Status: Assigned
Priority: Normal
Assignee: naruse (Yui NARUSE)
Category: core
Target version: next minor


rb_str_each_line()でmemmem(3)を使う事を[ruby-dev:45344] [Feature 
#6129]で提案しましたが、
string.cからmemmem(3)を直接使わずに検索をrb_memsearch()にまとめた上で、
検索文字列と被検索文字列の両方がvalidなencodingである場合と、そうでない場合に関数を分けてリファクタリングしたpatchを作りました。
(どちらかがinvalidな場合には、rb_enc_codepoint_len()で舐めていき途中でArgumentErrorを投げる必要があるので、旧来の処理を使う必要があります)

このpatchには以下の利点があります。

* string.c側でmemmem(3)の有無を意識する必要がない

* 
これまで検索文字列がrb_default_rsである場合にはrb_str_each_line()側でmemchr(3)を用いた最適化が施されていた(trunkのstring.cの6166行以降)が、
  rb_memsearch()を使えば検索文字列の長さに合わせて最適化された検索をしてくれるので、このような特別扱いが不要になる

* 検索文字列と被検索文字列のencodingがvalidなら、検索文字列がrb_default_rsでない場合のパフォーマンスが向上する

* これまでrb_str_each_line()の冒頭で宣言されていた多数の変数を分けた関数毎に局所化できるので、以前よりは読みやすくなる

また、以下のコードでベンチマークを取りました。

require 'benchmark'

str = "hogehifuga\n" * 100_0000

Benchmark.bm do |x|
  x.report("default rs") do
    10.times do
      str.each_line {}
    end
  end

  x.report("not default rs") do
    10.times do
      str.each_line("hi") {}
    end
  end
end

trunk(r37670):
       user     system      total        real
default rs  2.060000   0.000000   2.060000 (  2.055412)
not default rs  3.700000   0.000000   3.700000 (  3.698057)

proposed:
       user     system      total        real
default rs  2.100000   0.000000   2.100000 (  2.095167)
not default rs  2.150000   0.000000   2.150000 (  2.153824)

検索文字列がrb_default_rsな場合のパフォーマンスが低下していない事、検索文字列がrb_default_rsでない場合にはパフォーマンスが向上している事が確認できます。

手元ではtest-allが通っていますが、それなりにコードを削った上に関数を新設しているので、patchをレビューして頂けると大変助かります。
よろしくお願いします。