Setting `DefaultFactory` to nil will cause a segv about msgpack-ruby

[tenderlove]

I've sent two PRs to deal with this problem. I prefer #135, but it's a much larger patch than #134, so I understand if it won't get applied.

[tagomoris]

I prefer #134. IMO all ruby code in this repository should be shared with CRuby/JRuby implementations.

[tenderlove]

@tagomoris Understood. I have updated #135 so that JRuby and CRuby share the same Ruby code: tenderlove/msgpack-ruby@d6ef970

I was able to delete even more code. 😊

[tagomoris]

LGTM > #135.
It seems smart for me to reduce C code & move much code to Ruby without performance regression.
@frsyuki How do you think about it?

[frsyuki]

It is very good to move C code to Ruby without performance impact.
But I found one issue: Calling new public write_* methods with unexpected type makes the ruby VM crashed:

irb(main):002:0> MessagePack::Packer.new.write_array(1)
(irb):2: [BUG] Segmentation fault at 0x00000000000005

[tenderlove]

Sorry, I forgot to add the type checks. I've fixed in tenderlove/msgpack-ruby@b8493d7

Benchmark Results. Click to expand

Master Branch (before):

[aaron@TC msgpack-ruby (master)]$ be sh bench/run.sh 
pack
/Users/aaron/.rbenv/versions/2.4.0/lib/ruby/gems/2.4.0/gems/myrrha-1.2.2/lib/myrrha/to_ruby_literal.rb:6: warning: constant ::Fixnum is deprecated
/Users/aaron/.rbenv/versions/2.4.0/lib/ruby/gems/2.4.0/gems/myrrha-1.2.2/lib/myrrha/to_ruby_literal.rb:6: warning: constant ::Bignum is deprecated
+----------+---------+----------+----------+----------+----------+
| :bench   | :runs   | :user    | :system  | :total   | :real    |
+----------+---------+----------+----------+----------+----------+
| :strings |   10000 | 0.010000 | 0.000000 | 0.010000 | 0.011698 |
| :strings |  100000 | 0.110000 | 0.000000 | 0.110000 | 0.110015 |
| :strings | 1000000 | 1.110000 | 0.000000 | 1.110000 | 1.156325 |
| :symbols |   10000 | 0.010000 | 0.000000 | 0.010000 | 0.011114 |
| :symbols |  100000 | 0.140000 | 0.010000 | 0.150000 | 0.141470 |
| :symbols | 1000000 | 1.360000 | 0.020000 | 1.380000 | 1.450633 |
+----------+---------+----------+----------+----------+----------+
unpack
/Users/aaron/.rbenv/versions/2.4.0/lib/ruby/gems/2.4.0/gems/myrrha-1.2.2/lib/myrrha/to_ruby_literal.rb:6: warning: constant ::Fixnum is deprecated
/Users/aaron/.rbenv/versions/2.4.0/lib/ruby/gems/2.4.0/gems/myrrha-1.2.2/lib/myrrha/to_ruby_literal.rb:6: warning: constant ::Bignum is deprecated
+----------+---------+----------+----------+----------+----------+
| :bench   | :runs   | :user    | :system  | :total   | :real    |
+----------+---------+----------+----------+----------+----------+
| :strings |   10000 | 0.040000 | 0.010000 | 0.050000 | 0.042415 |
| :strings |  100000 | 0.300000 | 0.020000 | 0.320000 | 0.322543 |
| :strings | 1000000 | 2.970000 | 0.090000 | 3.060000 | 3.120010 |
| :symbols |   10000 | 0.030000 | 0.010000 | 0.040000 | 0.041115 |
| :symbols |  100000 | 0.330000 | 0.010000 | 0.340000 | 0.353664 |
| :symbols | 1000000 | 3.320000 | 0.090000 | 3.410000 | 3.477491 |
+----------+---------+----------+----------+----------+----------+
pack log
/Users/aaron/.rbenv/versions/2.4.0/lib/ruby/gems/2.4.0/gems/myrrha-1.2.2/lib/myrrha/to_ruby_literal.rb:6: warning: constant ::Fixnum is deprecated
/Users/aaron/.rbenv/versions/2.4.0/lib/ruby/gems/2.4.0/gems/myrrha-1.2.2/lib/myrrha/to_ruby_literal.rb:6: warning: constant ::Bignum is deprecated
+------------+---------+----------+----------+----------+----------+
| :bench     | :runs   | :user    | :system  | :total   | :real    |
+------------+---------+----------+----------+----------+----------+
| :plain     |   10000 | 0.010000 | 0.000000 | 0.010000 | 0.011917 |
| :plain     |  100000 | 0.120000 | 0.010000 | 0.130000 | 0.123696 |
| :plain     | 1000000 | 1.330000 | 0.040000 | 1.370000 | 1.454962 |
| :structure |   10000 | 0.010000 | 0.000000 | 0.010000 | 0.013853 |
| :structure |  100000 | 0.160000 | 0.000000 | 0.160000 | 0.181527 |
| :structure | 1000000 | 1.770000 | 0.060000 | 1.830000 | 1.895626 |
+------------+---------+----------+----------+----------+----------+
unpack log
/Users/aaron/.rbenv/versions/2.4.0/lib/ruby/gems/2.4.0/gems/myrrha-1.2.2/lib/myrrha/to_ruby_literal.rb:6: warning: constant ::Fixnum is deprecated
/Users/aaron/.rbenv/versions/2.4.0/lib/ruby/gems/2.4.0/gems/myrrha-1.2.2/lib/myrrha/to_ruby_literal.rb:6: warning: constant ::Bignum is deprecated
+------------+---------+----------+----------+----------+----------+
| :bench     | :runs   | :user    | :system  | :total   | :real    |
+------------+---------+----------+----------+----------+----------+
| :plain     |   10000 | 0.020000 | 0.020000 | 0.040000 | 0.048792 |
| :plain     |  100000 | 0.240000 | 0.050000 | 0.290000 | 0.288731 |
| :plain     | 1000000 | 2.330000 | 0.230000 | 2.560000 | 2.597261 |
| :structure |   10000 | 0.060000 | 0.000000 | 0.060000 | 0.057620 |
| :structure |  100000 | 0.620000 | 0.010000 | 0.630000 | 0.640666 |
| :structure | 1000000 | 6.440000 | 0.050000 | 6.490000 | 6.587186 |
+------------+---------+----------+----------+----------+----------+

