From b2ad4fec1a369e1cbd0c65d52062946a4fbfb84b Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Fri, 8 Dec 2023 21:16:02 -0800 Subject: [PATCH] Add missing GVL hooks for M:N threads and ractors --- test/-ext-/thread/helper.rb | 51 ++++++++++ test/-ext-/thread/test_instrumentation_api.rb | 95 ++++++++++--------- thread_pthread.c | 2 + thread_pthread_mn.c | 3 + 4 files changed, 105 insertions(+), 46 deletions(-) create mode 100644 test/-ext-/thread/helper.rb diff --git a/test/-ext-/thread/helper.rb b/test/-ext-/thread/helper.rb new file mode 100644 index 00000000000000..3ea2057d15f946 --- /dev/null +++ b/test/-ext-/thread/helper.rb @@ -0,0 +1,51 @@ +module ThreadInstrumentation + module TestHelper + private + + def record + Bug::ThreadInstrumentation.register_callback(!ENV["GVL_DEBUG"]) + yield + ensure + timeline = Bug::ThreadInstrumentation.unregister_callback + if $! + raise + else + return timeline + end + end + + def timeline_for(thread, timeline) + timeline.select { |t, _| t == thread }.map(&:last) + end + + def assert_consistent_timeline(events) + refute_predicate events, :empty? + + previous_event = nil + events.each do |event| + refute_equal :exited, previous_event, "`exited` must be the final event: #{events.inspect}" + case event + when :started + assert_nil previous_event, "`started` must be the first event: #{events.inspect}" + when :ready + unless previous_event.nil? + assert %i(started suspended).include?(previous_event), "`ready` must be preceded by `started` or `suspended`: #{events.inspect}" + end + when :resumed + unless previous_event.nil? + assert_equal :ready, previous_event, "`resumed` must be preceded by `ready`: #{events.inspect}" + end + when :suspended + unless previous_event.nil? + assert_equal :resumed, previous_event, "`suspended` must be preceded by `resumed`: #{events.inspect}" + end + when :exited + unless previous_event.nil? + assert %i(resumed suspended).include?(previous_event), "`exited` must be preceded by `resumed` or `suspended`: #{events.inspect}" + end + end + previous_event = event + end + end + end +end diff --git a/test/-ext-/thread/test_instrumentation_api.rb b/test/-ext-/thread/test_instrumentation_api.rb index ef3b8358b3999b..c9480c285f9ba5 100644 --- a/test/-ext-/thread/test_instrumentation_api.rb +++ b/test/-ext-/thread/test_instrumentation_api.rb @@ -1,7 +1,10 @@ # frozen_string_literal: false require 'envutil' +require_relative "helper" class TestThreadInstrumentation < Test::Unit::TestCase + include ThreadInstrumentation::TestHelper + def setup pend("No windows support") if /mswin|mingw|bccwin/ =~ RUBY_PLATFORM @@ -131,6 +134,52 @@ def test_queue_releases_gvl assert_equal %i(started ready resumed suspended ready resumed suspended exited), timeline end + def test_blocking_on_ractor + assert_ractor(<<-"RUBY", require_relative: "helper", require: "-test-/thread/instrumentation") + include ThreadInstrumentation::TestHelper + + ractor = Ractor.new { + Ractor.receive # wait until woke + Thread.current + } + + # Wait for the main thread to block, then wake the ractor + Thread.new do + while Thread.main.status != "sleep" + Thread.pass + end + ractor.send true + end + + full_timeline = record do + ractor.take + end + + timeline = timeline_for(Thread.current, full_timeline) + assert_consistent_timeline(timeline) + assert_equal %i(suspended ready resumed), timeline + RUBY + end + + def test_sleeping_inside_ractor + assert_ractor(<<-"RUBY", require_relative: "helper", require: "-test-/thread/instrumentation") + include ThreadInstrumentation::TestHelper + + thread = nil + + full_timeline = record do + thread = Ractor.new{ + sleep 0.1 + Thread.current + }.take + end + + timeline = timeline_for(thread, full_timeline) + assert_consistent_timeline(timeline) + assert_equal %i(started ready resumed suspended ready resumed suspended exited), timeline + RUBY + end + def test_thread_blocked_forever_on_mutex mutex = Mutex.new mutex.lock @@ -205,52 +254,6 @@ def test_thread_instrumentation_unregister private - def record - Bug::ThreadInstrumentation.register_callback(!ENV["GVL_DEBUG"]) - yield - ensure - timeline = Bug::ThreadInstrumentation.unregister_callback - if $! - raise - else - return timeline - end - end - - def assert_consistent_timeline(events) - refute_predicate events, :empty? - - previous_event = nil - events.each do |event| - refute_equal :exited, previous_event, "`exited` must be the final event: #{events.inspect}" - case event - when :started - assert_nil previous_event, "`started` must be the first event: #{events.inspect}" - when :ready - unless previous_event.nil? - assert %i(started suspended).include?(previous_event), "`ready` must be preceded by `started` or `suspended`: #{events.inspect}" - end - when :resumed - unless previous_event.nil? - assert_equal :ready, previous_event, "`resumed` must be preceded by `ready`: #{events.inspect}" - end - when :suspended - unless previous_event.nil? - assert_equal :resumed, previous_event, "`suspended` must be preceded by `resumed`: #{events.inspect}" - end - when :exited - unless previous_event.nil? - assert %i(resumed suspended).include?(previous_event), "`exited` must be preceded by `resumed` or `suspended`: #{events.inspect}" - end - end - previous_event = event - end - end - - def timeline_for(thread, timeline) - timeline.select { |t, _| t == thread }.map(&:last) - end - def fib(n = 30) return n if n <= 1 fib(n-1) + fib(n-2) diff --git a/thread_pthread.c b/thread_pthread.c index a8cf8c612660b0..64b636ec08b390 100644 --- a/thread_pthread.c +++ b/thread_pthread.c @@ -1304,6 +1304,8 @@ rb_ractor_sched_sleep(rb_execution_context_t *ec, rb_ractor_t *cr, rb_unblock_fu RB_VM_SAVE_MACHINE_CONTEXT(th); th->status = THREAD_STOPPED_FOREVER; + RB_INTERNAL_THREAD_HOOK(RUBY_INTERNAL_THREAD_EVENT_SUSPENDED, th); + bool can_direct_transfer = !th_has_dedicated_nt(th); thread_sched_wakeup_next_thread(sched, th, can_direct_transfer); thread_sched_wait_running_turn(sched, th, can_direct_transfer); diff --git a/thread_pthread_mn.c b/thread_pthread_mn.c index aa69515f320e48..e516d787b38153 100644 --- a/thread_pthread_mn.c +++ b/thread_pthread_mn.c @@ -74,6 +74,8 @@ thread_sched_wait_events(struct rb_thread_sched *sched, rb_thread_t *th, int fd, RB_VM_SAVE_MACHINE_CONTEXT(th); setup_ubf(th, ubf_event_waiting, (void *)th); + RB_INTERNAL_THREAD_HOOK(RUBY_INTERNAL_THREAD_EVENT_SUSPENDED, th); + thread_sched_lock(sched, th); { if (th->sched.waiting_reason.flags == thread_sched_waiting_none) { @@ -418,6 +420,7 @@ co_start(struct coroutine_context *from, struct coroutine_context *self) thread_sched_add_running_thread(TH_SCHED(th), th); thread_sched_unlock(sched, th); { + RB_INTERNAL_THREAD_HOOK(RUBY_INTERNAL_THREAD_EVENT_RESUMED, th); call_thread_start_func_2(th); } thread_sched_lock(sched, NULL);