From 85bc80a51be0ceedcc57e7b6b779e6f8f885859e Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Sun, 3 Dec 2023 18:13:25 -0800 Subject: [PATCH] Revert "Add missing GVL hooks for M:N threads and ractors" This reverts commit ad54fbf281ca1935e79f4df1460b0106ba76761e. --- test/-ext-/thread/helper.rb | 51 ----------- test/-ext-/thread/test_instrumentation_api.rb | 84 ++++++++++--------- thread_pthread.c | 2 - thread_pthread_mn.c | 3 - 4 files changed, 46 insertions(+), 94 deletions(-) delete mode 100644 test/-ext-/thread/helper.rb diff --git a/test/-ext-/thread/helper.rb b/test/-ext-/thread/helper.rb deleted file mode 100644 index 3ea2057d15f946..00000000000000 --- a/test/-ext-/thread/helper.rb +++ /dev/null @@ -1,51 +0,0 @@ -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 6ebdf3eac202e8..ef3b8358b3999b 100644 --- a/test/-ext-/thread/test_instrumentation_api.rb +++ b/test/-ext-/thread/test_instrumentation_api.rb @@ -1,10 +1,7 @@ # 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 @@ -134,41 +131,6 @@ 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 - - full_timeline = record do - Ractor.new{ - Thread.current - }.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 @@ -243,6 +205,52 @@ 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 64b636ec08b390..a8cf8c612660b0 100644 --- a/thread_pthread.c +++ b/thread_pthread.c @@ -1304,8 +1304,6 @@ 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 e516d787b38153..aa69515f320e48 100644 --- a/thread_pthread_mn.c +++ b/thread_pthread_mn.c @@ -74,8 +74,6 @@ 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) { @@ -420,7 +418,6 @@ 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);