New Branch (after):

[aaron@TC msgpack-ruby (remove-global-c)]$ be sh bench/run.sh 
pack
/Users/aaron/.rbenv/versions/2.4.0/lib/ruby/gems/2.4.0/gems/myrrha-1.2.2/lib/myrrha/to_ruby_literal.rb:6: warning: constant ::Fixnum is deprecated
/Users/aaron/.rbenv/versions/2.4.0/lib/ruby/gems/2.4.0/gems/myrrha-1.2.2/lib/myrrha/to_ruby_literal.rb:6: warning: constant ::Bignum is deprecated
+----------+---------+----------+----------+----------+----------+
| :bench   | :runs   | :user    | :system  | :total   | :real    |
+----------+---------+----------+----------+----------+----------+
| :strings |   10000 | 0.010000 | 0.000000 | 0.010000 | 0.010303 |
| :strings |  100000 | 0.120000 | 0.010000 | 0.130000 | 0.126337 |
| :strings | 1000000 | 1.190000 | 0.010000 | 1.200000 | 1.239178 |
| :symbols |   10000 | 0.010000 | 0.000000 | 0.010000 | 0.011960 |
| :symbols |  100000 | 0.140000 | 0.000000 | 0.140000 | 0.149942 |
| :symbols | 1000000 | 1.450000 | 0.010000 | 1.460000 | 1.487024 |
+----------+---------+----------+----------+----------+----------+
unpack
/Users/aaron/.rbenv/versions/2.4.0/lib/ruby/gems/2.4.0/gems/myrrha-1.2.2/lib/myrrha/to_ruby_literal.rb:6: warning: constant ::Fixnum is deprecated
/Users/aaron/.rbenv/versions/2.4.0/lib/ruby/gems/2.4.0/gems/myrrha-1.2.2/lib/myrrha/to_ruby_literal.rb:6: warning: constant ::Bignum is deprecated
+----------+---------+----------+----------+----------+----------+
| :bench   | :runs   | :user    | :system  | :total   | :real    |
+----------+---------+----------+----------+----------+----------+
| :strings |   10000 | 0.040000 | 0.010000 | 0.050000 | 0.054127 |
| :strings |  100000 | 0.360000 | 0.020000 | 0.380000 | 0.415750 |
| :strings | 1000000 | 3.170000 | 0.080000 | 3.250000 | 3.341190 |
| :symbols |   10000 | 0.030000 | 0.010000 | 0.040000 | 0.039577 |
| :symbols |  100000 | 0.390000 | 0.010000 | 0.400000 | 0.427426 |
| :symbols | 1000000 | 3.400000 | 0.100000 | 3.500000 | 3.561844 |
+----------+---------+----------+----------+----------+----------+
pack log
/Users/aaron/.rbenv/versions/2.4.0/lib/ruby/gems/2.4.0/gems/myrrha-1.2.2/lib/myrrha/to_ruby_literal.rb:6: warning: constant ::Fixnum is deprecated
/Users/aaron/.rbenv/versions/2.4.0/lib/ruby/gems/2.4.0/gems/myrrha-1.2.2/lib/myrrha/to_ruby_literal.rb:6: warning: constant ::Bignum is deprecated
+------------+---------+----------+----------+----------+----------+
| :bench     | :runs   | :user    | :system  | :total   | :real    |
+------------+---------+----------+----------+----------+----------+
| :plain     |   10000 | 0.010000 | 0.000000 | 0.010000 | 0.014956 |
| :plain     |  100000 | 0.140000 | 0.000000 | 0.140000 | 0.147023 |
| :plain     | 1000000 | 1.570000 | 0.040000 | 1.610000 | 1.833784 |
| :structure |   10000 | 0.030000 | 0.010000 | 0.040000 | 0.029624 |
| :structure |  100000 | 0.190000 | 0.010000 | 0.200000 | 0.200059 |
| :structure | 1000000 | 1.820000 | 0.060000 | 1.880000 | 1.944721 |
+------------+---------+----------+----------+----------+----------+
unpack log
/Users/aaron/.rbenv/versions/2.4.0/lib/ruby/gems/2.4.0/gems/myrrha-1.2.2/lib/myrrha/to_ruby_literal.rb:6: warning: constant ::Fixnum is deprecated
/Users/aaron/.rbenv/versions/2.4.0/lib/ruby/gems/2.4.0/gems/myrrha-1.2.2/lib/myrrha/to_ruby_literal.rb:6: warning: constant ::Bignum is deprecated
+------------+---------+----------+----------+----------+----------+
| :bench     | :runs   | :user    | :system  | :total   | :real    |
+------------+---------+----------+----------+----------+----------+
| :plain     |   10000 | 0.020000 | 0.020000 | 0.040000 | 0.045669 |
| :plain     |  100000 | 0.260000 | 0.030000 | 0.290000 | 0.304270 |
| :plain     | 1000000 | 2.450000 | 0.230000 | 2.680000 | 2.719803 |
| :structure |   10000 | 0.070000 | 0.010000 | 0.080000 | 0.072225 |
| :structure |  100000 | 0.620000 | 0.000000 | 0.620000 | 0.631887 |
| :structure | 1000000 | 6.320000 | 0.060000 | 6.380000 | 6.496013 |
+------------+---------+----------+----------+----------+----------+

[tagomoris]

There looks a bit performance difference. IMO it's not serious problem, and the benefit from moving code from C to Ruby is much larger than this performance difference.
@frsyuki How do you feel?

[iconara]

This does not seem to impact JRuby performance significantly. +1 for moving code to Ruby when not impacting performance.

The PR seems to target JRuby 9k exclusively though, is that by design or by accident?