Forum: Ruby-dev [ruby-trunk - Feature #6440][Open] 引数にIOを渡した場合のMarshal.loadにバッファを持たせたい

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

----------------------------------------
Feature #6440: 引数にIOを渡した場合のMarshal.loadにバッファを持たせたい
https://bugs.ruby-lang.org/issues/6440

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


現在のMarshal.loadでは、引数にIOを渡すとIO#getbyteやIO#readで当座に必要な部分のみの読み出しを繰り返すので
大量のメソッド呼び出しが発生し、そのコストが無視できません。
そこで、引数にIOを渡した場合のMarshal.loadにバッファを持たせる事を提案します。

require 'benchmark'
require 'tempfile'

ary = Array.new(1000){ "hoge" }
file = Tempfile.new("foo")
Marshal.dump(ary, file)

Benchmark.bm do |x|
  x.report do
    100.times do
      file.rewind
      Marshal.load(file)
    end
  end
end

file.close

上記のベンチマークでバッファを持つようにしたrubyとtrunkを比較したところ、以下の結果となりました。

trunk(r35660):
       user     system      total        real
   1.880000   0.000000   1.880000 (  1.874681)

proposed:
       user     system      total        real
   0.180000   0.000000   0.180000 (  0.178556)

patchを添付します。

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

Status changed from Open to Assigned
Assignee set to nobu (Nobuyoshi Nakada)

なんとなくなかださんに。

--
Yusuke Endoh <mame@tsg.ne.jp>
----------------------------------------
Feature #6440: 引数にIOを渡した場合のMarshal.loadにバッファを持たせたい
https://bugs.ruby-lang.org/issues/6440#change-26666

Author: Glass_saga (Masaki Matsushita)
Status: Assigned
Priority: Normal
Assignee: nobu (Nobuyoshi Nakada)
Category: core
Target version: 2.0.0


現在のMarshal.loadでは、引数にIOを渡すとIO#getbyteやIO#readで当座に必要な部分のみの読み出しを繰り返すので
大量のメソッド呼び出しが発生し、そのコストが無視できません。
そこで、引数にIOを渡した場合のMarshal.loadにバッファを持たせる事を提案します。

require 'benchmark'
require 'tempfile'

ary = Array.new(1000){ "hoge" }
file = Tempfile.new("foo")
Marshal.dump(ary, file)

Benchmark.bm do |x|
  x.report do
    100.times do
      file.rewind
      Marshal.load(file)
    end
  end
end

file.close

上記のベンチマークでバッファを持つようにしたrubyとtrunkを比較したところ、以下の結果となりました。

trunk(r35660):
       user     system      total        real
   1.880000   0.000000   1.880000 (  1.874681)

proposed:
       user     system      total        real
   0.180000   0.000000   0.180000 (  0.178556)

patchを添付します。

Issue #6440 has been updated by nobu (Nobuyoshi Nakada).


=begin
細かい問題を修正すればいいんじゃないですかね。

* struct load_arg.bufのメモリリーク
* r_bytes1_partial()でreadpartialでの不足分をreadで追加したときにtmp_ptrが不正

あとは感想をいくつか。

* arg->partialをフラグにするよりsymbolを持たせてはどうか
* r_bytes()とr_bytes0()の追加部分は関数に分けることはできないだろうか
* 予約語(if)とカッコの間が空いていない
=end

----------------------------------------
Feature #6440: 引数にIOを渡した場合のMarshal.loadにバッファを持たせたい
https://bugs.ruby-lang.org/issues/6440#change-26676

Author: Glass_saga (Masaki Matsushita)
Status: Assigned
Priority: Normal
Assignee: nobu (Nobuyoshi Nakada)
Category: core
Target version: 2.0.0


現在のMarshal.loadでは、引数にIOを渡すとIO#getbyteやIO#readで当座に必要な部分のみの読み出しを繰り返すので
大量のメソッド呼び出しが発生し、そのコストが無視できません。
そこで、引数にIOを渡した場合のMarshal.loadにバッファを持たせる事を提案します。

require 'benchmark'
require 'tempfile'

ary = Array.new(1000){ "hoge" }
file = Tempfile.new("foo")
Marshal.dump(ary, file)

Benchmark.bm do |x|
  x.report do
    100.times do
      file.rewind
      Marshal.load(file)
    end
  end
end

file.close

上記のベンチマークでバッファを持つようにしたrubyとtrunkを比較したところ、以下の結果となりました。

trunk(r35660):
       user     system      total        real
   1.880000   0.000000   1.880000 (  1.874681)

proposed:
       user     system      total        real
   0.180000   0.000000   0.180000 (  0.178556)

patchを添付します。

Issue #6440 has been updated by nobu (Nobuyoshi Nakada).

Description updated

=begin
もう一点、s_getbyteも不要になるはずです。
=end

----------------------------------------
Feature #6440: 引数にIOを渡した場合のMarshal.loadにバッファを持たせたい
https://bugs.ruby-lang.org/issues/6440#change-26677

Author: Glass_saga (Masaki Matsushita)
Status: Assigned
Priority: Normal
Assignee: nobu (Nobuyoshi Nakada)
Category: core
Target version: 2.0.0


=begin
現在の(({Marshal.load}))では、引数に(({IO}))を渡すと(({IO#getbyte}))や(({IO#read}))で当座に必要な部分のみの読み出しを繰り返すので
大量のメソッド呼び出しが発生し、そのコストが無視できません。
そこで、引数に(({IO}))を渡した場合の(({Marshal.load}))にバッファを持たせる事を提案します。

  require 'benchmark'
  require 'tempfile'

  ary = Array.new(1000){ "hoge" }
  file = Tempfile.new("foo")
  Marshal.dump(ary, file)

  Benchmark.bm do |x|
    x.report do
      100.times do
        file.rewind
        Marshal.load(file)
      end
    end
  end

  file.close

上記のベンチマークでバッファを持つようにしたrubyとtrunkを比較したところ、以下の結果となりました。

  trunk(r35660):
         user     system      total        real
     1.880000   0.000000   1.880000 (  1.874681)

  proposed:
         user     system      total        real
     0.180000   0.000000   0.180000 (  0.178556)

patchを添付します。
=end

2012/5/16 Glass_saga (Masaki Matsushita) <glass.saga@gmail.com>:
>
> Issue #6440 has been reported by Glass_saga (Masaki Matsushita).
>
> ----------------------------------------
> Feature #6440: 
$B0z?t$K(BIO$B$rEO$7$?>l9g$N(BMarshal.load$B$K%P%C%U%!$r;}$?$;$?$$(B
> https://bugs.ruby-lang.org/issues/6440

> 
$B8=:_$N(BMarshal.load$B$G$O!"0z?t$K(BIO$B$rEO$9$H(BIO#getbyte$B$d(BIO#read$B$GEv:B$KI,MW$JItJ,$N$_$NFI$_=P$7$r7+$jJV$9$N$G(B
> $BBgNL$N%a%=%C%I8F$S=P$7$,H/@8$7!"$=$N%3%9%H$,L5;k$G$-$^$;$s!#(B
> 
$B$=$3$G!"0z?t$K(BIO$B$rEO$7$?>l9g$N(BMarshal.load$B$K%P%C%U%!$r;}$?$;$k;v$rDs0F$7$^$9!#(B

$B%P%C%U%!$,$"$k$H!"FI$_$9$.$k$3$H$,$"$k$N$G$O$J$$$G$7$g$&$+!#(B

$BFI$_$9$.$?$V$s$r<N$F$F$7$^$&$H!"(B
marshal $B$7$?%G!<%?$,J#?tJB$s$G$$$k$b$N$rFI$_9~$`>l9g$K!"(B
$BF0:n$7$J$/$J$C$F$7$^$&$H;W$$$^$9!#(B

$B<B:]!"%Q%C%A$rEv$F$k$H0J2<$N$h$&$K%(%i!<$K$J$C$F$7$^$$$^$9!#(B

% ./ruby -e '2.times { Marshal.dump(1, STDOUT) }'|
./ruby -e '2.times { p Marshal.load(STDIN) }'
1
-e:1:in `readpartial': end of file reached (EOFError)
  from -e:1:in `load'
  from -e:1:in `block in <main>'
  from -e:1:in `times'
  from -e:1:in `<main>'

$B%Q%C%A$rEv$F$J$1$l$P!"0J2<$N$h$&$KF0:n$7$^$9!#(B

% ruby -e '2.times { Marshal.dump(1, STDOUT) }'|
ruby -e '2.times { p Marshal.load(STDIN) }'
1
1

$B8D?ME*$K:G6a:n$C$F$$$k(B tb 
$B$H$$$&$b$N$G!"$3$NF0:n$rMxMQ$7$F$$$k$N$G!"(B
$B5$$K$J$k$H$3$m$G$9!#(B

$B$J$+$@$G$9!#(B

(12/05/17 18:21), Tanaka Akira wrote:
> $B%P%C%U%!$,$"$k$H!"FI$_$9$.$k$3$H$,$"$k$N$G$O$J$$$G$7$g$&$+!#(B
>
> $BFI$_$9$.$?$V$s$r<N$F$F$7$^$&$H!"(B marshal 
$B$7$?%G!<%?$,J#?tJB$s$G$$$k$b$N$rFI$_9~$`>l9g$K!"(B 
$BF0:n$7$J$/$J$C$F$7$^$&$H;W$$$^$9!#(B

ungetc$B$9$k$N$O$I$&$G$7$g$&$+!#%W%m%;%9$r$^$?$$$G$7$^$&$H$d$O$j%@%a$G$9(B
$B$,!"$3$l$O8=>u$G$b$G$-$J$$$h$&$G$9$7!#(B


diff --git i/marshal.c w/marshal.c
index e05b9f5..20b22ea 100644
--- i/marshal.c
+++ w/marshal.c
@@ -81,7 +81,7 @@ shortlen(long len, BDIGIT *ds)

 static ID s_dump, s_load, s_mdump, s_mload;
 static ID s_dump_data, s_load_data, s_alloc, s_call;
-static ID s_getbyte, s_read, s_write, s_binmode;
+static ID s_ungetc, s_read, s_readpartial, s_write, s_binmode;

 typedef struct {
     VALUE newclass;
@@ -958,7 +958,10 @@ marshal_dump(int argc, VALUE *argv)

 struct load_arg {
     VALUE src;
+    char *buf;
+    long buflen;
     long offset;
+    int partial;
     st_table *symbols;
     st_table *data;
     VALUE proc;
@@ -1011,6 +1014,13 @@ static VALUE r_object(struct load_arg *arg);
 static ID r_symbol(struct load_arg *arg);
 static VALUE path2class(VALUE path);

+NORETURN(static void too_short(void));
+static void
+too_short(void)
+{
+    rb_raise(rb_eArgError, "marshal data too short");
+}
+
 static st_index_t
 r_prepare(struct load_arg *arg)
 {
@@ -1030,15 +1040,28 @@ r_byte(struct load_arg *arg)
       c = (unsigned char)RSTRING_PTR(arg->src)[arg->offset++];
   }
   else {
-      rb_raise(rb_eArgError, "marshal data too short");
+      too_short();
   }
     }
     else {
-  VALUE src = arg->src;
-  VALUE v = rb_funcall2(src, s_getbyte, 0, 0);
-  check_load_arg(arg, s_getbyte);
-  if (NIL_P(v)) rb_eof_error();
-  c = (unsigned char)NUM2CHR(v);
+  if (arg->buflen == 0) {
+      VALUE str, n = LONG2NUM(BUFSIZ);
+
+      if (arg->partial)
+    str = rb_funcall2(arg->src, s_readpartial, 1, &n);
+      else
+    str = rb_funcall2(arg->src, s_read, 1, &n);
+
+      check_load_arg(arg, s_read);
+      if (NIL_P(str)) too_short();
+      StringValue(str);
+      arg->infection |= (int)FL_TEST(str, MARSHAL_INFECTION);
+      memcpy(arg->buf, RSTRING_PTR(str), RSTRING_LEN(str));
+      arg->offset = 0;
+      arg->buflen = RSTRING_LEN(str);
+  }
+  c = (unsigned char)arg->buf[arg->offset++];
+  arg->buflen--;
     }
     return c;
 }
@@ -1091,6 +1114,63 @@ r_long(struct load_arg *arg)
     return x;
 }

+static VALUE
+r_bytes1(long len, struct load_arg *arg)
+{
+    VALUE str, n = LONG2NUM(len);
+
+    str = rb_funcall2(arg->src, s_read, 1, &n);
+    check_load_arg(arg, s_read);
+
+    if (NIL_P(str)) {
+  too_short();
+    }
+    StringValue(str);
+    if (RSTRING_LEN(str) < len) too_short();
+
+    arg->infection |= (int)FL_TEST(str, MARSHAL_INFECTION);
+
+    return str;
+}
+
+static VALUE
+r_bytes1_partial(long len, struct load_arg *arg)
+{
+    long buflen = arg->buflen;
+    long tmp_len, need = len - buflen;
+    const char *tmp_ptr;
+    VALUE str, tmp, n = LONG2NUM(need > BUFSIZ ? need : BUFSIZ);
+
+    tmp = rb_funcall2(arg->src, s_readpartial, 1, &n);
+
+    check_load_arg(arg, s_read);
+    if (NIL_P(tmp)) {
+  too_short();
+    }
+    StringValue(tmp);
+
+    tmp_len = RSTRING_LEN(tmp);
+
+    if (tmp_len < need) {
+  /* retry */
+  VALUE fill = r_bytes1(need-tmp_len, arg);
+  rb_str_concat(tmp, fill);
+  tmp_len = RSTRING_LEN(tmp);
+    }
+
+    tmp_ptr = RSTRING_PTR(tmp);
+    arg->infection |= (int)FL_TEST(tmp, MARSHAL_INFECTION);
+    str = rb_str_new(arg->buf+arg->offset, buflen);
+    rb_str_cat(str, tmp_ptr, need);
+    if (tmp_len-need > 0)
+  memcpy(arg->buf, tmp_ptr+need, tmp_len-need);
+
+    arg->offset = 0;
+    arg->buflen = tmp_len - need;
+
+    return str;
+}
+
 #define r_bytes(arg) r_bytes0(r_long(arg), (arg))

 static VALUE
@@ -1105,19 +1185,21 @@ r_bytes0(long len, struct load_arg *arg)
       arg->offset += len;
   }
   else {
-    too_short:
-      rb_raise(rb_eArgError, "marshal data too short");
+      too_short();
   }
     }
     else {
-  VALUE src = arg->src;
-  VALUE n = LONG2NUM(len);
-  str = rb_funcall2(src, s_read, 1, &n);
-  check_load_arg(arg, s_read);
-  if (NIL_P(str)) goto too_short;
-  StringValue(str);
-  if (RSTRING_LEN(str) != len) goto too_short;
-  arg->infection |= (int)FL_TEST(str, MARSHAL_INFECTION);
+  if (len <= arg->buflen) {
+      str = rb_str_new(arg->buf+arg->offset, len);
+      arg->offset += len;
+      arg->buflen -= len;
+  }
+  else {
+      if (arg->partial)
+    str = r_bytes1_partial(len, arg);
+      else
+    str = r_bytes1(len, arg);
+  }
     }
     return str;
 }
@@ -1732,6 +1814,16 @@ r_object(struct load_arg *arg)
 static void
 clear_load_arg(struct load_arg *arg)
 {
+    if (arg->buf) {
+  if (arg->buflen) {
+      VALUE str = rb_str_new(arg->buf+arg->offset, arg->buflen);
+      arg->buflen = 0;
+      rb_funcall2(arg->src, s_ungetc, 1, &str);
+  }
+  xfree(arg->buf);
+  arg->buf = 0;
+  arg->offset = 0;
+    }
     if (!arg->symbols) return;
     st_free_table(arg->symbols);
     arg->symbols = 0;
@@ -1767,7 +1859,7 @@ marshal_load(int argc, VALUE *argv)
   infection = (int)FL_TEST(port, MARSHAL_INFECTION); /* original 
taintedness */
   port = v;
     }
-    else if (rb_respond_to(port, s_getbyte) && rb_respond_to(port, 
s_read)) {
+    else if (rb_respond_to(port, s_ungetc) && rb_respond_to(port, 
s_read)) {
   if (rb_respond_to(port, s_binmode)) {
       rb_funcall2(port, s_binmode, 0, 0);
   }
@@ -1784,6 +1876,13 @@ marshal_load(int argc, VALUE *argv)
     arg->data    = st_init_numtable();
     arg->compat_tbl = st_init_numtable();
     arg->proc = 0;
+    arg->partial = 0;
+    arg->buf = 0;
+
+    if (NIL_P(v)) {
+  if (rb_respond_to(port, s_readpartial)) arg->partial = 1;
+  arg->buf = xmalloc(BUFSIZ);
+    }

     major = r_byte(arg);
     minor = r_byte(arg);
@@ -1919,8 +2018,9 @@ Init_marshal(void)
     s_load_data = rb_intern("_load_data");
     s_alloc = rb_intern("_alloc");
     s_call = rb_intern("call");
-    s_getbyte = rb_intern("getbyte");
+    s_ungetc = rb_intern("ungetc");
     s_read = rb_intern("read");
+    s_readpartial = rb_intern("readpartial");
     s_write = rb_intern("write");
     s_binmode = rb_intern("binmode");

diff --git i/test/ruby/test_marshal.rb w/test/ruby/test_marshal.rb
index 85cec0a..375d274 100644
--- i/test/ruby/test_marshal.rb
+++ w/test/ruby/test_marshal.rb
@@ -492,4 +492,14 @@ class TestMarshal < Test::Unit::TestCase
     assert_equal(Rational(1, 2), 
Marshal.load("\x04\bU:\rRational[\ai\x06i\a"))
     assert_raise(ArgumentError){Marshal.load("\x04\bU:\rRational[\bi\x00i\x00i\x00")}
   end
+
+  def test_successive
+    result = IO.pipe do |r, w|
+      Thread.start do
+        2.times {Marshal.dump(1, w)}
+      end
+      2.times.map {Marshal.load(r)}
+    end
+    assert_equal([1, 1], result, '[ruby-dev:45644]')
+  end
 end

2012/5/17 Nobuyoshi Nakada <nobu@ruby-lang.org>:
>
> ungetc$B$9$k$N$O$I$&$G$7$g$&$+!#%W%m%;%9$r$^$?$$$G$7$^$&$H$d$O$j%@%a$G$9(B
> $B$,!"$3$l$O8=>u$G$b$G$-$J$$$h$&$G$9$7!#(B

$B$&$%$`!#$I$N$/$i$$(B ungetc $B$G$-$k$+$O%P%C%U%!$N>u67$K$h$k$N$G!"(B
$B$A$c$s$HF0$-$^$9$+$M$'(B?

$B$H$$$&$+!"$o$6$o$6?7$7$/%P%C%U%!$r<BAu$7$J$/$F$b!"(B
IO $B$N%a%=%C%I$N<BBN$rD>@\8F$Y$P(B
(IO $B$N>l9g(B) $B%a%=%C%I8F$S=P$7%3%9%H$O>C$;$k$s$8$c$J$$$G$9$+$M!#(B
$B$=$l$G$O%@%a(B?

(12/05/18 0:00), Tanaka Akira wrote:
> 2012/5/17 Nobuyoshi Nakada <nobu@ruby-lang.org>:
>> ungetc$B$9$k$N$O$I$&$G$7$g$&$+!#%W%m%;%9$r$^$?$$$G$7$^$&$H$d$O$j%@%a$G$9(B
>> $B$,!"$3$l$O8=>u$G$b$G$-$J$$$h$&$G$9$7!#(B
>
> $B$&$%$`!#$I$N$/$i$$(B ungetc $B$G$-$k$+$O%P%C%U%!$N>u67$K$h$k$N$G!"(B
> $B$A$c$s$HF0$-$^$9$+$M$'(B?

$B$=$&$$$($P(Bungetc$B$G$O%P%C%U%!$rDI2C$9$k$o$1$8$c$J$$$G$9$M!#(B

> $B$H$$$&$+!"$o$6$o$6?7$7$/%P%C%U%!$r<BAu$7$J$/$F$b!"(B
> IO $B$N%a%=%C%I$N<BBN$rD>@\8F$Y$P(B
> (IO $B$N>l9g(B) $B%a%=%C%I8F$S=P$7%3%9%H$O>C$;$k$s$8$c$J$$$G$9$+$M!#(B
> $B$=$l$G$O%@%a(B?

$B;n$7$F$_$^$7$?$,!"$"$^$jB.$/$O$J$i$J$$$h$&$G$9!#(B

  original       4.15sec
  rb_method_call 4.11sec
  rb_vm_call     3.88sec

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


なかださん、田中さん、レビューとご意見をありがとうございます。

現在のMarshalの形式では全体のサイズを知る方法がない為に、バッファを持とうとするとどうしても読み過ぎてしまいます。
ungetcでは駄目となると、seekが可能なIOに対してのみバッファを持つようにして、最後にIO#seekで辻褄を合わせるというのはどうでしょうか。
高速化できるIOの種類が限られてしまいますが、互換性は崩さずに済むと思います。
----------------------------------------
Feature #6440: 引数にIOを渡した場合のMarshal.loadにバッファを持たせたい
https://bugs.ruby-lang.org/issues/6440#change-26684

Author: Glass_saga (Masaki Matsushita)
Status: Assigned
Priority: Normal
Assignee: nobu (Nobuyoshi Nakada)
Category: core
Target version: 2.0.0


=begin
現在の(({Marshal.load}))では、引数に(({IO}))を渡すと(({IO#getbyte}))や(({IO#read}))で当座に必要な部分のみの読み出しを繰り返すので
大量のメソッド呼び出しが発生し、そのコストが無視できません。
そこで、引数に(({IO}))を渡した場合の(({Marshal.load}))にバッファを持たせる事を提案します。

  require 'benchmark'
  require 'tempfile'

  ary = Array.new(1000){ "hoge" }
  file = Tempfile.new("foo")
  Marshal.dump(ary, file)

  Benchmark.bm do |x|
    x.report do
      100.times do
        file.rewind
        Marshal.load(file)
      end
    end
  end

  file.close

上記のベンチマークでバッファを持つようにしたrubyとtrunkを比較したところ、以下の結果となりました。

  trunk(r35660):
         user     system      total        real
     1.880000   0.000000   1.880000 (  1.874681)

  proposed:
         user     system      total        real
     0.180000   0.000000   0.180000 (  0.178556)

patchを添付します。
=end

(2012/05/18 1:39), Glass_saga (Masaki Matsushita) wrote:
> 現在のMarshalの形式では全体のサイズを知る方法がない為に、バッファを持とうとするとどうしても読み過ぎてしまいます。
> ungetcでは駄目となると、seekが可能なIOに対してのみバッファを持つようにして、最後にIO#seekで辻褄を合わせるというのはどうでしょうか。
> 高速化できるIOの種類が限られてしまいますが、互換性は崩さずに済むと思います。

　面白いですね．少し考えてみました．

# ご提案の「seek が効くもののみ」というのは，要するに File の時は，
# って感じですかねぇ．pipe 的なもの（ソケット等）を使うのとどっちが
# 用途として多いんだろう．


案1）
　でかいオブジェクトをやりとりするのは意識が高い人に違いないので，もっと
意識を高く（低く？），こんな感じで workaround をしてもらう．Marshal した
文字列を Marshal して送る．メモリは食いますが，今時気にするだろうか．

  require 'benchmark'
  require 'tempfile'

  ary = Array.new(10_000){ "hoge" }
  file = Tempfile.new("foo")
  file2 = Tempfile.new("bar")
  Marshal.dump(ary, file)
  Marshal.dump(Marshal.dump(ary), file2)

  Benchmark.bm do |x|
    x.report do
      100.times do
        file.rewind
        Marshal.load(file)
      end
    end
    x.report do
      100.times do
        file2.rewind
        Marshal.load(Marshal.load(file2))
      end
    end
  end

  file.close

ruby 2.0.0dev (2012-05-04 trunk 35535) [i386-mswin32_100]
       user     system      total        real
   6.068000   0.031000   6.099000 (  6.334804)
   0.530000   0.016000   0.546000 (  0.573573)


案2）真面目に先読み機構を入れる．

　n 要素の Array を load する場合，少なくとも n byte 先読み出来ます（と
いうことを調べるために，始めて Marshal フォーマット *1 を見た）．m 番目
まで読み進めた段階で先読みしたバッファが不足した場合，n - m byte 先読み
出来ます．ネストしてると面倒になりますが（この情報をスタックで管理して，
一番先読み可能なものを集めて先読み，みたいな）．

*1:
http://www.ruby-lang.org/ja/old-man/html/Marshal_A...

　ちょっと，複雑に過ぎるかも．

2012$BG/(B5$B7n(B18$BF|(B 1:03 Nobuyoshi Nakada <nobu@ruby-lang.org>:

>> $B$H$$$&$+!"$o$6$o$6?7$7$/%P%C%U%!$r<BAu$7$J$/$F$b!"(B
>> IO $B$N%a%=%C%I$N<BBN$rD>@\8F$Y$P(B
>> (IO $B$N>l9g(B) $B%a%=%C%I8F$S=P$7%3%9%H$O>C$;$k$s$8$c$J$$$G$9$+$M!#(B
>> $B$=$l$G$O%@%a(B?
>
> $B;n$7$F$_$^$7$?$,!"$"$^$jB.$/$O$J$i$J$$$h$&$G$9!#(B
>
>  original       4.15sec
>  rb_method_call 4.11sec
>  rb_vm_call     3.88sec

$B$$$^$R$H$D2?$r$d$C$?$N$+3N?.$r;}$F$J$$$N$G$9$,!"(B
$B$=$l$>$l$I$N$h$&$J0UL#$G$7$g$&(B?

2012$BG/(B5$B7n(B17$BF|(B 18:21 Tanaka Akira <akr@fsij.org>:

> $B%P%C%U%!$,$"$k$H!"FI$_$9$.$k$3$H$,$"$k$N$G$O$J$$$G$7$g$&$+!#(B
>
> $BFI$_$9$.$?$V$s$r<N$F$F$7$^$&$H!"(B
> marshal $B$7$?%G!<%?$,J#?tJB$s$G$$$k$b$N$rFI$_9~$`>l9g$K!"(B
> $BF0:n$7$J$/$J$C$F$7$^$&$H;W$$$^$9!#(B

> $B8D?ME*$K:G6a:n$C$F$$$k(B tb $B$H$$$&$b$N$G!"$3$NF0:n$rMxMQ$7$F$$$k$N$G!"(B
> $B5$$K$J$k$H$3$m$G$9!#(B

$B5$$,$D$$$?$N$G5-O?$N$?$a$K=q$$$F$*$-$^$9$,!"(Bmarshal $B$N(B load 
$B$,FI$_$9$.$J$$$H$$$&(B
$B@-<A$r(B process.c $B$G$bMxMQ$7$F$$$k$h$&$G$9!#(B

rb_fork_err $B$G!";R%W%m%;%9$NNc30$r(B marshal $B$GFI$_=P$7!"<!$K(B 
errno $B$rFI$_=P$7!"(B
$B%(%i!<%a%C%;!<%8$rFI$_=P$9!"$H$$$&$h$&$J$3$H$r$d$C$F$$$^$9!#(B

        if ((read(ep[0], &state, sizeof(state))) == sizeof(state) && 
state) {
            io = rb_io_fdopen(ep[0], O_RDONLY|O_BINARY, NULL);
            exc = rb_marshal_load(io);
            rb_set_errinfo(exc);
        }
#define READ_FROM_CHILD(ptr, len) \
        (NIL_P(io) ? read(ep[0], (ptr), (len)) : rb_io_bufread(io,
(ptr), (len)))
        if ((size = READ_FROM_CHILD(&err, sizeof(err))) < 0) {
            err = errno;
        }
        if (size == sizeof(err) &&
            errmsg && 0 < errmsg_buflen) {
            ssize_t ret = READ_FROM_CHILD(errmsg, errmsg_buflen-1);
            if (0 <= ret) {
                errmsg[ret] = '\0';
            }
        }

$B$N$"$?$j$G$9!#(B

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

File patch2.diff added

ささださんの案を元に読み過ぎないような先読み機構を考えてみました。

ささださんが指摘するように、n要素のArrayをm番目まで読んだところでバッファが不足した場合には、
少なくともn - m バイトは先読みできる事が保証されます。

Arrayがネストしている場合には、最も内側にあるArrayに関してはn - m バイト読む事ができますが、
そのひとつ外側のArrayに関しては、最も内側のArrayについて先読みした時点で既に1要素読んでしまっているので、
n - m - 1 バイト先読みする事ができます。それより外側のArrayに関しても同様です。
従って、最も内側のArrayのみn - m バイト先読みする事ができ、それ以外はn - m - 1 バイト先読みする事ができます。

今回は、struct load_argにreadableという「少なくとも何バイト先読みできるか」を表すメンバを追加する事にしました。
r_object0()でtypeがARRAYである場合には、arg->readableにlen - 1を足し、1要素読む毎にデクリメントします。
これは、n - m - 1バイトに対応します。

バッファが不足した場合には、arg->readable + 1バイト先読みできます。
arg->readable + 1とするのは、最も内側のArrayに関してだけn - mバイト先読みする事に相当します。

HashやStructに関しても同様の処理を行なっています。
ただし、これらは1要素に少なくとも2バイトは必要なのでarg->readableには(len - 1) * 
2を足し、1要素読む毎に2を引いています。
最も内側のHashやStructに関しては(n - m) * 2バイト、つまりarg->readable + 2バイトまで読めるのですが、
1バイト多く先読みする為だけにr_byteやr_bytes0にtypeを渡す必要もないと思ったので、そこまではやっていません。

[ruby-dev:45637]と同様のベンチマークを実行したところ、以下の結果となりました。

trunk(r35983)
       user     system      total        real
   0.560000   0.030000   0.590000 (  0.601471)

proposed:
       user     system      total        real
   0.090000   0.010000   0.100000 (  0.113099)

patchを添付します。
----------------------------------------
Feature #6440: 引数にIOを渡した場合のMarshal.loadにバッファを持たせたい
https://bugs.ruby-lang.org/issues/6440#change-27165

Author: Glass_saga (Masaki Matsushita)
Status: Assigned
Priority: Normal
Assignee: nobu (Nobuyoshi Nakada)
Category: core
Target version: 2.0.0


=begin
現在の(({Marshal.load}))では、引数に(({IO}))を渡すと(({IO#getbyte}))や(({IO#read}))で当座に必要な部分のみの読み出しを繰り返すので
大量のメソッド呼び出しが発生し、そのコストが無視できません。
そこで、引数に(({IO}))を渡した場合の(({Marshal.load}))にバッファを持たせる事を提案します。

  require 'benchmark'
  require 'tempfile'

  ary = Array.new(1000){ "hoge" }
  file = Tempfile.new("foo")
  Marshal.dump(ary, file)

  Benchmark.bm do |x|
    x.report do
      100.times do
        file.rewind
        Marshal.load(file)
      end
    end
  end

  file.close

上記のベンチマークでバッファを持つようにしたrubyとtrunkを比較したところ、以下の結果となりました。

  trunk(r35660):
         user     system      total        real
     1.880000   0.000000   1.880000 (  1.874681)

  proposed:
         user     system      total        real
     0.180000   0.000000   0.180000 (  0.178556)

patchを添付します。
=end

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


patch2.diffを適用してコミットしてよいでしょうか？
反対がなければコミットします。
----------------------------------------
Feature #6440: 引数にIOを渡した場合のMarshal.loadにバッファを持たせたい
https://bugs.ruby-lang.org/issues/6440#change-32957

Author: Glass_saga (Masaki Matsushita)
Status: Assigned
Priority: Normal
Assignee: nobu (Nobuyoshi Nakada)
Category: core
Target version: 2.0.0


=begin
現在の(({Marshal.load}))では、引数に(({IO}))を渡すと(({IO#getbyte}))や(({IO#read}))で当座に必要な部分のみの読み出しを繰り返すので
大量のメソッド呼び出しが発生し、そのコストが無視できません。
そこで、引数に(({IO}))を渡した場合の(({Marshal.load}))にバッファを持たせる事を提案します。

  require 'benchmark'
  require 'tempfile'

  ary = Array.new(1000){ "hoge" }
  file = Tempfile.new("foo")
  Marshal.dump(ary, file)

  Benchmark.bm do |x|
    x.report do
      100.times do
        file.rewind
        Marshal.load(file)
      end
    end
  end

  file.close

上記のベンチマークでバッファを持つようにしたrubyとtrunkを比較したところ、以下の結果となりました。

  trunk(r35660):
         user     system      total        real
     1.880000   0.000000   1.880000 (  1.874681)

  proposed:
         user     system      total        real
     0.180000   0.000000   0.180000 (  0.178556)

patchを添付します。
=end