From 15476c695d708796f2271d99deb3cd5288d016d2 Mon Sep 17 00:00:00 2001 From: normal Date: Mon, 23 Nov 2015 21:20:56 +0000 Subject: fiddle: release GVL for ffi_call Some external functions I wish to call may take a long time and unnecessarily block other threads. This may lead to performance regressions for fast functions as releasing/acquiring the GVL is not cheap, but can improve performance for long-running functions in multi-threaded applications. This also means we must reacquire the GVL when calling Ruby-defined callbacks for Fiddle::Closure, meaning we must detect whether the current thread has the GVL by exporting ruby_thread_has_gvl_p in internal.h * ext/fiddle/function.c (struct nogvl_ffi_call_args): new struct for GVL release (nogvl_ffi_call): new function (function_call): adjust for GVL release [ruby-core:71642] [Feature #11607] * ext/fiddle/closure.c (struct callback_args): new struct for GVL acquire (with_gvl_callback): adjusted original callback function (callback): wrapper for conditional GVL acquire * ext/fiddle/depend: add dependencies * ext/fiddle/extconf.rb: include top_srcdir for internal.h * internal.h (ruby_thread_has_gvl_p): expose for fiddle * vm_core.h (ruby_thread_has_gvl_p): moved to internal.h * test/fiddle/test_function.rb (test_nogvl_poll): new test git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@52723 b2dd03c8-39d4-4d8f-98ff-823fe69b080e --- ChangeLog | 17 ++++++++++ ext/fiddle/closure.c | 79 ++++++++++++++++++++++++++++++-------------- ext/fiddle/depend | 4 ++- ext/fiddle/extconf.rb | 1 + ext/fiddle/function.c | 36 +++++++++++++++----- internal.h | 3 ++ test/fiddle/test_function.rb | 19 +++++++++++ vm_core.h | 1 - 8 files changed, 124 insertions(+), 36 deletions(-) diff --git a/ChangeLog b/ChangeLog index dea9b0f967..7a7e8b3f4b 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,20 @@ +Tue Nov 24 05:13:35 2015 Eric Wong + + * ext/fiddle/function.c (struct nogvl_ffi_call_args): + new struct for GVL release + (nogvl_ffi_call): new function + (function_call): adjust for GVL release + [ruby-core:71642] [Feature #11607] + * ext/fiddle/closure.c (struct callback_args): + new struct for GVL acquire + (with_gvl_callback): adjusted original callback function + (callback): wrapper for conditional GVL acquire + * ext/fiddle/depend: add dependencies + * ext/fiddle/extconf.rb: include top_srcdir for internal.h + * internal.h (ruby_thread_has_gvl_p): expose for fiddle + * vm_core.h (ruby_thread_has_gvl_p): moved to internal.h + * test/fiddle/test_function.rb (test_nogvl_poll): new test + Mon Nov 23 19:53:12 2015 Naohisa Goto * configure.in: On Solaris, with gcc, "-std=iso9899:1999" diff --git a/ext/fiddle/closure.c b/ext/fiddle/closure.c index 5d8a3d200f..2263b410e8 100644 --- a/ext/fiddle/closure.c +++ b/ext/fiddle/closure.c @@ -1,4 +1,6 @@ #include +#include +#include "internal.h" /* rb_thread_has_gvl_p */ VALUE cFiddleClosure; @@ -55,10 +57,19 @@ const rb_data_type_t closure_data_type = { {0, dealloc, closure_memsize,}, }; -static void -callback(ffi_cif *cif, void *resp, void **args, void *ctx) +struct callback_args { + ffi_cif *cif; + void *resp; + void **args; + void *ctx; +}; + +static void * +with_gvl_callback(void *ptr) { - VALUE self = (VALUE)ctx; + struct callback_args *x = ptr; + + VALUE self = (VALUE)x->ctx; VALUE rbargs = rb_iv_get(self, "@args"); VALUE ctype = rb_iv_get(self, "@ctype"); int argc = RARRAY_LENINT(rbargs); @@ -76,46 +87,46 @@ callback(ffi_cif *cif, void *resp, void **args, void *ctx) argc = 0; break; case TYPE_INT: - rb_ary_push(params, INT2NUM(*(int *)args[i])); + rb_ary_push(params, INT2NUM(*(int *)x->args[i])); break; case -TYPE_INT: - rb_ary_push(params, UINT2NUM(*(unsigned int *)args[i])); + rb_ary_push(params, UINT2NUM(*(unsigned int *)x->args[i])); break; case TYPE_VOIDP: rb_ary_push(params, rb_funcall(cPointer, rb_intern("[]"), 1, - PTR2NUM(*(void **)args[i]))); + PTR2NUM(*(void **)x->args[i]))); break; case TYPE_LONG: - rb_ary_push(params, LONG2NUM(*(long *)args[i])); + rb_ary_push(params, LONG2NUM(*(long *)x->args[i])); break; case -TYPE_LONG: - rb_ary_push(params, ULONG2NUM(*(unsigned long *)args[i])); + rb_ary_push(params, ULONG2NUM(*(unsigned long *)x->args[i])); break; case TYPE_CHAR: - rb_ary_push(params, INT2NUM(*(signed char *)args[i])); + rb_ary_push(params, INT2NUM(*(signed char *)x->args[i])); break; case -TYPE_CHAR: - rb_ary_push(params, UINT2NUM(*(unsigned char *)args[i])); + rb_ary_push(params, UINT2NUM(*(unsigned char *)x->args[i])); break; case TYPE_SHORT: - rb_ary_push(params, INT2NUM(*(signed short *)args[i])); + rb_ary_push(params, INT2NUM(*(signed short *)x->args[i])); break; case -TYPE_SHORT: - rb_ary_push(params, UINT2NUM(*(unsigned short *)args[i])); + rb_ary_push(params, UINT2NUM(*(unsigned short *)x->args[i])); break; case TYPE_DOUBLE: - rb_ary_push(params, rb_float_new(*(double *)args[i])); + rb_ary_push(params, rb_float_new(*(double *)x->args[i])); break; case TYPE_FLOAT: - rb_ary_push(params, rb_float_new(*(float *)args[i])); + rb_ary_push(params, rb_float_new(*(float *)x->args[i])); break; #if HAVE_LONG_LONG case TYPE_LONG_LONG: - rb_ary_push(params, LL2NUM(*(LONG_LONG *)args[i])); + rb_ary_push(params, LL2NUM(*(LONG_LONG *)x->args[i])); break; case -TYPE_LONG_LONG: - rb_ary_push(params, ULL2NUM(*(unsigned LONG_LONG *)args[i])); + rb_ary_push(params, ULL2NUM(*(unsigned LONG_LONG *)x->args[i])); break; #endif default: @@ -131,41 +142,59 @@ callback(ffi_cif *cif, void *resp, void **args, void *ctx) case TYPE_VOID: break; case TYPE_LONG: - *(long *)resp = NUM2LONG(ret); + *(long *)x->resp = NUM2LONG(ret); break; case -TYPE_LONG: - *(unsigned long *)resp = NUM2ULONG(ret); + *(unsigned long *)x->resp = NUM2ULONG(ret); break; case TYPE_CHAR: case TYPE_SHORT: case TYPE_INT: - *(ffi_sarg *)resp = NUM2INT(ret); + *(ffi_sarg *)x->resp = NUM2INT(ret); break; case -TYPE_CHAR: case -TYPE_SHORT: case -TYPE_INT: - *(ffi_arg *)resp = NUM2UINT(ret); + *(ffi_arg *)x->resp = NUM2UINT(ret); break; case TYPE_VOIDP: - *(void **)resp = NUM2PTR(ret); + *(void **)x->resp = NUM2PTR(ret); break; case TYPE_DOUBLE: - *(double *)resp = NUM2DBL(ret); + *(double *)x->resp = NUM2DBL(ret); break; case TYPE_FLOAT: - *(float *)resp = (float)NUM2DBL(ret); + *(float *)x->resp = (float)NUM2DBL(ret); break; #if HAVE_LONG_LONG case TYPE_LONG_LONG: - *(LONG_LONG *)resp = NUM2LL(ret); + *(LONG_LONG *)x->resp = NUM2LL(ret); break; case -TYPE_LONG_LONG: - *(unsigned LONG_LONG *)resp = NUM2ULL(ret); + *(unsigned LONG_LONG *)x->resp = NUM2ULL(ret); break; #endif default: rb_raise(rb_eRuntimeError, "closure retval: %d", type); } + return 0; +} + +static void +callback(ffi_cif *cif, void *resp, void **args, void *ctx) +{ + struct callback_args x; + + x.cif = cif; + x.resp = resp; + x.args = args; + x.ctx = ctx; + + if (ruby_thread_has_gvl_p()) { + (void)with_gvl_callback(&x); + } else { + (void)rb_thread_call_with_gvl(with_gvl_callback, &x); + } } static VALUE diff --git a/ext/fiddle/depend b/ext/fiddle/depend index 0052787934..bf264a18eb 100644 --- a/ext/fiddle/depend +++ b/ext/fiddle/depend @@ -9,7 +9,9 @@ CONFIGURE_LIBFFI = \ $(OBJS): $(HDRS) $(ruby_headers) \ $(hdrdir)/ruby/io.h \ $(hdrdir)/ruby/encoding.h \ - $(hdrdir)/ruby/oniguruma.h + $(hdrdir)/ruby/oniguruma.h \ + $(hdrdir)/ruby/thread.h \ + $(top_srcdir)/internal.h $(STATIC_LIB) $(RUBYARCHDIR)/$(DLLIB) $(DLLIB): $(LIBFFI_A) diff --git a/ext/fiddle/extconf.rb b/ext/fiddle/extconf.rb index 5cb638611e..c264a33429 100644 --- a/ext/fiddle/extconf.rb +++ b/ext/fiddle/extconf.rb @@ -143,6 +143,7 @@ if libffi $LOCAL_LIBS.prepend("./#{libffi.a} ").strip! # to exts.mk $INCFLAGS.gsub!(/-I#{libffi.dir}/, '-I$(LIBFFI_DIR)') end +$INCFLAGS << " -I$(top_srcdir)" create_makefile 'fiddle' do |conf| if !libffi next conf << "LIBFFI_CLEAN = none\n" diff --git a/ext/fiddle/function.c b/ext/fiddle/function.c index 209c03f71e..164e5f3df6 100644 --- a/ext/fiddle/function.c +++ b/ext/fiddle/function.c @@ -1,4 +1,5 @@ #include +#include #ifdef PRIsVALUE # define RB_OBJ_CLASSNAME(obj) rb_obj_class(obj) @@ -128,13 +129,28 @@ initialize(int argc, VALUE argv[], VALUE self) return self; } +struct nogvl_ffi_call_args { + ffi_cif *cif; + void (*fn)(void); + void **values; + fiddle_generic retval; +}; + +static void * +nogvl_ffi_call(void *ptr) +{ + struct nogvl_ffi_call_args *args = ptr; + + ffi_call(args->cif, args->fn, &args->retval, args->values); + + return NULL; +} + static VALUE function_call(int argc, VALUE argv[], VALUE self) { - ffi_cif * cif; - fiddle_generic retval; + struct nogvl_ffi_call_args args = { 0 }; fiddle_generic *generic_args; - void **values; VALUE cfunc, types, cPointer; int i; VALUE alloc_buffer = 0; @@ -148,7 +164,7 @@ function_call(int argc, VALUE argv[], VALUE self) rb_error_arity(argc, i, i); } - TypedData_Get_Struct(self, ffi_cif, &function_data_type, cif); + TypedData_Get_Struct(self, ffi_cif, &function_data_type, args.cif); if (rb_safe_level() >= 1) { for (i = 0; i < argc; i++) { @@ -161,7 +177,8 @@ function_call(int argc, VALUE argv[], VALUE self) generic_args = ALLOCV(alloc_buffer, (size_t)(argc + 1) * sizeof(void *) + (size_t)argc * sizeof(fiddle_generic)); - values = (void **)((char *)generic_args + (size_t)argc * sizeof(fiddle_generic)); + args.values = (void **)((char *)generic_args + + (size_t)argc * sizeof(fiddle_generic)); for (i = 0; i < argc; i++) { VALUE type = RARRAY_AREF(types, i); @@ -177,11 +194,12 @@ function_call(int argc, VALUE argv[], VALUE self) } VALUE2GENERIC(NUM2INT(type), src, &generic_args[i]); - values[i] = (void *)&generic_args[i]; + args.values[i] = (void *)&generic_args[i]; } - values[argc] = NULL; + args.values[argc] = NULL; + args.fn = NUM2PTR(rb_Integer(cfunc)); - ffi_call(cif, NUM2PTR(rb_Integer(cfunc)), &retval, values); + (void)rb_thread_call_without_gvl(nogvl_ffi_call, &args, 0, 0); rb_funcall(mFiddle, rb_intern("last_error="), 1, INT2NUM(errno)); #if defined(_WIN32) @@ -190,7 +208,7 @@ function_call(int argc, VALUE argv[], VALUE self) ALLOCV_END(alloc_buffer); - return GENERIC2VALUE(rb_iv_get(self, "@return_type"), retval); + return GENERIC2VALUE(rb_iv_get(self, "@return_type"), args.retval); } void diff --git a/internal.h b/internal.h index 7e6f26db11..138f718cd1 100644 --- a/internal.h +++ b/internal.h @@ -1320,6 +1320,9 @@ VALUE rb_gcd_gmp(VALUE x, VALUE y); VALUE rb_setup_fake_str(struct RString *fake_str, const char *name, long len, rb_encoding *enc); #endif +/* thread.c (export) */ +int ruby_thread_has_gvl_p(void); /* for ext/fiddle/closure.c */ + /* util.c (export) */ extern const signed char ruby_digit36_to_number_table[]; extern const char ruby_hexdigits[]; diff --git a/test/fiddle/test_function.rb b/test/fiddle/test_function.rb index 2e3a432ec3..9351f961df 100644 --- a/test/fiddle/test_function.rb +++ b/test/fiddle/test_function.rb @@ -73,6 +73,25 @@ module Fiddle assert_equal("123", str.to_s) end + def test_nogvl_poll + begin + poll = @libc['poll'] + rescue Fiddle::DLError + skip 'poll(2) not available' + end + f = Function.new(poll, [TYPE_VOIDP, TYPE_INT, TYPE_INT], TYPE_INT) + + msec = 200 + t0 = Process.clock_gettime(Process::CLOCK_MONOTONIC, :millisecond) + th = Thread.new { f.call(nil, 0, msec) } + n1 = f.call(nil, 0, msec) + n2 = th.value + t1 = Process.clock_gettime(Process::CLOCK_MONOTONIC, :millisecond) + assert_in_delta(msec, t1 - t0, 100, 'slept correct amount of time') + assert_equal(0, n1, 'poll(2) called correctly main-thread') + assert_equal(0, n2, 'poll(2) called correctly in sub-thread') + end + def test_no_memory_leak prep = 'r = Fiddle::Function.new(Fiddle.dlopen(nil)["rb_obj_tainted"], [Fiddle::TYPE_UINTPTR_T], Fiddle::TYPE_UINTPTR_T); a = "a"' code = 'begin r.call(a); rescue TypeError; end' diff --git a/vm_core.h b/vm_core.h index aacdd3b897..da1455266d 100644 --- a/vm_core.h +++ b/vm_core.h @@ -1036,7 +1036,6 @@ rb_vm_living_threads_remove(rb_vm_t *vm, rb_thread_t *th) vm->living_thread_num--; } -int ruby_thread_has_gvl_p(void); typedef int rb_backtrace_iter_func(void *, VALUE, int, VALUE); rb_control_frame_t *rb_vm_get_ruby_level_next_cfp(const rb_thread_t *th, const rb_control_frame_t *cfp); rb_control_frame_t *rb_vm_get_binding_creatable_next_cfp(const rb_thread_t *th, const rb_control_frame_t *cfp); -- cgit v1.2.3