ruby-changes:66460
From: Takashi <ko1@a...>
Date: Thu, 10 Jun 2021 16:36:50 +0900 (JST)
Subject: [ruby-changes:66460] c5e8a49bde (master): Avoid enqueueing the same ISeq twice
https://git.ruby-lang.org/ruby.git/commit/?id=c5e8a49bde From c5e8a49bdeadd8e424274c17c0d2a9ffed64417b Mon Sep 17 00:00:00 2001 From: Takashi Kokubun <takashikkbn@g...> Date: Thu, 10 Jun 2021 00:32:15 -0700 Subject: Avoid enqueueing the same ISeq twice by a race condition by multiple Ractors. Atmically incrementing body->total_calls may have its own cost, so for now we intentionally leave the unreliable total_calls. So we allow an ISeq to be never pushed when you use multiple Ractors. However, if you enqueue a single ccan node twice, get_from_list loops infinitely. Thus this patch takes care of such a situation. --- common.mk | 1 + mjit.c | 30 ++++++++++++++++++++---------- mjit.h | 2 +- mjit_worker.c | 2 +- 4 files changed, 23 insertions(+), 12 deletions(-) diff --git a/common.mk b/common.mk index 626040a..27a643c 100644 --- a/common.mk +++ b/common.mk @@ -8608,6 +8608,7 @@ mjit.$(OBJEXT): {$(VPATH)}util.h https://github.com/ruby/ruby/blob/trunk/common.mk#L8608 mjit.$(OBJEXT): {$(VPATH)}vm_callinfo.h mjit.$(OBJEXT): {$(VPATH)}vm_core.h mjit.$(OBJEXT): {$(VPATH)}vm_opts.h +mjit.$(OBJEXT): {$(VPATH)}vm_sync.h mjit_build_dir.$(OBJEXT): {$(VPATH)}config.h mjit_build_dir.$(OBJEXT): {$(VPATH)}internal/compiler_is.h mjit_build_dir.$(OBJEXT): {$(VPATH)}internal/compiler_is/apple.h diff --git a/mjit.c b/mjit.c index 3379b34..fb55cdc 100644 --- a/mjit.c +++ b/mjit.c @@ -23,6 +23,7 @@ https://github.com/ruby/ruby/blob/trunk/mjit.c#L23 #include "internal/file.h" #include "internal/hash.h" #include "internal/warnings.h" +#include "vm_sync.h" #include "mjit_worker.c" @@ -253,18 +254,28 @@ mjit_target_iseq_p(struct rb_iseq_constant_body *body) https://github.com/ruby/ruby/blob/trunk/mjit.c#L254 && !body->builtin_inline_p; } -// This is called from an MJIT worker when worker_p is true. +// If recompile_p is true, the call is initiated by mjit_recompile. +// This assumes the caller holds CRITICAL_SECTION when recompile_p is true. static void -mjit_add_iseq_to_process(const rb_iseq_t *iseq, const struct rb_mjit_compile_info *compile_info, bool worker_p) +mjit_add_iseq_to_process(const rb_iseq_t *iseq, const struct rb_mjit_compile_info *compile_info, bool recompile_p) { if (!mjit_enabled || pch_status == PCH_FAILED) return; - if (!mjit_target_iseq_p(iseq->body)) { iseq->body->jit_func = (mjit_func_t)NOT_COMPILED_JIT_ISEQ_FUNC; // skip mjit_wait return; } + if (!recompile_p) { + CRITICAL_SECTION_START(3, "in add_iseq_to_process"); + + // This prevents multiple Ractors from enqueueing the same ISeq twice. + if (rb_multi_ractor_p() && iseq->body->jit_func != NOT_ADDED_JIT_ISEQ_FUNC) { + CRITICAL_SECTION_FINISH(3, "in add_iseq_to_process"); + return; + } + } + RB_DEBUG_COUNTER_INC(mjit_add_iseq_to_process); iseq->body->jit_func = (mjit_func_t)NOT_READY_JIT_ISEQ_FUNC; create_unit(iseq); @@ -273,15 +284,12 @@ mjit_add_iseq_to_process(const rb_iseq_t *iseq, const struct rb_mjit_compile_inf https://github.com/ruby/ruby/blob/trunk/mjit.c#L284 return; if (compile_info != NULL) iseq->body->jit_unit->compile_info = *compile_info; - - if (!worker_p) { - CRITICAL_SECTION_START(3, "in add_iseq_to_process"); - } add_to_list(iseq->body->jit_unit, &unit_queue); if (active_units.length >= mjit_opts.max_cache_size) { unload_requests++; } - if (!worker_p) { + + if (!recompile_p) { verbose(3, "Sending wakeup signal to workers in mjit_add_iseq_to_process"); rb_native_cond_broadcast(&mjit_worker_wakeup); CRITICAL_SECTION_FINISH(3, "in add_iseq_to_process"); @@ -356,9 +364,11 @@ mjit_recompile(const rb_iseq_t *iseq) https://github.com/ruby/ruby/blob/trunk/mjit.c#L364 assert(iseq->body->jit_unit != NULL); if (UNLIKELY(mjit_opts.wait)) { + CRITICAL_SECTION_START(3, "in rb_mjit_recompile_iseq"); remove_from_list(iseq->body->jit_unit, &active_units); add_to_list(iseq->body->jit_unit, &stale_units); - mjit_add_iseq_to_process(iseq, &iseq->body->jit_unit->compile_info, false); + mjit_add_iseq_to_process(iseq, &iseq->body->jit_unit->compile_info, true); + CRITICAL_SECTION_FINISH(3, "in rb_mjit_recompile_iseq"); mjit_wait(iseq->body); } else { @@ -367,7 +377,7 @@ mjit_recompile(const rb_iseq_t *iseq) https://github.com/ruby/ruby/blob/trunk/mjit.c#L377 // It's good to avoid a race condition between mjit_add_iseq_to_process and mjit_compile around jit_unit as well. CRITICAL_SECTION_START(3, "in rb_mjit_recompile_iseq"); iseq->body->jit_unit->stale_p = true; - iseq->body->jit_func = (mjit_func_t)NOT_ADDED_JIT_ISEQ_FUNC; + iseq->body->jit_func = (mjit_func_t)NOT_READY_JIT_ISEQ_FUNC; pending_stale_p = true; CRITICAL_SECTION_FINISH(3, "in rb_mjit_recompile_iseq"); } diff --git a/mjit.h b/mjit.h index 8cfc3d0..4d27e39 100644 --- a/mjit.h +++ b/mjit.h @@ -20,7 +20,7 @@ https://github.com/ruby/ruby/blob/trunk/mjit.h#L20 // Special address values of a function generated from the // corresponding iseq by MJIT: enum rb_mjit_iseq_func { - // ISEQ was not queued yet for the machine code generation + // ISEQ has never been enqueued to unit_queue yet NOT_ADDED_JIT_ISEQ_FUNC = 0, // ISEQ is already queued for the machine code generation but the // code is not ready yet for the execution diff --git a/mjit_worker.c b/mjit_worker.c index bfcf8c0..046d3a9 100644 --- a/mjit_worker.c +++ b/mjit_worker.c @@ -1106,7 +1106,7 @@ load_func_from_so(const char *so_file, const char *funcname, struct rb_mjit_unit https://github.com/ruby/ruby/blob/trunk/mjit_worker.c#L1106 handle = dlopen(so_file, RTLD_NOW); if (handle == NULL) { mjit_warning("failure in loading code from '%s': %s", so_file, dlerror()); - return (void *)NOT_ADDED_JIT_ISEQ_FUNC; + return (void *)NOT_COMPILED_JIT_ISEQ_FUNC; } func = dlsym(handle, funcname); -- cgit v1.1 -- ML: ruby-changes@q... Info: http://www.atdot.net/~ko1/quickml/