From 65c360a2cab5684c9f2059937c314ffde0ff0587 Mon Sep 17 00:00:00 2001 From: Dirkjan Bussink Date: Sat, 17 Mar 2012 11:28:19 +0100 Subject: [PATCH 1/3] Don't use MRI internals in the Ruby extension Using internals of MRI by using RARRAY_PTR makes it necessary for other implementations such as Rubinius to continuously copy the structure returned by RARRAY_PTR back and forth since in Rubinius objects are layed out differently internally. Extensions should not depend and use these internal MRI structures if this is not necessary and when there are API methods that can provide the same functionality. This makes sure other implementations can also use the extension without any big problems. For this reason I also removed the FIXME comment, since that change would also heavily depend on the internal memory layout of objects on MRI. --- ruby/pack.c | 11 ++++++----- ruby/unpack.c | 2 +- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/ruby/pack.c b/ruby/pack.c index c0e135c8..826f4e27 100644 --- a/ruby/pack.c +++ b/ruby/pack.c @@ -219,11 +219,12 @@ static VALUE MessagePack_Array_to_msgpack(int argc, VALUE *argv, VALUE self) { ARG_BUFFER(out, argc, argv); // FIXME check sizeof(long) > sizeof(unsigned int) && RARRAY_LEN(self) > UINT_MAX - msgpack_pack_array(out, (unsigned int)RARRAY_LEN(self)); - VALUE* p = RARRAY_PTR(self); - VALUE* const pend = p + RARRAY_LEN(self); - for(;p != pend; ++p) { - rb_funcall(*p, s_to_msgpack, 1, out); + unsigned int ary_length = (unsigned int)RARRAY_LEN(self); + unsigned int i = 0; + msgpack_pack_array(out, ary_length); + for(; i < ary_length; ++i) { + VALUE p = rb_ary_entry(self, i); + rb_funcall(p, s_to_msgpack, 1, out); } return out; } diff --git a/ruby/unpack.c b/ruby/unpack.c index f61fe6db..a3bb861d 100644 --- a/ruby/unpack.c +++ b/ruby/unpack.c @@ -112,7 +112,7 @@ static inline int template_callback_array(unpack_user* u, unsigned int n, VALUE* { *o = rb_ary_new2(n); return 0; } static inline int template_callback_array_item(unpack_user* u, VALUE* c, VALUE o) -{ rb_ary_push(*c, o); return 0; } // FIXME set value directry RARRAY_PTR(obj)[RARRAY_LEN(obj)++] +{ rb_ary_push(*c, o); return 0; } static inline int template_callback_map(unpack_user* u, unsigned int n, VALUE* o) { *o = rb_hash_new(); return 0; } From 4a0d7f18fdd0b3c0d9aa45f73aed90be833e143f Mon Sep 17 00:00:00 2001 From: Dirkjan Bussink Date: Sat, 17 Mar 2012 12:40:29 +0100 Subject: [PATCH 2/3] Explicitly state msgpack doesn't modify char* buffers from RSTRING_PTR From what I could investigate, msgpack doesn't modify char* buffers obtained from RSTRING_PTR. This means that on Rubinius we don't have to copy back and forth the buffer to make sure it's also updated on the Ruby side. This copying of buffers is a similar problem as the RARRAY_PTR problem, because it is not safe to expose GC'ed memory on Rubinius to extensions since it can move due to Rubinius having a moving GC. --- ruby/pack.c | 1 + 1 file changed, 1 insertion(+) diff --git a/ruby/pack.c b/ruby/pack.c index 826f4e27..35717090 100644 --- a/ruby/pack.c +++ b/ruby/pack.c @@ -15,6 +15,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +#define RSTRING_NOT_MODIFIED #include "ruby.h" #include "compat.h" From bf18e04134f674830cbb96bac4c58fbcaca2854f Mon Sep 17 00:00:00 2001 From: Dirkjan Bussink Date: Sat, 17 Mar 2012 12:43:26 +0100 Subject: [PATCH 3/3] Detect whether st.h is present and don't use RUBY_VM as the condition --- ruby/extconf.rb | 1 + ruby/pack.c | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/ruby/extconf.rb b/ruby/extconf.rb index 10a807e7..4082a92f 100644 --- a/ruby/extconf.rb +++ b/ruby/extconf.rb @@ -1,5 +1,6 @@ require 'mkmf' require './version.rb' $CFLAGS << %[ -I.. -Wall -O3 -DMESSAGEPACK_VERSION=\\"#{MessagePack::VERSION}\\" -g] +have_header("ruby/st.h") create_makefile('msgpack') diff --git a/ruby/pack.c b/ruby/pack.c index 35717090..c09bf0f1 100644 --- a/ruby/pack.c +++ b/ruby/pack.c @@ -40,8 +40,8 @@ static ID s_append; #include "msgpack/pack_template.h" -#ifndef RUBY_VM -#include "st.h" // ruby hash +#ifdef HAVE_RUBY_ST_H +#include "ruby/st.h" // ruby hash #endif #define ARG_BUFFER(name, argc, argv) \