From 3cfe0429776aa8f66373462b15833f943439cafb Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Fri, 19 Apr 2024 18:40:45 -0700 Subject: [PATCH] Release GVL in Statement#step This allows other threads to execute Ruby code (including performing more work sqlite operations if on another DB connection). This should significantly improve multithreaded performance, however will add overhead to single-threaded performance. This required wrapping all callbacks which can happen within statement execution with rb_thread_call_with_gvl. --- ext/sqlite3/aggregator.c | 44 +++++++++++++--- ext/sqlite3/database.c | 102 +++++++++++++++++++++++++++++-------- ext/sqlite3/sqlite3_ruby.h | 1 + ext/sqlite3/statement.c | 9 ++++ 4 files changed, 128 insertions(+), 28 deletions(-) diff --git a/ext/sqlite3/aggregator.c b/ext/sqlite3/aggregator.c index e6330043..003f595d 100644 --- a/ext/sqlite3/aggregator.c +++ b/ext/sqlite3/aggregator.c @@ -113,10 +113,20 @@ rb_sqlite3_aggregate_instance_destroy(sqlite3_context *ctx) *inst_ptr = Qnil; } -static void -rb_sqlite3_aggregator_step(sqlite3_context *ctx, int argc, sqlite3_value **argv) +struct rb_sqlite3_aggregator_step_args { + sqlite3_context *ctx; + int argc; + sqlite3_value **argv; +}; + +static void * +rb_sqlite3_aggregator_step(void *gvl_context) { - VALUE inst = rb_sqlite3_aggregate_instance(ctx); + struct rb_sqlite3_aggregator_step_args *args = gvl_context; + int argc = args->argc; + sqlite3_value **argv = args->argv; + + VALUE inst = rb_sqlite3_aggregate_instance(args->ctx); VALUE handler_instance = rb_iv_get(inst, "-handler_instance"); VALUE *params = NULL; VALUE one_param; @@ -124,7 +134,7 @@ rb_sqlite3_aggregator_step(sqlite3_context *ctx, int argc, sqlite3_value **argv) int i; if (exc_status) { - return; + return NULL; } if (argc == 1) { @@ -144,12 +154,22 @@ rb_sqlite3_aggregator_step(sqlite3_context *ctx, int argc, sqlite3_value **argv) } rb_iv_set(inst, "-exc_status", INT2NUM(exc_status)); + + return NULL; } -/* we assume that this function is only called once per execution context */ static void -rb_sqlite3_aggregator_final(sqlite3_context *ctx) +rb_sqlite3_aggregator_step_nogvl(sqlite3_context *ctx, int argc, sqlite3_value **argv) { + struct rb_sqlite3_aggregator_step_args args = { ctx, argc, argv }; + rb_thread_call_with_gvl(rb_sqlite3_aggregator_step, &args); +} + +/* we assume that this function is only called once per execution context */ +static void * +rb_sqlite3_aggregator_final(void *gvl_context) +{ + sqlite3_context *ctx = gvl_context; VALUE inst = rb_sqlite3_aggregate_instance(ctx); VALUE handler_instance = rb_iv_get(inst, "-handler_instance"); int exc_status = NUM2INT(rb_iv_get(inst, "-exc_status")); @@ -170,6 +190,14 @@ rb_sqlite3_aggregator_final(sqlite3_context *ctx) } rb_sqlite3_aggregate_instance_destroy(ctx); + + return NULL; +} + +static void +rb_sqlite3_aggregator_final_nogvl(sqlite3_context *ctx) +{ + rb_thread_call_with_gvl(rb_sqlite3_aggregator_final, ctx); } /* call-seq: define_aggregator2(aggregator) @@ -247,8 +275,8 @@ rb_sqlite3_define_aggregator2(VALUE self, VALUE aggregator, VALUE ruby_name) SQLITE_UTF8, (void *)aw, NULL, - rb_sqlite3_aggregator_step, - rb_sqlite3_aggregator_final + rb_sqlite3_aggregator_step_nogvl, + rb_sqlite3_aggregator_final_nogvl ); CHECK(ctx->db, status); diff --git a/ext/sqlite3/database.c b/ext/sqlite3/database.c index d61bf0ac..ef25543b 100644 --- a/ext/sqlite3/database.c +++ b/ext/sqlite3/database.c @@ -170,12 +170,26 @@ total_changes(VALUE self) return INT2NUM(sqlite3_total_changes(ctx->db)); } +struct tracefunc_args { + VALUE self; + const char *sql; +}; + +static void * +tracefunc(void *nogvl_context) +{ + struct tracefunc_args *args = nogvl_context; + VALUE thing = rb_iv_get(args->self, "@tracefunc"); + rb_funcall(thing, rb_intern("call"), 1, rb_str_new2(args->sql)); + return NULL; +} + static void -tracefunc(void *data, const char *sql) +tracefunc_nogvl(void *data, const char *sql) { - VALUE self = (VALUE)data; - VALUE thing = rb_iv_get(self, "@tracefunc"); - rb_funcall(thing, rb_intern("call"), 1, rb_str_new2(sql)); + /* This probably also needs rb_protected */ + struct tracefunc_args args = { (VALUE)data, sql }; + rb_thread_call_with_gvl(tracefunc, &args); } /* call-seq: @@ -201,18 +215,32 @@ trace(int argc, VALUE *argv, VALUE self) rb_iv_set(self, "@tracefunc", block); - sqlite3_trace(ctx->db, NIL_P(block) ? NULL : tracefunc, (void *)self); + sqlite3_trace(ctx->db, NIL_P(block) ? NULL : tracefunc_nogvl, (void *)self); return self; } -static int -rb_sqlite3_busy_handler(void *context, int count) +struct busy_handler_args { + void *context; + int count; +}; + +static void * +gvl_call_busy_handler(void *context) { - sqlite3RubyPtr ctx = (sqlite3RubyPtr)context; + struct busy_handler_args *args = context; + sqlite3RubyPtr ctx = (sqlite3RubyPtr)args->context; VALUE handle = ctx->busy_handler; - VALUE result = rb_funcall(handle, rb_intern("call"), 1, INT2NUM(count)); + VALUE result = rb_funcall(handle, rb_intern("call"), 1, INT2NUM(args->count)); + return (void *)result; +} + +static int +rb_sqlite3_busy_handler(void *context, int count) +{ + struct busy_handler_args args = {context, count}; + VALUE result = (VALUE)rb_thread_call_with_gvl(gvl_call_busy_handler, &args); if (Qfalse == result) { return 0; } @@ -393,9 +421,20 @@ set_sqlite3_func_result(sqlite3_context *ctx, VALUE result) } } -static void -rb_sqlite3_func(sqlite3_context *ctx, int argc, sqlite3_value **argv) +struct rb_sqlite3_func_args { + sqlite3_context *ctx; + int argc; + sqlite3_value **argv; +}; + +static void * +rb_sqlite3_func(void *gvl_context) { + struct rb_sqlite3_func_args *args = gvl_context; + sqlite3_context *ctx = args->ctx; + int argc = args->argc; + sqlite3_value **argv = args->argv; + VALUE callable = (VALUE)sqlite3_user_data(ctx); VALUE params = rb_ary_new2(argc); VALUE result; @@ -411,6 +450,14 @@ rb_sqlite3_func(sqlite3_context *ctx, int argc, sqlite3_value **argv) result = rb_apply(callable, rb_intern("call"), params); set_sqlite3_func_result(ctx, result); + return NULL; +} + +static void +rb_sqlite3_func_nogvl(sqlite3_context *ctx, int argc, sqlite3_value **argv) +{ + struct rb_sqlite3_func_args args = { ctx, argc, argv }; + rb_thread_call_with_gvl(rb_sqlite3_func, &args); } #ifndef HAVE_RB_PROC_ARITY @@ -444,7 +491,7 @@ define_function_with_flags(VALUE self, VALUE name, VALUE flags) rb_proc_arity(block), NUM2INT(flags), (void *)block, - rb_sqlite3_func, + rb_sqlite3_func_nogvl, NULL, NULL ); @@ -638,10 +685,19 @@ set_extended_result_codes(VALUE self, VALUE enable) return self; } -int -rb_comparator_func(void *ctx, int a_len, const void *a, int b_len, const void *b) -{ +struct comparator_func_args { VALUE comparator; + int a_len; + const char *a; + int b_len; + const char *b; +}; + +static void * +rb_comparator_func(void *context) +{ + struct comparator_func_args *args = context; + VALUE comparator = args->comparator; VALUE a_str; VALUE b_str; VALUE comparison; @@ -649,9 +705,8 @@ rb_comparator_func(void *ctx, int a_len, const void *a, int b_len, const void *b internal_encoding = rb_default_internal_encoding(); - comparator = (VALUE)ctx; - a_str = rb_str_new((const char *)a, a_len); - b_str = rb_str_new((const char *)b, b_len); + a_str = rb_str_new(args->a, args->a_len); + b_str = rb_str_new(args->b, args->b_len); rb_enc_associate_index(a_str, rb_utf8_encindex()); rb_enc_associate_index(b_str, rb_utf8_encindex()); @@ -663,7 +718,14 @@ rb_comparator_func(void *ctx, int a_len, const void *a, int b_len, const void *b comparison = rb_funcall(comparator, rb_intern("compare"), 2, a_str, b_str); - return NUM2INT(comparison); + return (void *)(VALUE)NUM2INT(comparison); +} + +static int +rb_comparator_func_nogvl(void *ctx, int a_len, const void *a, int b_len, const void *b) +{ + struct comparator_func_args args = {(VALUE)ctx, a_len, a, b_len, b}; + return (int)(VALUE)rb_thread_call_with_gvl(rb_comparator_func, &args); } /* call-seq: db.collation(name, comparator) @@ -685,7 +747,7 @@ collation(VALUE self, VALUE name, VALUE comparator) StringValuePtr(name), SQLITE_UTF8, (void *)comparator, - NIL_P(comparator) ? NULL : rb_comparator_func)); + NIL_P(comparator) ? NULL : rb_comparator_func_nogvl)); /* Make sure our comparator doesn't get garbage collected. */ rb_hash_aset(rb_iv_get(self, "@collations"), name, comparator); diff --git a/ext/sqlite3/sqlite3_ruby.h b/ext/sqlite3/sqlite3_ruby.h index 088d3cd5..cbee9e98 100644 --- a/ext/sqlite3/sqlite3_ruby.h +++ b/ext/sqlite3/sqlite3_ruby.h @@ -2,6 +2,7 @@ #define SQLITE3_RUBY #include +#include #ifdef UNUSED #elif defined(__GNUC__) diff --git a/ext/sqlite3/statement.c b/ext/sqlite3/statement.c index cb65efb7..4d6d0bf7 100644 --- a/ext/sqlite3/statement.c +++ b/ext/sqlite3/statement.c @@ -110,6 +110,10 @@ closed_p(VALUE self) return Qfalse; } +void *step_without_gvl(void *data) { + return (void *)(VALUE)sqlite3_step(data); +} + static VALUE step(VALUE self) { @@ -129,7 +133,12 @@ step(VALUE self) stmt = ctx->st; +#if 0 value = sqlite3_step(stmt); +#else + value = (VALUE)rb_thread_call_without_gvl(step_without_gvl, (void *)stmt, NULL, NULL); +#endif + if (rb_errinfo() != Qnil) { /* some user defined function was invoked as a callback during step and * it raised an exception that has been suppressed until step returns.