Skip to content

Commit

Permalink
Add missing GVL hooks for M:N threads and ractors
Browse files Browse the repository at this point in the history
[Bug #20019]

This fixes GVL instrumentation in three locations it was missing:
- Suspending when blocking on a Ractor
- Suspending when doing a coroutine transfer from an M:N thread
- Resuming after an M:N thread starts

Co-authored-by: Matthew Draper <matthew@trebex.net>
  • Loading branch information
jhawthorn and matthewd committed Dec 2, 2023
1 parent 715cf9b commit ad54fbf
Show file tree
Hide file tree
Showing 4 changed files with 94 additions and 46 deletions.
51 changes: 51 additions & 0 deletions 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
84 changes: 38 additions & 46 deletions 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

Expand Down Expand Up @@ -131,6 +134,41 @@ 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
Expand Down Expand Up @@ -205,52 +243,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)
Expand Down
2 changes: 2 additions & 0 deletions thread_pthread.c
Expand Up @@ -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);
Expand Down
3 changes: 3 additions & 0 deletions thread_pthread_mn.c
Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
Expand Down

0 comments on commit ad54fbf

Please sign in to comment.