From b4280737cd10ee8bc84ed22e0b6c2ee3508a74b1 Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Fri, 25 Oct 2024 18:20:03 +1300 Subject: [PATCH 1/5] Use `rb_io_wait` function and cache `io` instance. --- contrib/ruby/ext/trilogy-ruby/cext.c | 128 +++++++++++++++++++++-- contrib/ruby/ext/trilogy-ruby/extconf.rb | 4 + inc/trilogy/socket.h | 1 + 3 files changed, 124 insertions(+), 9 deletions(-) diff --git a/contrib/ruby/ext/trilogy-ruby/cext.c b/contrib/ruby/ext/trilogy-ruby/cext.c index 54dc8168..6a0ab11b 100644 --- a/contrib/ruby/ext/trilogy-ruby/cext.c +++ b/contrib/ruby/ext/trilogy-ruby/cext.c @@ -4,10 +4,10 @@ #include #include #include + #include #include #include - #include #include @@ -15,6 +15,14 @@ #include "trilogy-ruby.h" +#if defined(HAVE_RB_IO_WAIT) && defined(RB_IO_OPEN_DESCRIPTOR) && defined(HAVE_RUBY_FIBER_SCHEDULER_H) +#define TRILOGY_RB_IO_WAIT +#endif + +#ifdef TRILOGY_RB_IO_WAIT +#include +#endif + VALUE Trilogy_CastError; static VALUE Trilogy_BaseConnectionError, Trilogy_ProtocolError, Trilogy_SSLError, Trilogy_QueryError, Trilogy_ConnectionClosedError, @@ -28,26 +36,58 @@ static ID id_socket, id_host, id_port, id_username, id_password, id_found_rows, id_from_code, id_from_errno, id_connection_options, id_max_allowed_packet; struct trilogy_ctx { + VALUE self; + trilogy_conn_t conn; + +#ifdef TRILOGY_RB_IO_WAIT + VALUE io; +#endif + char server_version[TRILOGY_SERVER_VERSION_SIZE + 1]; unsigned int query_flags; VALUE encoding; }; -static void mark_trilogy(void *ptr) +static void trilogy_ctx_mark(void *ptr) { struct trilogy_ctx *ctx = ptr; - rb_gc_mark(ctx->encoding); + + rb_gc_mark_movable(ctx->self); + +#ifdef TRILOGY_RB_IO_WAIT + rb_gc_mark_movable(ctx->io); +#endif + + rb_gc_mark_movable(ctx->encoding); } -static void free_trilogy(void *ptr) +static void trilogy_ctx_compact(void *ptr) { struct trilogy_ctx *ctx = ptr; - trilogy_free(&ctx->conn); + + ctx->self = rb_gc_location(ctx->self); + +#ifdef TRILOGY_RB_IO_WAIT + if (RTEST(ctx->io)) + ctx->io = rb_gc_location(ctx->io); +#endif + + ctx->encoding = rb_gc_location(ctx->encoding); +} + +static void trilogy_ctx_free(void *ptr) +{ + struct trilogy_ctx *ctx = ptr; + + if (ctx->conn.socket != NULL) { + trilogy_free(&ctx->conn); + } + xfree(ptr); } -static size_t trilogy_memsize(const void *ptr) { +static size_t trilogy_ctx_memsize(const void *ptr) { const struct trilogy_ctx *ctx = ptr; size_t memsize = sizeof(struct trilogy_ctx); if (ctx->conn.socket != NULL) { @@ -60,9 +100,10 @@ static size_t trilogy_memsize(const void *ptr) { static const rb_data_type_t trilogy_data_type = { .wrap_struct_name = "trilogy", .function = { - .dmark = mark_trilogy, - .dfree = free_trilogy, - .dsize = trilogy_memsize, + .dmark = trilogy_ctx_mark, + .dcompact = trilogy_ctx_compact, + .dfree = trilogy_ctx_free, + .dsize = trilogy_ctx_memsize, }, .flags = RUBY_TYPED_FREE_IMMEDIATELY | RUBY_TYPED_WB_PROTECTED }; @@ -176,6 +217,12 @@ static VALUE allocate_trilogy(VALUE klass) VALUE obj = TypedData_Make_Struct(klass, struct trilogy_ctx, &trilogy_data_type, ctx); + RB_OBJ_WRITE(obj, &ctx->self, obj); + +#ifdef TRILOGY_RB_IO_WAIT + ctx->io = Qnil; +#endif + ctx->query_flags = TRILOGY_FLAGS_DEFAULT; if (trilogy_init(&ctx->conn) < 0) { @@ -224,6 +271,66 @@ struct rb_trilogy_wait_args { int rc; }; +#ifdef TRILOGY_RB_IO_WAIT +static int _cb_ruby_wait(trilogy_sock_t *sock, trilogy_wait_t wait) +{ + struct trilogy_ctx *ctx = sock->user_data; + struct timeval *timeout = NULL; + int wait_flag = 0; + + switch (wait) { + case TRILOGY_WAIT_READ: + timeout = &sock->opts.read_timeout; + wait_flag = RUBY_IO_READABLE; + break; + + case TRILOGY_WAIT_WRITE: + timeout = &sock->opts.write_timeout; + wait_flag = RUBY_IO_WRITABLE; + break; + + case TRILOGY_WAIT_CONNECT: + // wait for connection to be writable + timeout = &sock->opts.connect_timeout; + if (timeout->tv_sec == 0 && timeout->tv_usec == 0) { + // We used to use the write timeout for this, so if a connect timeout isn't configured, default to that. + timeout = &sock->opts.write_timeout; + } + wait_flag = RUBY_IO_WRITABLE; + break; + + case TRILOGY_WAIT_HANDSHAKE: + // wait for handshake packet on initial connection + timeout = &sock->opts.connect_timeout; + wait_flag = RUBY_IO_READABLE; + break; + + default: + return TRILOGY_ERR; + } + + if (ctx->io == Qnil) { + VALUE io = rb_io_open_descriptor(rb_cIO, trilogy_sock_fd(sock), FMODE_EXTERNAL, RUBY_Qnil, RUBY_Qnil, NULL); + RB_OBJ_WRITE(ctx->self, &ctx->io, io); + } + + if (timeout->tv_sec == 0 && timeout->tv_usec == 0) { + timeout = NULL; + } + + VALUE result = rb_io_wait(ctx->io, RB_INT2NUM(wait_flag), rb_fiber_scheduler_make_timeout(timeout)); + + if (result == RUBY_Qfalse) { + return TRILOGY_TIMEOUT; + } + + if (RTEST(result)) { + return TRILOGY_OK; + } else { + return TRILOGY_SYSERR; + } +} +#else static VALUE rb_trilogy_wait_protected(VALUE vargs) { struct rb_trilogy_wait_args *args = (void *)vargs; @@ -294,6 +401,7 @@ static int _cb_ruby_wait(trilogy_sock_t *sock, trilogy_wait_t wait) return TRILOGY_OK; } +#endif struct nogvl_sock_args { int rc; @@ -330,6 +438,8 @@ static int try_connect(struct trilogy_ctx *ctx, trilogy_handshake_t *handshake, /* replace the default wait callback with our GVL-aware callback so we can escape the GVL on each wait operation without going through call_without_gvl */ sock->wait_cb = _cb_ruby_wait; + sock->user_data = ctx; + rc = trilogy_connect_send_socket(&ctx->conn, sock); if (rc < 0) { trilogy_sock_close(sock); diff --git a/contrib/ruby/ext/trilogy-ruby/extconf.rb b/contrib/ruby/ext/trilogy-ruby/extconf.rb index f618ef21..3d559935 100644 --- a/contrib/ruby/ext/trilogy-ruby/extconf.rb +++ b/contrib/ruby/ext/trilogy-ruby/extconf.rb @@ -18,4 +18,8 @@ have_library("ssl", "SSL_new") have_func("rb_interned_str", "ruby.h") +have_func("rb_io_wait", "ruby.h") +have_func("rb_io_open_descriptor", "ruby.h") +have_header("ruby/fiber/scheduler.h") + create_makefile "trilogy/cext" diff --git a/inc/trilogy/socket.h b/inc/trilogy/socket.h index 6de19476..69f6cb9b 100644 --- a/inc/trilogy/socket.h +++ b/inc/trilogy/socket.h @@ -84,6 +84,7 @@ typedef struct trilogy_sock_t { int (*fd_cb)(struct trilogy_sock_t *self); trilogy_sockopt_t opts; + void *user_data; } trilogy_sock_t; static inline int trilogy_sock_connect(trilogy_sock_t *sock) { return sock->connect_cb(sock); } From d6ce51413e2dfb474f9a8d9dcd3613a89de9db02 Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Sat, 25 Feb 2023 21:16:53 +1300 Subject: [PATCH 2/5] Non-blocking hostname -> numeric address. --- contrib/ruby/ext/trilogy-ruby/cext.c | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/contrib/ruby/ext/trilogy-ruby/cext.c b/contrib/ruby/ext/trilogy-ruby/cext.c index 6a0ab11b..c2e27918 100644 --- a/contrib/ruby/ext/trilogy-ruby/cext.c +++ b/contrib/ruby/ext/trilogy-ruby/cext.c @@ -415,7 +415,7 @@ static void *no_gvl_resolve(void *data) return NULL; } -static int try_connect(struct trilogy_ctx *ctx, trilogy_handshake_t *handshake, const trilogy_sockopt_t *opts) +static int try_connect(struct trilogy_ctx *ctx, trilogy_handshake_t *handshake, trilogy_sockopt_t *opts) { trilogy_sock_t *sock = trilogy_sock_new(opts); if (sock == NULL) { @@ -424,6 +424,28 @@ static int try_connect(struct trilogy_ctx *ctx, trilogy_handshake_t *handshake, struct nogvl_sock_args args = {.rc = 0, .sock = sock}; + // Attempt to resolve a non-numeric hostname using the fiber scheduler if possible. +#ifdef TRILOGY_RB_IO_WAIT + if (opts->hostname != NULL) { + VALUE scheduler = rb_fiber_scheduler_current(); + + if (scheduler != Qnil) { + VALUE addresses = rb_fiber_scheduler_address_resolve(scheduler, rb_str_new_cstr(opts->hostname)); + + if (RARRAY_LEN(addresses) == 0) { + return TRILOGY_DNS_ERR; + } + + free(opts->hostname); + opts->hostname = NULL; + VALUE address = rb_ary_entry(addresses, 0); + StringValue(address); + + opts->hostname = strndup(RSTRING_PTR(address), RSTRING_LEN(address)); + } + } +#endif + // Do the DNS resolving with the GVL unlocked. At this point all // configuration data is copied and available to the trilogy socket. rb_thread_call_without_gvl(no_gvl_resolve, (void *)&args, RUBY_UBF_IO, NULL); From 8ea19f556a2137bae1a2df33280657b66dca14b9 Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Tue, 25 Feb 2025 11:09:33 +1300 Subject: [PATCH 3/5] Remove `RTEST` from `rb_gc_location(ctx->io)`. --- contrib/ruby/ext/trilogy-ruby/cext.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/contrib/ruby/ext/trilogy-ruby/cext.c b/contrib/ruby/ext/trilogy-ruby/cext.c index c2e27918..f1c89b9a 100644 --- a/contrib/ruby/ext/trilogy-ruby/cext.c +++ b/contrib/ruby/ext/trilogy-ruby/cext.c @@ -69,8 +69,7 @@ static void trilogy_ctx_compact(void *ptr) ctx->self = rb_gc_location(ctx->self); #ifdef TRILOGY_RB_IO_WAIT - if (RTEST(ctx->io)) - ctx->io = rb_gc_location(ctx->io); + ctx->io = rb_gc_location(ctx->io); #endif ctx->encoding = rb_gc_location(ctx->encoding); From 03c0d155d465d3f4dbd1dc995a5ab0d58b07320c Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Tue, 25 Feb 2025 12:17:11 +1300 Subject: [PATCH 4/5] Also check for presence of `rb_fiber_scheduler_make_timeout`. --- contrib/ruby/ext/trilogy-ruby/cext.c | 2 +- contrib/ruby/ext/trilogy-ruby/extconf.rb | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/contrib/ruby/ext/trilogy-ruby/cext.c b/contrib/ruby/ext/trilogy-ruby/cext.c index f1c89b9a..d56c8e86 100644 --- a/contrib/ruby/ext/trilogy-ruby/cext.c +++ b/contrib/ruby/ext/trilogy-ruby/cext.c @@ -15,7 +15,7 @@ #include "trilogy-ruby.h" -#if defined(HAVE_RB_IO_WAIT) && defined(RB_IO_OPEN_DESCRIPTOR) && defined(HAVE_RUBY_FIBER_SCHEDULER_H) +#if defined(HAVE_RB_IO_WAIT) && defined(RB_IO_OPEN_DESCRIPTOR) && defined(HAVE_RB_FIBER_SCHEDULER_MAKE_TIMEOUT) && defined(HAVE_RUBY_FIBER_SCHEDULER_H) #define TRILOGY_RB_IO_WAIT #endif diff --git a/contrib/ruby/ext/trilogy-ruby/extconf.rb b/contrib/ruby/ext/trilogy-ruby/extconf.rb index 3d559935..c694798e 100644 --- a/contrib/ruby/ext/trilogy-ruby/extconf.rb +++ b/contrib/ruby/ext/trilogy-ruby/extconf.rb @@ -20,6 +20,7 @@ have_func("rb_io_wait", "ruby.h") have_func("rb_io_open_descriptor", "ruby.h") +have_func("rb_fiber_scheduler_make_timeout") have_header("ruby/fiber/scheduler.h") create_makefile "trilogy/cext" From 3e367d2d045a6ed454ad9138e26f893c0e8e71f9 Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Tue, 25 Feb 2025 12:34:47 +1300 Subject: [PATCH 5/5] Simplify implementation of `_cb_ruby_wait`. --- contrib/ruby/ext/trilogy-ruby/cext.c | 109 ++++++++++----------------- 1 file changed, 41 insertions(+), 68 deletions(-) diff --git a/contrib/ruby/ext/trilogy-ruby/cext.c b/contrib/ruby/ext/trilogy-ruby/cext.c index d56c8e86..e7b3f6b0 100644 --- a/contrib/ruby/ext/trilogy-ruby/cext.c +++ b/contrib/ruby/ext/trilogy-ruby/cext.c @@ -266,74 +266,40 @@ static double timeval_to_double(struct timeval tv) struct rb_trilogy_wait_args { struct timeval *timeout; int wait_flag; - int fd; - int rc; -}; #ifdef TRILOGY_RB_IO_WAIT -static int _cb_ruby_wait(trilogy_sock_t *sock, trilogy_wait_t wait) -{ - struct trilogy_ctx *ctx = sock->user_data; - struct timeval *timeout = NULL; - int wait_flag = 0; - - switch (wait) { - case TRILOGY_WAIT_READ: - timeout = &sock->opts.read_timeout; - wait_flag = RUBY_IO_READABLE; - break; - - case TRILOGY_WAIT_WRITE: - timeout = &sock->opts.write_timeout; - wait_flag = RUBY_IO_WRITABLE; - break; - - case TRILOGY_WAIT_CONNECT: - // wait for connection to be writable - timeout = &sock->opts.connect_timeout; - if (timeout->tv_sec == 0 && timeout->tv_usec == 0) { - // We used to use the write timeout for this, so if a connect timeout isn't configured, default to that. - timeout = &sock->opts.write_timeout; - } - wait_flag = RUBY_IO_WRITABLE; - break; - - case TRILOGY_WAIT_HANDSHAKE: - // wait for handshake packet on initial connection - timeout = &sock->opts.connect_timeout; - wait_flag = RUBY_IO_READABLE; - break; - - default: - return TRILOGY_ERR; - } - - if (ctx->io == Qnil) { - VALUE io = rb_io_open_descriptor(rb_cIO, trilogy_sock_fd(sock), FMODE_EXTERNAL, RUBY_Qnil, RUBY_Qnil, NULL); - RB_OBJ_WRITE(ctx->self, &ctx->io, io); - } + VALUE io; +#else + int fd; +#endif - if (timeout->tv_sec == 0 && timeout->tv_usec == 0) { - timeout = NULL; - } + int rc; +}; - VALUE result = rb_io_wait(ctx->io, RB_INT2NUM(wait_flag), rb_fiber_scheduler_make_timeout(timeout)); +static VALUE rb_trilogy_wait_protected(VALUE vargs) { + struct rb_trilogy_wait_args *args = (void *)vargs; +#ifdef TRILOGY_RB_IO_WAIT + VALUE result = rb_io_wait(args->io, RB_INT2NUM(args->wait_flag), rb_fiber_scheduler_make_timeout(args->timeout)); + if (result == RUBY_Qfalse) { - return TRILOGY_TIMEOUT; - } - - if (RTEST(result)) { - return TRILOGY_OK; + args->rc = TRILOGY_TIMEOUT; + } else if (RTEST(result)) { + args->rc = TRILOGY_OK; } else { - return TRILOGY_SYSERR; + args->rc = TRILOGY_SYSERR; } -} #else -static VALUE rb_trilogy_wait_protected(VALUE vargs) { - struct rb_trilogy_wait_args *args = (void *)vargs; + int result = rb_wait_for_single_fd(args->fd, args->wait_flag, args->timeout); - args->rc = rb_wait_for_single_fd(args->fd, args->wait_flag, args->timeout); + if (result == 0) { + args->rc = TRILOGY_TIMEOUT; + } else if (result < 0) { + args->rc = TRILOGY_SYSERR; + } else { + args->rc = TRILOGY_OK; + } +#endif return Qnil; } @@ -379,6 +345,21 @@ static int _cb_ruby_wait(trilogy_sock_t *sock, trilogy_wait_t wait) } struct rb_trilogy_wait_args args; + +#ifdef TRILOGY_RB_IO_WAIT + struct trilogy_ctx *ctx = sock->user_data; + + // Create the IO instance on demand: + if (ctx->io == Qnil) { + VALUE io = rb_io_open_descriptor(rb_cIO, trilogy_sock_fd(sock), FMODE_EXTERNAL, RUBY_Qnil, RUBY_Qnil, NULL); + RB_OBJ_WRITE(ctx->self, &ctx->io, io); + } + + args.io = ctx->io; +#else + args.fd = trilogy_sock_fd(sock); +#endif + args.fd = trilogy_sock_fd(sock); args.wait_flag = wait_flag; args.timeout = timeout; @@ -391,16 +372,8 @@ static int _cb_ruby_wait(trilogy_sock_t *sock, trilogy_wait_t wait) rb_jump_tag(state); } - // rc here comes from rb_wait_for_single_fd which (similar to poll(3)) returns 0 to indicate that the call timed out - // or -1 to indicate a system error with errno set. - if (args.rc < 0) - return TRILOGY_SYSERR; - if (args.rc == 0) - return TRILOGY_TIMEOUT; - - return TRILOGY_OK; + return args.rc; } -#endif struct nogvl_sock_args { int rc; @@ -423,8 +396,8 @@ static int try_connect(struct trilogy_ctx *ctx, trilogy_handshake_t *handshake, struct nogvl_sock_args args = {.rc = 0, .sock = sock}; - // Attempt to resolve a non-numeric hostname using the fiber scheduler if possible. #ifdef TRILOGY_RB_IO_WAIT + // Attempt to resolve a non-numeric hostname using the fiber scheduler if possible. if (opts->hostname != NULL) { VALUE scheduler = rb_fiber_scheduler_current();