From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: X-Spam-Status: No, score=-2.9 required=3.0 tests=ALL_TRUSTED,AWL,BAYES_00, URIBL_BLOCKED shortcircuit=no autolearn=unavailable version=3.3.2 X-Original-To: kgio-public@bogomips.org Received: from localhost (dcvr.yhbt.net [127.0.0.1]) by dcvr.yhbt.net (Postfix) with ESMTP id E9F146338D4; Thu, 13 Aug 2015 20:55:40 +0000 (UTC) From: Eric Wong To: kgio-public@bogomips.org Cc: Eric Wong Subject: [PATCH] remove autopush support and make it a no-op Date: Thu, 13 Aug 2015 20:55:39 +0000 Message-Id: <1439499339-29222-1-git-send-email-e@80x24.org> List-Id: Maintaining global state in a library like kgio is ugly, and it is not optimal from a performance standpoint compared to using MSG_MORE. TCP_CORK and TCP_NOPUSH require extra syscalls so it still offers sub-optimal performance compared to MSG_MORE. Instead, server developers should use MSG_MORE if their OS supports it (and should add MSG_MORE to their OS if lacking it). --- .document | 1 - ext/kgio/accept.c | 7 -- ext/kgio/autopush.c | 247 -------------------------------------------------- ext/kgio/kgio.h | 12 --- ext/kgio/kgio_ext.c | 1 - ext/kgio/read.c | 8 -- ext/kgio/write.c | 4 - ext/kgio/writev.c | 2 - lib/kgio.rb | 10 ++ test/test_autopush.rb | 160 +------------------------------- 10 files changed, 13 insertions(+), 439 deletions(-) delete mode 100644 ext/kgio/autopush.c diff --git a/.document b/.document index c7e09c6..f3a0872 100644 --- a/.document +++ b/.document @@ -7,7 +7,6 @@ ISSUES HACKING lib ext/kgio/accept.c -ext/kgio/autopush.c ext/kgio/connect.c ext/kgio/kgio_ext.c ext/kgio/poll.c diff --git a/ext/kgio/accept.c b/ext/kgio/accept.c index c847c92..4a45e2f 100644 --- a/ext/kgio/accept.c +++ b/ext/kgio/accept.c @@ -160,12 +160,6 @@ static VALUE in_addr_set(VALUE io, struct sockaddr_storage *addr, socklen_t len) return rb_ivar_set(io, iv_kgio_addr, host); } -#if defined(__linux__) -# define post_accept kgio_autopush_accept -#else -# define post_accept(a,b) for(;0;) -#endif - static VALUE my_accept(struct accept_args *a, int force_nonblock) { @@ -211,7 +205,6 @@ retry: } } client_io = sock_for_fd(a->accepted_class, client_fd); - post_accept(a->accept_io, client_io); if (a->addr) in_addr_set(client_io, diff --git a/ext/kgio/autopush.c b/ext/kgio/autopush.c deleted file mode 100644 index 74576e8..0000000 --- a/ext/kgio/autopush.c +++ /dev/null @@ -1,247 +0,0 @@ -/* - * We use a very basic strategy to use TCP_CORK semantics optimally - * in most TCP servers: On corked sockets, we will uncork on recv() - * if there was a previous send(). Otherwise we do not fiddle - * with TCP_CORK at all. - * - * Under Linux, we can rely on TCP_CORK being inherited in an - * accept()-ed client socket so we can avoid syscalls for each - * accept()-ed client if we know the accept() socket corks. - * - * This module does NOTHING for client TCP sockets, we only deal - * with accept()-ed sockets right now. - */ - -#include "kgio.h" -#include "my_fileno.h" -#include - -/* - * As of FreeBSD 4.5, TCP_NOPUSH == TCP_CORK - * ref: http://dotat.at/writing/nopush.html - * We won't care for older FreeBSD since nobody runs Ruby on them... - */ -#ifdef TCP_CORK -# define KGIO_NOPUSH TCP_CORK -#elif defined(TCP_NOPUSH) -# define KGIO_NOPUSH TCP_NOPUSH -#endif - -#ifdef KGIO_NOPUSH -static ID id_autopush_state; -static int enabled = 1; - -enum autopush_state { - AUTOPUSH_STATE_ACCEPTOR_IGNORE = -1, - AUTOPUSH_STATE_IGNORE = 0, - AUTOPUSH_STATE_WRITER = 1, - AUTOPUSH_STATE_WRITTEN = 2, - AUTOPUSH_STATE_ACCEPTOR = 3 -}; - -#if defined(R_CAST) && \ - defined(HAVE_TYPE_STRUCT_RFILE) && \ - defined(HAVE_TYPE_STRUCT_ROBJECT) && \ - ((SIZEOF_STRUCT_RFILE + SIZEOF_INT) <= (SIZEOF_STRUCT_ROBJECT)) - -struct AutopushSocket { - struct RFile rfile; - enum autopush_state autopush_state; -}; - -static enum autopush_state state_get(VALUE io) -{ - return ((struct AutopushSocket *)(io))->autopush_state; -} - -static void state_set(VALUE io, enum autopush_state state) -{ - ((struct AutopushSocket *)(io))->autopush_state = state; -} -#else -static enum autopush_state state_get(VALUE io) -{ - VALUE val; - - if (rb_ivar_defined(io, id_autopush_state) == Qfalse) - return AUTOPUSH_STATE_IGNORE; - val = rb_ivar_get(io, id_autopush_state); - - return (enum autopush_state)NUM2INT(val); -} - -static void state_set(VALUE io, enum autopush_state state) -{ - rb_ivar_set(io, id_autopush_state, INT2NUM(state)); -} -#endif /* IVAR fallback */ - -static enum autopush_state detect_acceptor_state(VALUE io); -static void push_pending_data(VALUE io); - -/* - * call-seq: - * Kgio.autopush? -> true or false - * - * Returns whether or not autopush is enabled. - * - * Only available on systems with TCP_CORK (Linux) or - * TCP_NOPUSH (FreeBSD, and maybe other *BSDs). - */ -static VALUE s_get_autopush(VALUE self) -{ - return enabled ? Qtrue : Qfalse; -} - -/* - * call-seq: - * Kgio.autopush = true - * Kgio.autopush = false - * - * Enables or disables autopush for sockets created with kgio_accept - * and kgio_tryaccept methods. Autopush relies on TCP_CORK/TCP_NOPUSH - * being enabled on the listen socket. - * - * Only available on systems with TCP_CORK (Linux) or - * TCP_NOPUSH (FreeBSD, and maybe other *BSDs). - */ -static VALUE s_set_autopush(VALUE self, VALUE val) -{ - enabled = RTEST(val); - - return val; -} - -/* - * call-seq: - * - * io.kgio_autopush? -> true or false - * - * Returns the current autopush state of the Kgio::SocketMethods-enabled - * socket. - * - * Only available on systems with TCP_CORK (Linux) or - * TCP_NOPUSH (FreeBSD, and maybe other *BSDs). - */ -static VALUE autopush_get(VALUE io) -{ - return state_get(io) <= 0 ? Qfalse : Qtrue; -} - -/* - * call-seq: - * - * io.kgio_autopush = true - * io.kgio_autopush = false - * - * Enables or disables autopush on any given Kgio::SocketMethods-capable - * IO object. This does NOT enable or disable TCP_NOPUSH/TCP_CORK right - * away, that must be done with IO.setsockopt - * - * Only available on systems with TCP_CORK (Linux) or - * TCP_NOPUSH (FreeBSD, and maybe other *BSDs). - */ -static VALUE autopush_set(VALUE io, VALUE vbool) -{ - if (RTEST(vbool)) - state_set(io, AUTOPUSH_STATE_WRITER); - else - state_set(io, AUTOPUSH_STATE_IGNORE); - return vbool; -} - -void init_kgio_autopush(void) -{ - VALUE mKgio = rb_define_module("Kgio"); - VALUE tmp; - - rb_define_singleton_method(mKgio, "autopush?", s_get_autopush, 0); - rb_define_singleton_method(mKgio, "autopush=", s_set_autopush, 1); - - tmp = rb_define_module_under(mKgio, "SocketMethods"); - rb_define_method(tmp, "kgio_autopush=", autopush_set, 1); - rb_define_method(tmp, "kgio_autopush?", autopush_get, 0); - - id_autopush_state = rb_intern("@kgio_autopush_state"); -} - -/* - * called after a successful write, just mark that we've put something - * in the skb and will need to uncork on the next write. - */ -void kgio_autopush_send(VALUE io) -{ - if (state_get(io) == AUTOPUSH_STATE_WRITER) - state_set(io, AUTOPUSH_STATE_WRITTEN); -} - -/* called on successful accept() */ -void kgio_autopush_accept(VALUE accept_io, VALUE client_io) -{ - enum autopush_state acceptor_state; - - if (!enabled) - return; - acceptor_state = state_get(accept_io); - if (acceptor_state == AUTOPUSH_STATE_IGNORE) - acceptor_state = detect_acceptor_state(accept_io); - if (acceptor_state == AUTOPUSH_STATE_ACCEPTOR) - state_set(client_io, AUTOPUSH_STATE_WRITER); - else - state_set(client_io, AUTOPUSH_STATE_IGNORE); -} - -void kgio_autopush_recv(VALUE io) -{ - if (enabled && (state_get(io) == AUTOPUSH_STATE_WRITTEN)) { - push_pending_data(io); - state_set(io, AUTOPUSH_STATE_WRITER); - } -} - -static enum autopush_state detect_acceptor_state(VALUE io) -{ - int corked = 0; - int fd = my_fileno(io); - socklen_t optlen = sizeof(int); - enum autopush_state state; - - if (getsockopt(fd, IPPROTO_TCP, KGIO_NOPUSH, &corked, &optlen) != 0) { - if (errno != EOPNOTSUPP) - rb_sys_fail("getsockopt(TCP_CORK/TCP_NOPUSH)"); - errno = 0; - state = AUTOPUSH_STATE_ACCEPTOR_IGNORE; - } else if (corked) { - state = AUTOPUSH_STATE_ACCEPTOR; - } else { - state = AUTOPUSH_STATE_ACCEPTOR_IGNORE; - } - state_set(io, state); - - return state; -} - -/* - * checks to see if we've written anything since the last recv() - * If we have, uncork the socket and immediately recork it. - */ -static void push_pending_data(VALUE io) -{ - int optval = 0; - const socklen_t optlen = sizeof(int); - const int fd = my_fileno(io); - - if (setsockopt(fd, IPPROTO_TCP, KGIO_NOPUSH, &optval, optlen) != 0) - rb_sys_fail("setsockopt(TCP_CORK/TCP_NOPUSH, 0)"); - /* immediately recork */ - optval = 1; - if (setsockopt(fd, IPPROTO_TCP, KGIO_NOPUSH, &optval, optlen) != 0) - rb_sys_fail("setsockopt(TCP_CORK/TCP_NOPUSH, 1)"); -} -#else /* !KGIO_NOPUSH */ -void kgio_autopush_recv(VALUE io){} -void kgio_autopush_send(VALUE io){} -void init_kgio_autopush(void) -{ -} -#endif /* ! KGIO_NOPUSH */ diff --git a/ext/kgio/kgio.h b/ext/kgio/kgio.h index c0630ae..a3f2f66 100644 --- a/ext/kgio/kgio.h +++ b/ext/kgio/kgio.h @@ -29,14 +29,9 @@ void init_kgio_write(void); void init_kgio_writev(void); void init_kgio_accept(void); void init_kgio_connect(void); -void init_kgio_autopush(void); void init_kgio_poll(void); void init_kgio_tryopen(void); -void kgio_autopush_accept(VALUE, VALUE); -void kgio_autopush_recv(VALUE); -void kgio_autopush_send(VALUE); - VALUE kgio_call_wait_writable(VALUE io); VALUE kgio_call_wait_readable(VALUE io); #if defined(HAVE_RB_THREAD_CALL_WITHOUT_GVL) && defined(HAVE_RUBY_THREAD_H) @@ -90,13 +85,6 @@ NORETURN(void kgio_rd_sys_fail(const char *)); # define USE_MSG_DONTWAIT # endif -#ifdef USE_MSG_DONTWAIT -/* we don't need these variants, we call kgio_autopush_send/recv directly */ -static inline void kgio_autopush_write(VALUE io) { } -#else -static inline void kgio_autopush_write(VALUE io) { kgio_autopush_send(io); } -#endif - /* prefer rb_str_subseq because we don't use negative offsets */ #ifndef HAVE_RB_STR_SUBSEQ #define MY_STR_SUBSEQ(str,beg,len) rb_str_substr((str),(beg),(len)) diff --git a/ext/kgio/kgio_ext.c b/ext/kgio/kgio_ext.c index 8829eae..c3e59ec 100644 --- a/ext/kgio/kgio_ext.c +++ b/ext/kgio/kgio_ext.c @@ -95,7 +95,6 @@ void Init_kgio_ext(void) init_kgio_writev(); init_kgio_connect(); init_kgio_accept(); - init_kgio_autopush(); init_kgio_poll(); init_kgio_tryopen(); } diff --git a/ext/kgio/read.c b/ext/kgio/read.c index 472a592..e55db16 100644 --- a/ext/kgio/read.c +++ b/ext/kgio/read.c @@ -7,13 +7,8 @@ static VALUE sym_wait_readable; #ifdef USE_MSG_DONTWAIT static const int peek_flags = MSG_DONTWAIT|MSG_PEEK; - -/* we don't need these variants, we call kgio_autopush_recv directly */ -static inline void kgio_autopush_read(VALUE io) { } - #else static const int peek_flags = MSG_PEEK; -static inline void kgio_autopush_read(VALUE io) { kgio_autopush_recv(io); } #endif struct rd_args { @@ -85,7 +80,6 @@ static VALUE my_read(int io_wait, int argc, VALUE *argv, VALUE io) long n; prepare_read(&a, argc, argv, io); - kgio_autopush_read(io); if (a.len > 0) { set_nonblocking(a.fd); @@ -158,7 +152,6 @@ static VALUE my_recv(int io_wait, int argc, VALUE *argv, VALUE io) long n; prepare_read(&a, argc, argv, io); - kgio_autopush_recv(io); if (a.len > 0) { retry: @@ -212,7 +205,6 @@ static VALUE my_peek(int io_wait, int argc, VALUE *argv, VALUE io) long n; prepare_read(&a, argc, argv, io); - kgio_autopush_recv(io); if (a.len > 0) { if (peek_flags == MSG_PEEK) diff --git a/ext/kgio/write.c b/ext/kgio/write.c index ce4aa75..fa0d53c 100644 --- a/ext/kgio/write.c +++ b/ext/kgio/write.c @@ -72,8 +72,6 @@ retry: n = (long)write(a.fd, a.ptr, a.len); if (write_check(&a, n, "write", io_wait) != 0) goto retry; - if (TYPE(a.buf) != T_SYMBOL) - kgio_autopush_write(io); return a.buf; } @@ -126,8 +124,6 @@ retry: n = (long)send(a.fd, a.ptr, a.len, MSG_DONTWAIT); if (write_check(&a, n, "send", io_wait) != 0) goto retry; - if (TYPE(a.buf) != T_SYMBOL) - kgio_autopush_send(io); return a.buf; } diff --git a/ext/kgio/writev.c b/ext/kgio/writev.c index d3ec53e..736aa6f 100644 --- a/ext/kgio/writev.c +++ b/ext/kgio/writev.c @@ -249,8 +249,6 @@ static VALUE my_writev(VALUE io, VALUE ary, int io_wait) } while (writev_check(&a, n, "writev", io_wait) != 0); rb_str_resize(a.vec_buf, 0); - if (TYPE(a.buf) != T_SYMBOL) - kgio_autopush_write(io); return a.buf; } diff --git a/lib/kgio.rb b/lib/kgio.rb index 5de431b..f192074 100644 --- a/lib/kgio.rb +++ b/lib/kgio.rb @@ -16,6 +16,16 @@ module Kgio # PipeMethods#kgio_trywrite and SocketMethods#kgio_trywrite will return # :wait_writable when waiting for a read is required. WaitWritable = :wait_writable + + # autopush is no-op nowadays + @autopush = false + + class << self + attr_accessor :autopush # :nodoc: + def autopush? # :nodoc: + !!@autopush + end + end end require 'kgio_ext' diff --git a/test/test_autopush.rb b/test/test_autopush.rb index 38b7c52..4e5af92 100644 --- a/test/test_autopush.rb +++ b/test/test_autopush.rb @@ -1,165 +1,11 @@ -require 'tempfile' require 'test/unit' -begin - $-w = false - RUBY_PLATFORM =~ /linux/ and require 'strace' -rescue LoadError -end -$-w = true require 'kgio' class TestAutopush < Test::Unit::TestCase - TCP_CORK = 3 - TCP_NOPUSH = 4 - - def setup - Kgio.autopush = false - assert_equal false, Kgio.autopush? - - @host = ENV["TEST_HOST"] || '127.0.0.1' - @srv = Kgio::TCPServer.new(@host, 0) - RUBY_PLATFORM =~ /linux/ and - @srv.setsockopt(Socket::IPPROTO_TCP, TCP_CORK, 1) - RUBY_PLATFORM =~ /freebsd/ and - @srv.setsockopt(Socket::IPPROTO_TCP, TCP_NOPUSH, 1) - @port = @srv.addr[1] - end - - def test_autopush_accessors - Kgio.autopush = true - opt = RUBY_PLATFORM =~ /freebsd/ ? TCP_NOPUSH : TCP_CORK - s = Kgio::TCPSocket.new(@host, @port) - assert_equal 0, s.getsockopt(Socket::IPPROTO_TCP, opt).unpack('i')[0] - assert ! s.kgio_autopush? - s.kgio_autopush = true - assert s.kgio_autopush? - s.kgio_write 'asdf' - assert_equal :wait_readable, s.kgio_tryread(1) - assert s.kgio_autopush? - val = s.getsockopt(Socket::IPPROTO_TCP, opt).unpack('i')[0] - assert_operator val, :>, 0, "#{opt}=#{val} (#{RUBY_PLATFORM})" - end - - def test_autopush_true_unix - Kgio.autopush = true - tmp = Tempfile.new('kgio_unix') - @path = tmp.path - tmp.close! - @srv = Kgio::UNIXServer.new(@path) - @rd = Kgio::UNIXSocket.new(@path) - t0 = nil - if defined?(Strace) - io, err = Strace.me { @wr = @srv.kgio_accept } - assert_nil err - rc = nil - io, err = Strace.me { - t0 = Time.now - @wr.kgio_write "HI\n" - rc = @wr.kgio_tryread 666 - } - assert_nil err - lines = io.readlines - assert lines.grep(/TCP_CORK/).empty?, lines.inspect - else - @wr = @srv.kgio_accept - t0 = Time.now - @wr.kgio_write "HI\n" - rc = @wr.kgio_tryread 666 - end - assert_equal "HI\n", @rd.kgio_read(3) - diff = Time.now - t0 - assert(diff < 0.200, "nopush on UNIX sockets? diff=#{diff} > 200ms") - assert_equal :wait_readable, rc - ensure - File.unlink(@path) rescue nil - end - - def test_autopush_false - Kgio.autopush = nil - assert_equal false, Kgio.autopush? - - @wr = Kgio::TCPSocket.new(@host, @port) - if defined?(Strace) - io, err = Strace.me { @rd = @srv.kgio_accept } - assert_nil err - lines = io.readlines - assert lines.grep(/TCP_CORK/).empty?, lines.inspect - assert_equal 1, @rd.getsockopt(Socket::SOL_TCP, TCP_CORK).unpack("i")[0] - else - @rd = @srv.kgio_accept - end - - rbuf = "..." - t0 = Time.now - @rd.kgio_write "HI\n" - @wr.kgio_read(3, rbuf) - diff = Time.now - t0 - assert(diff >= 0.190, "nopush broken? diff=#{diff} > 200ms") - assert_equal "HI\n", rbuf - end - - def test_autopush_true + def test_compatibility Kgio.autopush = true assert_equal true, Kgio.autopush? - @wr = Kgio::TCPSocket.new(@host, @port) - - if defined?(Strace) - io, err = Strace.me { @rd = @srv.kgio_accept } - assert_nil err - lines = io.readlines - assert_equal 1, lines.grep(/TCP_CORK/).size, lines.inspect - assert_equal 1, @rd.getsockopt(Socket::SOL_TCP, TCP_CORK).unpack("i")[0] - else - @rd = @srv.kgio_accept - end - - @wr.write "HI\n" - rbuf = "" - if defined?(Strace) - io, err = Strace.me { @rd.kgio_read(3, rbuf) } - assert_nil err - lines = io.readlines - assert lines.grep(/TCP_CORK/).empty?, lines.inspect - assert_equal "HI\n", rbuf - else - assert_equal "HI\n", @rd.kgio_read(3, rbuf) - end - - t0 = Time.now - @rd.kgio_write "HI2U2\n" - @rd.kgio_write "HOW\n" - rc = false - - if defined?(Strace) - io, err = Strace.me { rc = @rd.kgio_tryread(666) } - else - rc = @rd.kgio_tryread(666) - end - - @wr.readpartial(666, rbuf) - rbuf == "HI2U2\nHOW\n" or warn "rbuf=#{rbuf.inspect} looking bad?" - diff = Time.now - t0 - assert(diff < 0.200, "time diff=#{diff} >= 200ms") - assert_equal :wait_readable, rc - if defined?(Strace) - assert_nil err - lines = io.readlines - assert_equal 2, lines.grep(/TCP_CORK/).size, lines.inspect - end - @wr.close - @rd.close - - @wr = Kgio::TCPSocket.new(@host, @port) - if defined?(Strace) - io, err = Strace.me { @rd = @srv.kgio_accept } - assert_nil err - lines = io.readlines - assert lines.grep(/TCP_CORK/).empty?,"optimization fail: #{lines.inspect}" - assert_equal 1, @rd.getsockopt(Socket::SOL_TCP, TCP_CORK).unpack("i")[0] - end - end - - def teardown Kgio.autopush = false + assert_equal false, Kgio.autopush? end -end if RUBY_PLATFORM =~ /linux|freebsd/ +end -- EW