* [PATCH] Don't use deprecated api @ 2013-12-26 19:57 Hleb Valoshka 2013-12-27 0:19 ` Eric Wong 2014-01-16 23:56 ` Eric Wong 0 siblings, 2 replies; 9+ messages in thread From: Hleb Valoshka @ 2013-12-26 19:57 UTC (permalink / raw) To: kgio This patch allows to use current Ruby API (rb_thread_call_without_gvl) instead of deprecated `rb_thread_blocking_region'. --- ext/kgio/accept.c | 7 +++++-- ext/kgio/blocking_io_region.h | 3 +++ ext/kgio/extconf.rb | 8 ++++++-- ext/kgio/tryopen.c | 6 +++++- 4 files changed, 19 insertions(+), 5 deletions(-) diff --git a/ext/kgio/accept.c b/ext/kgio/accept.c index 67a8ded..ebfcf54 100644 --- a/ext/kgio/accept.c +++ b/ext/kgio/accept.c @@ -10,7 +10,9 @@ static VALUE cKgio_Socket; static VALUE mSocketMethods; static VALUE iv_kgio_addr; -#if defined(__linux__) && defined(HAVE_RB_THREAD_BLOCKING_REGION) +#if defined(__linux__) && \ + (defined(HAVE_RB_THREAD_BLOCKING_REGION) || \ + defined(HAVE_RB_THREAD_CALL_WITHOUT_GVL)) static int accept4_flags = SOCK_CLOEXEC; #else /* ! linux */ static int accept4_flags = SOCK_CLOEXEC | SOCK_NONBLOCK; @@ -76,7 +78,8 @@ static VALUE xaccept(void *ptr) return (VALUE)rv; } -#ifdef HAVE_RB_THREAD_BLOCKING_REGION +#if defined HAVE_RB_THREAD_BLOCKING_REGION || \ + defined HAVE_RB_THREAD_CALL_WITHOUT_GVL # include <time.h> # include "blocking_io_region.h" static int thread_accept(struct accept_args *a, int force_nonblock) diff --git a/ext/kgio/blocking_io_region.h b/ext/kgio/blocking_io_region.h index 30c7106..2a17c05 100644 --- a/ext/kgio/blocking_io_region.h +++ b/ext/kgio/blocking_io_region.h @@ -6,4 +6,7 @@ VALUE rb_thread_io_blocking_region(rb_blocking_function_t *, void *, int); # define rb_thread_io_blocking_region(fn,data,fd) \ rb_thread_blocking_region((fn),(data),RUBY_UBF_IO,0) # endif +#else +# define rb_thread_io_blocking_region(fn,data,fd) \ + rb_thread_call_without_gvl((fn),(data),RUBY_UBF_IO,0) #endif diff --git a/ext/kgio/extconf.rb b/ext/kgio/extconf.rb index 5fb15ac..eaf1af2 100644 --- a/ext/kgio/extconf.rb +++ b/ext/kgio/extconf.rb @@ -46,8 +46,12 @@ have_func('rb_io_ascii8bit_binmode') have_func('rb_update_max_fd') have_func('rb_fd_fix_cloexec') have_func('rb_cloexec_open') -have_func('rb_thread_blocking_region') -have_func('rb_thread_io_blocking_region') +if have_header('ruby/thread.h') + have_func('rb_thread_call_without_gvl') +else + have_func('rb_thread_blocking_region') + have_func('rb_thread_io_blocking_region') +end have_func('rb_str_set_len') have_func('rb_time_interval') have_func('rb_wait_for_single_fd') diff --git a/ext/kgio/tryopen.c b/ext/kgio/tryopen.c index 902c745..bcf443d 100644 --- a/ext/kgio/tryopen.c +++ b/ext/kgio/tryopen.c @@ -38,7 +38,7 @@ static VALUE nogvl_open(void *ptr) return (VALUE)rb_cloexec_open(o->pathname, o->flags, o->mode); } -#ifndef HAVE_RB_THREAD_BLOCKING_REGION +#if !defined HAVE_RB_THREAD_BLOCKING_REGION && !defined RUBY_UBF_IO # define RUBY_UBF_IO ((void *)(-1)) # include "rubysig.h" typedef void rb_unblock_function_t(void *); @@ -105,7 +105,11 @@ static VALUE s_tryopen(int argc, VALUE *argv, VALUE klass) } retry: +#ifdef HAVE_RB_THREAD_CALL_WITHOUT_GVL + fd = (int)rb_thread_call_without_gvl(nogvl_open, &o, RUBY_UBF_IO, 0); +#else fd = (int)rb_thread_blocking_region(nogvl_open, &o, RUBY_UBF_IO, 0); +#endif if (fd < 0) { if (errno == EMFILE || errno == ENFILE || errno == ENOMEM) { rb_gc(); -- 1.8.5.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] Don't use deprecated api 2013-12-26 19:57 [PATCH] Don't use deprecated api Hleb Valoshka @ 2013-12-27 0:19 ` Eric Wong 2014-01-16 23:56 ` Eric Wong 1 sibling, 0 replies; 9+ messages in thread From: Eric Wong @ 2013-12-27 0:19 UTC (permalink / raw) To: kgio Hleb Valoshka <375gnu@gmail.com> wrote: > This patch allows to use current Ruby API (rb_thread_call_without_gvl) > instead of deprecated `rb_thread_blocking_region'. Thanks. I agree in principle, but the implementation needs to be tweaked. Comments inline. > ext/kgio/accept.c | 7 +++++-- > ext/kgio/blocking_io_region.h | 3 +++ > ext/kgio/extconf.rb | 8 ++++++-- > ext/kgio/tryopen.c | 6 +++++- > 4 files changed, 19 insertions(+), 5 deletions(-) connect.c needs to be updated for the MSG_FASTOPEN code. I can do it if you can't test TFO on your system > static VALUE mSocketMethods; > static VALUE iv_kgio_addr; > > -#if defined(__linux__) && defined(HAVE_RB_THREAD_BLOCKING_REGION) > +#if defined(__linux__) && \ > + (defined(HAVE_RB_THREAD_BLOCKING_REGION) || \ > + defined(HAVE_RB_THREAD_CALL_WITHOUT_GVL)) > static int accept4_flags = SOCK_CLOEXEC; > #else /* ! linux */ > static int accept4_flags = SOCK_CLOEXEC | SOCK_NONBLOCK; > @@ -76,7 +78,8 @@ static VALUE xaccept(void *ptr) > return (VALUE)rv; > } > > -#ifdef HAVE_RB_THREAD_BLOCKING_REGION > +#if defined HAVE_RB_THREAD_BLOCKING_REGION || \ > + defined HAVE_RB_THREAD_CALL_WITHOUT_GVL We should probably have a KGIO_ macro for both of these combined, so it'll be harder to miss pieces like connect.c > diff --git a/ext/kgio/extconf.rb b/ext/kgio/extconf.rb > index 5fb15ac..eaf1af2 100644 > --- a/ext/kgio/extconf.rb > +++ b/ext/kgio/extconf.rb > @@ -46,8 +46,12 @@ have_func('rb_io_ascii8bit_binmode') > have_func('rb_update_max_fd') > have_func('rb_fd_fix_cloexec') > have_func('rb_cloexec_open') > -have_func('rb_thread_blocking_region') > -have_func('rb_thread_io_blocking_region') > +if have_header('ruby/thread.h') > + have_func('rb_thread_call_without_gvl') > +else > + have_func('rb_thread_blocking_region') > + have_func('rb_thread_io_blocking_region') > +end Even though rb_thread_io_blocking_region is not officially in the public API, it is superior when an FD is known as it allows blocked threads to be woken/interrupted on IO#close The socket stdlib uses it (as does all the core IO methods). We should check for all of these rb_thread_* functions unconditionally, since sometimes these HAVE_* macros may be defined in headers regardless of what we do in extconf.rb, so we shouldn't remove/change existing checks for HAVE_* macros: > -#ifndef HAVE_RB_THREAD_BLOCKING_REGION > +#if !defined HAVE_RB_THREAD_BLOCKING_REGION && !defined RUBY_UBF_IO > # define RUBY_UBF_IO ((void *)(-1)) > # include "rubysig.h" > typedef void rb_unblock_function_t(void *); > @@ -105,7 +105,11 @@ static VALUE s_tryopen(int argc, VALUE *argv, VALUE klass) > } > retry: > +#ifdef HAVE_RB_THREAD_CALL_WITHOUT_GVL > + fd = (int)rb_thread_call_without_gvl(nogvl_open, &o, RUBY_UBF_IO, 0); > +#else > fd = (int)rb_thread_blocking_region(nogvl_open, &o, RUBY_UBF_IO, 0); > +#endif Please define a wrapper macro, I intensely dislike #ifdef inside function bodies. I cannot stand reading some MRI code because of this; my preferred C style is from the Linux kernel (Documentation/CodingStyle). ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Don't use deprecated api 2013-12-26 19:57 [PATCH] Don't use deprecated api Hleb Valoshka 2013-12-27 0:19 ` Eric Wong @ 2014-01-16 23:56 ` Eric Wong 2014-01-17 8:37 ` Hleb Valoshka 1 sibling, 1 reply; 9+ messages in thread From: Eric Wong @ 2014-01-16 23:56 UTC (permalink / raw) To: kgio Btw, are you planning on providing a followup patch after my comments? I am interested in integrating this, thanks. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Don't use deprecated api 2014-01-16 23:56 ` Eric Wong @ 2014-01-17 8:37 ` Hleb Valoshka 2014-01-19 19:42 ` Hleb Valoshka 0 siblings, 1 reply; 9+ messages in thread From: Hleb Valoshka @ 2014-01-17 8:37 UTC (permalink / raw) To: kgio On 1/17/14, Eric Wong <normalperson@yhbt.net> wrote: > Btw, are you planning on providing a followup patch after > my comments? I am interested in integrating this, thanks. Yes, but I currently busy and have no enough time to do that :( ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Don't use deprecated api 2014-01-17 8:37 ` Hleb Valoshka @ 2014-01-19 19:42 ` Hleb Valoshka 2014-01-19 23:13 ` Eric Wong 0 siblings, 1 reply; 9+ messages in thread From: Hleb Valoshka @ 2014-01-19 19:42 UTC (permalink / raw) To: kgio The patch is almost done (it was tested with mri1.8, 1.9.3 and 2.0), but before I finish it, I should ask about correctness of the following patch. Is this replacement is correct? commit a6e30828439d5fb194ffe74b26a0ad06b96057c9 Author: Hleb Valoshka <375gnu@gmail.com> Date: Sun Jan 19 22:35:11 2014 +0300 use rb_thread_io_blocking_region dunno whether this is correct replacement diff --git a/ext/kgio/poll.c b/ext/kgio/poll.c index 15774d8..5573247 100644 --- a/ext/kgio/poll.c +++ b/ext/kgio/poll.c @@ -5,6 +5,7 @@ #include "broken_system_compat.h" #include <poll.h> #include <errno.h> +#include "blocking_io_region.h" #ifdef HAVE_RUBY_ST_H # include <ruby/st.h> #else @@ -145,7 +146,7 @@ static VALUE do_poll(VALUE args) retry: hash2pollfds(a); - nr = (int)rb_thread_blocking_region(nogvl_poll, a, RUBY_UBF_IO, NULL); + nr = (int)rb_thread_io_blocking_region(nogvl_poll, a, 0); if (nr < 0) { if (interrupted()) { if (retryable(a)) { ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] Don't use deprecated api 2014-01-19 19:42 ` Hleb Valoshka @ 2014-01-19 23:13 ` Eric Wong 2014-01-20 19:35 ` Hleb Valoshka 0 siblings, 1 reply; 9+ messages in thread From: Eric Wong @ 2014-01-19 23:13 UTC (permalink / raw) To: kgio Hleb Valoshka <375gnu@gmail.com> wrote: > The patch is almost done (it was tested with mri1.8, 1.9.3 and 2.0), > but before I finish it, I should ask about correctness of the > following patch. Is this replacement is correct? Not exactly, but it actually works... The FD arg (0, stdin) means Ruby will wake up the polling thread if that FD is closed, however: 1) poll may not be watching FD=0 2) things tend to break if you trie to close the standard FDs, so Ruby (MRI at least) doesn't actually close them I might be inclined to accept this if there's a comment about the fd=0 being a dummy arg as well as the reasoning of using this function vs rb_thread_call_without_gvl or rb_thread_blocking_region because the former is detectable-but-unlinkable in 1.9.3, while the later is deprecated in 2.x > retry: > hash2pollfds(a); > - nr = (int)rb_thread_blocking_region(nogvl_poll, a, RUBY_UBF_IO, NULL); > + nr = (int)rb_thread_io_blocking_region(nogvl_poll, a, 0); > if (nr < 0) { > if (interrupted()) { > if (retryable(a)) { ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Don't use deprecated api 2014-01-19 23:13 ` Eric Wong @ 2014-01-20 19:35 ` Hleb Valoshka 2014-01-20 19:36 ` Hleb Valoshka 0 siblings, 1 reply; 9+ messages in thread From: Hleb Valoshka @ 2014-01-20 19:35 UTC (permalink / raw) To: kgio On 1/20/14, Eric Wong <normalperson@yhbt.net> wrote: > I might be inclined to accept this if there's a comment about the fd=0 > being a dummy arg as well as the reasoning of using this function vs > rb_thread_call_without_gvl or rb_thread_blocking_region because the > former is detectable-but-unlinkable in 1.9.3, while the later is > deprecated in 2.x So it seems to be better not to touch it. The next message is updated patch, I've tested it with ruby 1.8.7p358, 1.9.3p484, 2.0.0p353, 2.1.0p0 and rubinius 2.0.0.n257. With mri 2.x it uses rb_thread_call_without_gvl and rb_thread_io_blocking_region, rb_thread_blocking_region and rb_thread_io_blocking_region with 1.9.3, rb_thread_call_without_gvl with rbx and old functions with 1.8.7. Waiting for comments. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] Don't use deprecated api 2014-01-20 19:35 ` Hleb Valoshka @ 2014-01-20 19:36 ` Hleb Valoshka 2014-01-20 21:12 ` Eric Wong 0 siblings, 1 reply; 9+ messages in thread From: Hleb Valoshka @ 2014-01-20 19:36 UTC (permalink / raw) To: kgio --- ext/kgio/accept.c | 8 ++++---- ext/kgio/blocking_io_region.h | 9 ++++++--- ext/kgio/connect.c | 4 ++-- ext/kgio/extconf.rb | 2 ++ ext/kgio/kgio.h | 14 +++++++++++++- ext/kgio/tryopen.c | 5 +++-- 6 files changed, 30 insertions(+), 12 deletions(-) diff --git a/ext/kgio/accept.c b/ext/kgio/accept.c index 67a8ded..911e169 100644 --- a/ext/kgio/accept.c +++ b/ext/kgio/accept.c @@ -10,7 +10,7 @@ static VALUE cKgio_Socket; static VALUE mSocketMethods; static VALUE iv_kgio_addr; -#if defined(__linux__) && defined(HAVE_RB_THREAD_BLOCKING_REGION) +#if defined(__linux__) && defined(KGIO_HAVE_THREAD_CALL_WITHOUT_GVL) static int accept4_flags = SOCK_CLOEXEC; #else /* ! linux */ static int accept4_flags = SOCK_CLOEXEC | SOCK_NONBLOCK; @@ -76,7 +76,7 @@ static VALUE xaccept(void *ptr) return (VALUE)rv; } -#ifdef HAVE_RB_THREAD_BLOCKING_REGION +#ifdef KGIO_HAVE_THREAD_CALL_WITHOUT_GVL # include <time.h> # include "blocking_io_region.h" static int thread_accept(struct accept_args *a, int force_nonblock) @@ -86,7 +86,7 @@ static int thread_accept(struct accept_args *a, int force_nonblock) return (int)rb_thread_io_blocking_region(xaccept, a, a->fd); } -#else /* ! HAVE_RB_THREAD_BLOCKING_REGION */ +#else /* ! KGIO_HAVE_THREAD_CALL_WITHOUT_GVL */ # include <rubysig.h> static int thread_accept(struct accept_args *a, int force_nonblock) { @@ -103,7 +103,7 @@ static int thread_accept(struct accept_args *a, int force_nonblock) TRAP_END; return rv; } -#endif /* ! HAVE_RB_THREAD_BLOCKING_REGION */ +#endif /* ! KGIO_HAVE_THREAD_CALL_WITHOUT_GVL */ static void prepare_accept(struct accept_args *a, VALUE self, int argc, const VALUE *argv) diff --git a/ext/kgio/blocking_io_region.h b/ext/kgio/blocking_io_region.h index 30c7106..10e7533 100644 --- a/ext/kgio/blocking_io_region.h +++ b/ext/kgio/blocking_io_region.h @@ -1,8 +1,11 @@ -#ifdef HAVE_RB_THREAD_BLOCKING_REGION -# ifdef HAVE_RB_THREAD_IO_BLOCKING_REGION +#ifdef KGIO_HAVE_THREAD_CALL_WITHOUT_GVL +# if defined(HAVE_RB_THREAD_IO_BLOCKING_REGION) /* temporary API for Ruby 1.9.3 */ VALUE rb_thread_io_blocking_region(rb_blocking_function_t *, void *, int); -# else +# elif defined(HAVE_RB_THREAD_CALL_WITHOUT_GVL) +# define rb_thread_io_blocking_region(fn,data,fd) \ + rb_thread_call_without_gvl((fn),(data),RUBY_UBF_IO,0) +# elif defined(HAVE_RB_THREAD_BLOCKING_REGION) # define rb_thread_io_blocking_region(fn,data,fd) \ rb_thread_blocking_region((fn),(data),RUBY_UBF_IO,0) # endif diff --git a/ext/kgio/connect.c b/ext/kgio/connect.c index 2027795..ef1a396 100644 --- a/ext/kgio/connect.c +++ b/ext/kgio/connect.c @@ -143,7 +143,7 @@ static struct sockaddr *sockaddr_from(socklen_t *addrlen, VALUE addr) return NULL; } -#if defined(MSG_FASTOPEN) && defined(HAVE_RB_THREAD_BLOCKING_REGION) +#if defined(MSG_FASTOPEN) && defined(KGIO_HAVE_THREAD_CALL_WITHOUT_GVL) #ifndef HAVE_RB_STR_SUBSEQ #define rb_str_subseq rb_str_substr #endif @@ -381,7 +381,7 @@ void init_kgio_connect(void) rb_define_singleton_method(cKgio_Socket, "new", kgio_new, -1); rb_define_singleton_method(cKgio_Socket, "connect", kgio_connect, 1); rb_define_singleton_method(cKgio_Socket, "start", kgio_start, 1); -#if defined(MSG_FASTOPEN) && defined(HAVE_RB_THREAD_BLOCKING_REGION) +#if defined(MSG_FASTOPEN) && defined(KGIO_HAVE_THREAD_CALL_WITHOUT_GVL) rb_define_method(cKgio_Socket, "kgio_fastopen", fastopen, 2); #endif /* diff --git a/ext/kgio/extconf.rb b/ext/kgio/extconf.rb index 5fb15ac..44888dd 100644 --- a/ext/kgio/extconf.rb +++ b/ext/kgio/extconf.rb @@ -46,6 +46,8 @@ have_func('rb_io_ascii8bit_binmode') have_func('rb_update_max_fd') have_func('rb_fd_fix_cloexec') have_func('rb_cloexec_open') +have_header('ruby/thread.h') +have_func('rb_thread_call_without_gvl', %w{ruby/thread.h}) have_func('rb_thread_blocking_region') have_func('rb_thread_io_blocking_region') have_func('rb_str_set_len') diff --git a/ext/kgio/kgio.h b/ext/kgio/kgio.h index 983280d..0c689a3 100644 --- a/ext/kgio/kgio.h +++ b/ext/kgio/kgio.h @@ -7,6 +7,9 @@ #else # include <rubyio.h> #endif +#ifdef HAVE_RUBY_THREAD_H +# include <ruby/thread.h> +#endif #include <errno.h> #include <sys/types.h> #include <sys/socket.h> @@ -42,7 +45,16 @@ void kgio_autopush_send(VALUE); VALUE kgio_call_wait_writable(VALUE io); VALUE kgio_call_wait_readable(VALUE io); -#if defined(HAVE_RB_THREAD_BLOCKING_REGION) && defined(HAVE_POLL) +#if defined(HAVE_RB_THREAD_CALL_WITHOUT_GVL) +# define KGIO_HAVE_THREAD_CALL_WITHOUT_GVL 1 +typedef void *(*kgio_blocking_fn_t)(void*); +# define rb_thread_blocking_region(fn,data1,ubf,data2) \ + rb_thread_call_without_gvl((kgio_blocking_fn_t)(fn),(data1),(ubf),(data2)) +#elif defined(HAVE_RB_THREAD_BLOCKING_REGION) +# define KGIO_HAVE_THREAD_CALL_WITHOUT_GVL 1 +#endif /* HAVE_RB_THREAD_CALL_WITHOUT_GVL || HAVE_RB_THREAD_BLOCKING_REGION */ + +#if defined(KGIO_HAVE_THREAD_CALL_WITHOUT_GVL) && defined(HAVE_POLL) # define USE_KGIO_POLL #endif /* USE_KGIO_POLL */ diff --git a/ext/kgio/tryopen.c b/ext/kgio/tryopen.c index 902c745..1d8fffd 100644 --- a/ext/kgio/tryopen.c +++ b/ext/kgio/tryopen.c @@ -17,6 +17,7 @@ #include <errno.h> #include "set_file_path.h" #include "ancient_ruby.h" +#include "kgio.h" static ID id_for_fd, id_to_path, id_path; static st_table *errno2sym; @@ -38,7 +39,7 @@ static VALUE nogvl_open(void *ptr) return (VALUE)rb_cloexec_open(o->pathname, o->flags, o->mode); } -#ifndef HAVE_RB_THREAD_BLOCKING_REGION +#ifndef KGIO_HAVE_THREAD_CALL_WITHOUT_GVL # define RUBY_UBF_IO ((void *)(-1)) # include "rubysig.h" typedef void rb_unblock_function_t(void *); @@ -57,7 +58,7 @@ static VALUE my_thread_blocking_region( } #define rb_thread_blocking_region(fn,data1,ubf,data2) \ my_thread_blocking_region((fn),(data1),(ubf),(data2)) -#endif /* ! HAVE_RB_THREAD_BLOCKING_REGION */ +#endif /* ! KGIO_HAVE_THREAD_CALL_WITHOUT_GVL */ /* * call-seq: -- 1.8.5.3 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] Don't use deprecated api 2014-01-20 19:36 ` Hleb Valoshka @ 2014-01-20 21:12 ` Eric Wong 0 siblings, 0 replies; 9+ messages in thread From: Eric Wong @ 2014-01-20 21:12 UTC (permalink / raw) To: kgio Thanks! Signed-off and pushed (commit 008483785d1a5bb801219af8afbb77be18b9bef1) ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-01-20 21:13 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-12-26 19:57 [PATCH] Don't use deprecated api Hleb Valoshka 2013-12-27 0:19 ` Eric Wong 2014-01-16 23:56 ` Eric Wong 2014-01-17 8:37 ` Hleb Valoshka 2014-01-19 19:42 ` Hleb Valoshka 2014-01-19 23:13 ` Eric Wong 2014-01-20 19:35 ` Hleb Valoshka 2014-01-20 19:36 ` Hleb Valoshka 2014-01-20 21:12 ` Eric Wong
Code repositories for project(s) associated with this public inbox https://yhbt.net/kgio.git/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).