From 0ba488d7f51c8b52811445245c87cb824e564069 Mon Sep 17 00:00:00 2001 From: Alan Wu Date: Fri, 8 Aug 2025 14:54:53 -0400 Subject: [PATCH 1/4] ZJIT: Avoid compiling and direct sends to forwardable ISEQs These `...` ISEQs have a special calling convention in the interpreter and our stubs and JIT calling convention don't deal well. Reject for now. Debugged with help from `@tekknolagi` and `tool/zjit_bisect.rb`. Merely avoiding direct sends is enough to pass the attached test, but also avoid compiling ISEQs with `...` parameter to limit exposure for now. `SendWithoutBlock`, which does dynamic dispatch using interpreter code, seems to handle calling into forwardable ISEQs correctly, so they are fine -- we can't predict where these dynamic sends land anyways. --- test/ruby/test_zjit.rb | 9 +++++++++ zjit/src/hir.rs | 30 +++++++++++++----------------- 2 files changed, 22 insertions(+), 17 deletions(-) diff --git a/test/ruby/test_zjit.rb b/test/ruby/test_zjit.rb index e040463bbf5868..c86ac62a9f92ca 100644 --- a/test/ruby/test_zjit.rb +++ b/test/ruby/test_zjit.rb @@ -70,6 +70,15 @@ def test_setlocal_on_eval } end + def test_call_a_forwardable_method + assert_runs '[]', %q{ + def test_root = forwardable + def forwardable(...) = Array.[](...) + test_root + test_root + }, call_threshold: 2 + end + def test_setlocal_on_eval_with_spill assert_compiles '1', %q{ @b = binding diff --git a/zjit/src/hir.rs b/zjit/src/hir.rs index f2f990afdeada2..87d2a613d0e930 100644 --- a/zjit/src/hir.rs +++ b/zjit/src/hir.rs @@ -969,6 +969,7 @@ fn can_direct_send(iseq: *const rb_iseq_t) -> bool { else if unsafe { rb_get_iseq_flags_has_kw(iseq) } { false } else if unsafe { rb_get_iseq_flags_has_kwrest(iseq) } { false } else if unsafe { rb_get_iseq_flags_has_block(iseq) } { false } + else if unsafe { rb_get_iseq_flags_forwardable(iseq) } { false } else { true } } @@ -2581,6 +2582,9 @@ pub enum CallType { #[derive(Debug, PartialEq)] pub enum ParameterType { Optional, + /// For example, `foo(...)`. Interaction of JIT + /// calling convention and side exits currently unsolved. + Forwardable, } #[derive(Debug, PartialEq)] @@ -2650,6 +2654,7 @@ pub const SELF_PARAM_IDX: usize = 0; fn filter_unknown_parameter_type(iseq: *const rb_iseq_t) -> Result<(), ParseError> { if unsafe { rb_get_iseq_body_param_opt_num(iseq) } != 0 { return Err(ParseError::UnknownParameterType(ParameterType::Optional)); } + if unsafe { rb_get_iseq_flags_forwardable(iseq) } { return Err(ParseError::UnknownParameterType(ParameterType::Forwardable)); } Ok(()) } @@ -4583,11 +4588,13 @@ mod tests { eval(" def test(...) = super(...) "); - assert_method_hir("test", expect![[r#" - fn test@:2: - bb0(v0:BasicObject, v1:BasicObject): - SideExit UnknownOpcode(invokesuperforward) - "#]]); + assert_compile_fails("test", ParseError::UnknownParameterType(ParameterType::Forwardable)); + } + + #[test] + fn test_cant_compile_forwardable() { + eval("def forwardable(...) = nil"); + assert_compile_fails("forwardable", ParseError::UnknownParameterType(ParameterType::Forwardable)); } // TODO(max): Figure out how to generate a call with OPT_SEND flag @@ -4631,11 +4638,7 @@ mod tests { eval(" def test(...) = foo(...) "); - assert_method_hir("test", expect![[r#" - fn test@:2: - bb0(v0:BasicObject, v1:BasicObject): - SideExit UnknownOpcode(sendforward) - "#]]); + assert_compile_fails("test", ParseError::UnknownParameterType(ParameterType::Forwardable)); } #[test] @@ -5691,7 +5694,6 @@ mod opt_tests { def kw_rest(**k) = k def post(*rest, post) = post def block(&b) = nil - def forwardable(...) = nil "); assert_optimized_method_hir("rest", expect![[r#" @@ -5721,12 +5723,6 @@ mod opt_tests { bb0(v0:BasicObject, v1:ArrayExact, v2:BasicObject): Return v2 "#]]); - assert_optimized_method_hir("forwardable", expect![[r#" - fn forwardable@:7: - bb0(v0:BasicObject, v1:BasicObject): - v3:NilClass = Const Value(nil) - Return v3 - "#]]); } #[test] From e639e5fd1af51e2462879d6db862ee5320914ba7 Mon Sep 17 00:00:00 2001 From: Peter Zhu Date: Fri, 8 Aug 2025 15:04:48 -0400 Subject: [PATCH 2/4] Make rb_gc_impl_writebarrier_remember Ractor-safe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit rb_gc_impl_writebarrier_remember is not Ractor safe because it writes to bitmaps and also pushes onto the mark stack during incremental marking. We should acquire the VM lock to prevent race conditions. In the case that the object is not old, there is no performance impact. However, we can see a performance impact in this microbenchmark where the object is old: 4.times.map do Ractor.new do ary = [] 3.times { GC.start } 10_000_000.times do |i| ary.push(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17) ary.clear end end end.map(&:value) Before: Time (mean ± σ): 682.4 ms ± 5.1 ms [User: 2564.8 ms, System: 16.0 ms] After: Time (mean ± σ): 5.522 s ± 0.096 s [User: 8.237 s, System: 7.931 s] Co-Authored-By: Luke Gruber Co-Authored-By: John Hawthorn --- gc/default/default.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/gc/default/default.c b/gc/default/default.c index 9038a01e4e88a8..d4e34b9d0379ee 100644 --- a/gc/default/default.c +++ b/gc/default/default.c @@ -6110,15 +6110,19 @@ rb_gc_impl_writebarrier_remember(void *objspace_ptr, VALUE obj) gc_report(1, objspace, "rb_gc_writebarrier_remember: %s\n", rb_obj_info(obj)); - if (is_incremental_marking(objspace)) { - if (RVALUE_BLACK_P(objspace, obj)) { - gc_grey(objspace, obj); - } - } - else { - if (RVALUE_OLD_P(objspace, obj)) { - rgengc_remember(objspace, obj); + if (is_incremental_marking(objspace) || RVALUE_OLD_P(objspace, obj)) { + int lev = RB_GC_VM_LOCK_NO_BARRIER(); + { + if (is_incremental_marking(objspace)) { + if (RVALUE_BLACK_P(objspace, obj)) { + gc_grey(objspace, obj); + } + } + else if (RVALUE_OLD_P(objspace, obj)) { + rgengc_remember(objspace, obj); + } } + RB_GC_VM_UNLOCK_NO_BARRIER(lev); } } From 07878ebe787843f510be460738ff02dd883bf9ad Mon Sep 17 00:00:00 2001 From: Luke Gruber Date: Mon, 23 Jun 2025 14:33:52 -0400 Subject: [PATCH 3/4] Fix lock ordering issue for rb_ractor_sched_wait() and rb_ractor_sched_wakeup() In rb_ractor_sched_wait() (ex: Ractor.receive), we acquire RACTOR_LOCK(cr) and then thread_sched_lock(cur_th). However, on wakeup if we're a dnt, in thread_sched_wait_running_turn() we acquire thread_sched_lock(cur_th) after condvar wakeup and then RACTOR_LOCK(cr). This lock inversion can cause a deadlock with rb_ractor_wakeup_all() (ex: port.send(obj)), where we acquire RACTOR_LOCK(other_r) and then thread_sched_lock(other_th). So, the error happens: nt 1: Ractor.receive rb_ractor_sched_wait() after condvar wakeup in thread_sched_wait_running_turn(): - thread_sched_lock(cur_th) (condvar) # acquires lock - rb_ractor_lock_self(cr) # deadlock here: tries to acquire, HANGS nt 2: port.send ractor_wakeup_all() - RACTOR_LOCK(port_r) # acquires lock - thread_sched_lock # tries to acquire, HANGS To fix it, we now unlock the thread_sched_lock before acquiring the ractor_lock in rb_ractor_sched_wait(). Script that reproduces issue: ```ruby require "async" class RactorWrapper def initialize @ractor = Ractor.new do Ractor.recv # Ractor doesn't start until explicitly told to # Do some calculations fib = ->(x) { x < 2 ? 1 : fib.call(x - 1) + fib.call(x - 2) } fib.call(20) end end def take_async @ractor.send(nil) Thread.new { @ractor.value }.value end end Async do |task| 10_000.times do |i| task.async do RactorWrapper.new.take_async puts i end end end exit 0 ``` Fixes [Bug #21398] Co-authored-by: John Hawthorn --- test/ruby/test_ractor.rb | 39 +++++++++++++++++++++++++++++++++++++++ thread_pthread.c | 13 +++++-------- 2 files changed, 44 insertions(+), 8 deletions(-) diff --git a/test/ruby/test_ractor.rb b/test/ruby/test_ractor.rb index 0a456a1d0f5e1f..74de2bf9cd1af5 100644 --- a/test/ruby/test_ractor.rb +++ b/test/ruby/test_ractor.rb @@ -162,6 +162,45 @@ def test_require_non_string RUBY end + # [Bug #21398] + def test_port_receive_dnt_with_port_send + assert_ractor(<<~'RUBY', timeout: 30) + THREADS = 10 + JOBS_PER_THREAD = 50 + ARRAY_SIZE = 20_000 + def ractor_job(job_count, array_size) + port = Ractor::Port.new + workers = (1..4).map do |i| + Ractor.new(port) do |job_port| + while job = Ractor.receive + result = job.map { |x| x * 2 }.sum + job_port.send result + end + end + end + jobs = Array.new(job_count) { Array.new(array_size) { rand(1000) } } + jobs.each_with_index do |job, i| + w_idx = i % 4 + workers[w_idx].send(job) + end + results = [] + jobs.size.times do + result = port.receive # dnt receive + results << result + end + results + end + threads = [] + # creates 40 ractors (THREADSx4) + THREADS.times do + threads << Thread.new do + ractor_job(JOBS_PER_THREAD, ARRAY_SIZE) + end + end + threads.each(&:join) + RUBY + end + def assert_make_shareable(obj) refute Ractor.shareable?(obj), "object was already shareable" Ractor.make_shareable(obj) diff --git a/thread_pthread.c b/thread_pthread.c index 377e1d9f64a4c5..730ecb54163f7d 100644 --- a/thread_pthread.c +++ b/thread_pthread.c @@ -1351,6 +1351,7 @@ rb_ractor_sched_wait(rb_execution_context_t *ec, rb_ractor_t *cr, rb_unblock_fun } thread_sched_lock(sched, th); + rb_ractor_unlock_self(cr); { // setup sleep bool can_direct_transfer = !th_has_dedicated_nt(th); @@ -1358,16 +1359,12 @@ rb_ractor_sched_wait(rb_execution_context_t *ec, rb_ractor_t *cr, rb_unblock_fun th->status = THREAD_STOPPED_FOREVER; RB_INTERNAL_THREAD_HOOK(RUBY_INTERNAL_THREAD_EVENT_SUSPENDED, th); thread_sched_wakeup_next_thread(sched, th, can_direct_transfer); - - rb_ractor_unlock_self(cr); - { - // sleep - thread_sched_wait_running_turn(sched, th, can_direct_transfer); - th->status = THREAD_RUNNABLE; - } - rb_ractor_lock_self(cr); + // sleep + thread_sched_wait_running_turn(sched, th, can_direct_transfer); + th->status = THREAD_RUNNABLE; } thread_sched_unlock(sched, th); + rb_ractor_lock_self(cr); ubf_clear(th); From d80c03d22a5e92a5423a18da1d6494c484392c87 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Fri, 8 Aug 2025 16:31:29 -0700 Subject: [PATCH 4/4] Fix id2ref table build when GC in progress Previously, if GC was in progress when we're initially building the id2ref table, it could see the empty table and then crash when trying to remove ids from it. This commit fixes the bug by only publishing the table after GC is done. Co-authored-by: Aaron Patterson --- gc.c | 7 +++++-- test/ruby/test_objectspace.rb | 15 +++++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/gc.c b/gc.c index 3d0935ad1a4f10..4c8a042c1e8513 100644 --- a/gc.c +++ b/gc.c @@ -1975,14 +1975,17 @@ object_id_to_ref(void *objspace_ptr, VALUE object_id) // GC Must not trigger while we build the table, otherwise if we end // up freeing an object that had an ID, we might try to delete it from // the table even though it wasn't inserted yet. - id2ref_tbl = st_init_table(&object_id_hash_type); - id2ref_value = TypedData_Wrap_Struct(0, &id2ref_tbl_type, id2ref_tbl); + st_table *tmp_id2ref_tbl = st_init_table(&object_id_hash_type); + VALUE tmp_id2ref_value = TypedData_Wrap_Struct(0, &id2ref_tbl_type, tmp_id2ref_tbl); // build_id2ref_i will most certainly malloc, which could trigger GC and sweep // objects we just added to the table. // By calling rb_gc_disable() we also save having to handle potentially garbage objects. bool gc_disabled = RTEST(rb_gc_disable()); { + id2ref_tbl = tmp_id2ref_tbl; + id2ref_value = tmp_id2ref_value; + rb_gc_impl_each_object(objspace, build_id2ref_i, (void *)id2ref_tbl); } if (!gc_disabled) rb_gc_enable(); diff --git a/test/ruby/test_objectspace.rb b/test/ruby/test_objectspace.rb index f27f586ab79fca..a479547599a03a 100644 --- a/test/ruby/test_objectspace.rb +++ b/test/ruby/test_objectspace.rb @@ -284,6 +284,21 @@ def test_each_object_recursive_key end; end + def test_id2ref_table_build + assert_separately([], <<-End) + 10.times do + Object.new.object_id + end + + GC.start(immediate_mark: false) + + obj = Object.new + EnvUtil.suppress_warning do + assert_equal obj, ObjectSpace._id2ref(obj.object_id) + end + End + end + def test_each_object_singleton_class assert_separately([], <<-End) class C