I really like this approach. I haven't played with it much (I will next week) but I have a few thoughts:

1. This only works for dedicated threads, so not for ractors or with RUBY_MN_THREADS=1 (unlike the background thread implementation). This is okay for now, I think. One way to make this work for ractors would be to dynamically use hot thread swapback if the number of ractors is smaller than the number of cores, or something similar. If there are a large number of ractors, we can't expect our ractor to be scheduled again before the end of the system call.

2. Hopefully this doesn't lead to starvation of other ruby threads if 1 thread is running lots of fast system calls in a row. I think the timer thread will take care of it but it's worth checking.

3. I'm not sure about the thread_sched_unlock directly followed by thread_sched_lock in wait_running_turn for the hot thread. Giving this hot thread a last chance might not make sense because the thread probably isn't spinning. Chances are it's been put to sleep by the OS if it isn't in the system call anymore.

Possible optimizations

1. It would be nice if we didn't have to take the hot thread off the timeslice list and then put it back on in this case. It's a lot of bookkeeping overhead at the moment. If we skipped these, the hot thread would have to deal with joining a barrier if one is called for, though.

2. Threads should respect their priority, so I don't think a thread should set itself "hot" if its priority is < 0.

I've been using puma's benchmark suite as more "real-world" benchmarks for thread scheduling.
You can clone the repo and run, for example:
bundle install
./benchmarks/local/response_time_wrk.sh -w2 -s tcp -Y

Thanks for having a look @luke-gruber!

On your first set of thoughts:

1. It's true this works for native threads only, but I thought that would include native threads on ractors?
2. IIUC, the Ruby quantum would expire and the thread would still voluntarily yield. The voluntary yield doesn't set the runnable_hot_th, so it cannot steal back. So it should not lead to starvation. It should behave more like a CPU thread that wasn't making any syscalls.
3. This makes a big difference for multi-core performance. pthread_mutex uses a spinlock internally. The sequence to consider is:

 T1 is running. sets runnable_hot_th. yields GVL to thread2. drops sched mutex.
 T1 makes fast syscall. 
 T2 wakes up
 T1 completes syscall.
 T2 grabs sched mutex.
 T1 tries to get sched mutex. Starts spinning.

The above only happens on multi-core, when thread1 and thread2 are running on different cores. Allowing thread2 to proceed results in a core-switch for the process which is quite expensive. The simple drop/acquire allows thread1 to continue IFF it's actively spinning, which can only happen if it's running on a different core.

Regarding the optimizations

1. I think adding/removing from the readyq is only a handful of instructions? It's a doubly-linked-list. This seemed small compared to everything else the scheduler is doing (mutex, cond-variables, etc.) Or are you referring to something else?
2. "hot" is meant to imply that the cache is warm for the given thread on the core where it's running. It isn't meant to imply priority. We could certainly use a different name for this field if it's confusing.

Thanks for the benchmarks pointer! I'll give them a try.

It's true this works for native threads only, but I thought that would include native threads on ractors?

It does if SNT is parked (it's considered a DNT then) so actually it does, my mistake. It looks like there's a problem with the unlock then lock of the sched when RUBY_MN_THREADS=1. Try puma's benchmarks but set RUBY_MN_THREADS=1. I'm getting a crash. I'm guessing it has to do with snt/dnt bookkeeping on the native thread.

We could always enable this optimization for the main ractor and only without RUBY_MN_THREADS enabled until we figure out a good solution:

 if (!yield_immediately && !sched->enable_mn_threads && th->ractor == th->vm->ractor.main_ractor) {
        RUBY_ASSERT(!sched->runnable_hot_th);
        sched->runnable_hot_th = th;
)

It should behave more like a CPU thread that wasn't making any syscalls.

What I was worried about was if the syscall is always fast and the hot thread always steals control back, and let's say there's a loop like this:

loop do
   # fast syscall, hot thread always wins
end

The thread calling the syscall may never yield because the time between the next syscall is very minimal and the timer thread only checks interrupts every 10ms. Fortunately, it looks like even for fast syscalls, the hot thread doesn't always win so eventually it yields.

The simple drop/acquire allows thread1 to continue IFF it's actively spinning, which can only happen if it's running on a different core

I guess the assumption is that if the non-hot thread acquires the mutex, the hot thread must either still be in the syscall or be spinning and not put back to sleep. This is probably true because there aren't many instructions between the wakeup of the non-hot thread and the unlock.

I think adding/removing from the readyq is only a handful of instructions?

I was thinking about it taking the ractor_sched_lock, which is very contended with ractors. I'll have to investigate to see how this interacts with ractors.

We could certainly use a different name for this field if it's confusing.

No, it's fine as is. I was thinking it's kind of weird if a low priority thread signals another thread to run, does a fast syscall, takes control back and then makes the other thread wakeup and go right back asleep.

I made similar idea in last year:

1. Th1 into waiting, but don't set the next thread (Th2), but only wakeup next thread (Th2) (sched->running == NULL).
2. Th1 or Th2 will take a GVL (fill sched->running with either of them)

But I observed slowdown on my benchmark.

My understanding of this PR is:

1. Th1 releases GVL, set next running thread Th2, and wakeup Th2 (sched->running = Th2). Also sched->runnable_hot_th == Th1`).
2. If Th1 finished the blocking operation and check the running thread, but if the sched->running doesn't resumed yet (by checking runnable_hot_th == Th1), retake the GVL by Th1.

(in other words, on step 1, mark Th2 as stealable state until the Th2 really resumes)

and I think it is same essentially with my implementation. I'll check it on my benchmark and think about the difference.

@luke-gruber I just pushed a fix for the MN_THREAD issue. I confirmed the benchmark numbers are unchanged.

@ko1 my first stab at this was to do exactly what you described having tried. However I found it broke some assumptions/asserts and the scope of the change increased beyond my comfort level. I started from scratch with the approach here (which you have accurately summarized) and it seemed much simpler. I agree they should behave similarly so I'm curious what benchmark you saw a regression on.

@luke-gruber: Unfortunately I haven't gotten the PUMA benchmarks running yet. I'm having a variety of issues. It's likely my fault, as I'm also unable to run even the ruby benchmarks.

# make benchmark COMPARE_RUBY=../rel_orig/ruby
...
../benchmark/benchmark-driver/exe/benchmark-driver:133:in `slice!': invalid byte sequence in US-ASCII (ArgumentError)

What are the next steps to nudge this forward?

• Is there a way to submit a benchmarking job to a hermetic environment?
• Should I request a review from anyone else?
• Are we blocked on those PUMA benchmarks?

I would also like to see this move forward. The issue with benchmarking these types of changes is that normally we rely on ruby-bench, but it doesn't exercise threads at all. I'm looking to change that soon, but for now that's the way it is. I'll run this patch through one of Shopify's apps with shadow traffic and see what it gives us. It will take a few days to get reliable results from this and I'm not expecting a noticeable speedup but I'll post the conclusion of the results once I get them. As for other thread/scheduler benchmarks, maybe Koichi has some private ones that he uses. Either way, he would need to sign off on this change.

One issue with the PR as is is the fact that another Ractor could be waiting on the thread_sched_lock of the main Ractor, so the unlock/relock could actually lead to more "spurious" wakeups or that thread could have been spinning and win. Also, another thread on the same Ractor could come back from a blocking action and be blocked on thread_sched_lock as well, so the same applies there.

Is there a way to submit a benchmarking job to a hermetic environment?

Benchmarking jobs are run on commit, and are from ruby-bench.

Should I request a review from anyone else?

No, not necessary.

Are we blocked on those PUMA benchmarks?

No, I have some results I'll post in the redmine ticket.

I see the code you're referring to where one ractor can acquire the thread_sched_lock of another ractor, but I'm not sure I understand the logic as it is today. There are two acquisition paths:

• ractor_wait_receive->ractor_wait->rb_ractor_sched_wait->thread_sched_lock
• rb_ractor_sched_wakeup->thread_sched_lock

If the wakeup (sequence 2) happens in the middle of the wait (sequence 1), before the thread_sched_lock is acquired, would the wake get dropped?

With the possible caveat that I don't understand the current behavior, I'm not sure this PR would make it worse?

• The PUMA results you shared look positive, if unremarkable. In the neighborhood of ~5% perf win?
• @ko1 do you have thoughts on next steps?

Hey @ko1 do you have any idea how to move this forward?

We finally got around to testing this on the github rails app and saw >10% improvements on context-switching, request-latencies, and cpu efficiency.

Sorry for absent. I couldn't try it yet.

Here is my benchmark I was tried last year and could you try it on your machine? (please replace my HOME to appropriate directory)

TN = 10
LN = 1_000

wtype = :same_io
wtype = :mix_1_4
wtype = :mix_2_3
wtype = :mix_3_2
wtype = :mix_4_1
wtype = :same_calc

def io_task
  case $io_op
  when :read
    File.read($io_file)
  when :write
    File.write($io_file, '*')
  end
end

def fib n
  if n > 1
    fib(n-1) + fib(n-2)
  else
    1
  end
end

def calc_task
  fib(15)
end

def set_wtype calc_ratio # 0 to 5
  calc_types = (0...calc_ratio).to_a
  $task = -> tid do
    case tid % 5
    when *calc_types
      LN.times{ calc_task }
    else
      LN.times{ io_task }
    end
  end
end

def kick type
  case type
  when :thread
    ts = TN.times.map do |i|
      Thread.new(i){|ti|
        $task.call ti
      }
    end
    ts.each(&:join)
  when :serial
    TN.times{ $task.call it }
  else
    raise
  end
end

$:.unshift "/home/ko1/ruby/gems/benchmark/lib"
require 'benchmark'

Benchmark.bm(20){|x|
  ['/dev/null', '/tmp/null', '/home/ko1/null'].each do |io_file|
    [:write, :read].each do |io_op|
      $io_file, $io_op = io_file, io_op
      wtypes = (0..5)
      wtypes.each do |t|
        set_wtype t

        x.report("task:#{io_op},#{io_file}/c#{t}/serial"){ kick(:serial) }
        x.report("task:#{io_op},#{io_file}/c#{t}/thread"){ kick(:thread) }
      end
    end
  end
}

The results from my mac M4 are here.

There are 3 variations. The one that uses /dev/null seems the most interesting, as it's not dominated by IO. That one shows 25% reduction in real time and 75% reduction in system time.

@ko1, do these results alleviate your concerns?

What are the next steps to review and ultimately merge? I don't reviewers or labels or milestones assigned to this PR. I don't see anyone assigned to the ruby issue (3 months old now). It's hard to tell where this is going.

@jpl-coconut maybe add it to the next meeting agenda? https://bugs.ruby-lang.org/issues/21877

Thank you. Let me confirm that /tmp/ is on memfs?

Based on these results, we can make the following observations with respect to the blocking time (BT):

a. If BT is sufficiently short (e.g., read/write to /dev/null), the proposed modification performs well.
b. If BT is neither very short nor very long (e.g., read/write to /tmp/null), the current approach is preferable.
c. If BT is sufficiently long (e.g., read/write to a nullfile), there is no noticeable difference from the current approach.

This qualitative assessment seems reasonable. (The method is designed to optimize case a, so if switching would have been more beneficial, the disadvantage in b would manifest.)

In real-world scenarios, as long as case b is relatively rare, introducing the modification is advantageous due to the effect in a.

In my experience, when performing operations over a TCP/IP network, I did observe b to some extent (apologies for not having a fully reproducible benchmark to share).

Could you make some more survey? If a is enough bigger than b, we can merge it.

on your comment:

We finally got around to testing this on the github rails app and saw >10% improvements on context-switching, request-latencies, and cpu efficiency.

is it normal GitHub workload?

@jpl-coconut I can invite you to slack channel I'm in and you can ping me easier, if you want.

Thanks for the analysis @ko1!

Thanks also for your offer! I do have slack. You can also reach me by email, which is in my commit header.

github app

Yes, the >10% numbers are for the main github rails app. The reason it helps (I think) is that we have various background threads that compete for the GIL, and this reduces the frequency that hot threads get descheduled and drop their CPU cache. (Most of those threads come from standard middleware, so I don't think this should be specific to github.)

BT - A/B/C framework

The framing you provided seems pretty reasonable. To get a sense of ABC I wrote a microbenchmark just for the io_tasks.

At 2usec, we see an improvement. At 41usec per syscall, we see a regression, and then again at 61 usec we see an improvement. That's a little surprising.

However, an io_task is not just one syscall. The ruby write task is open, ioctl, write, close. I tried measuring each separately using strace but strace itself distorts the results beyond the point of being useful. Still, from this (sketchy) data it does seem B falls somewhere in the neighborhood of 40 usec.

wall clock vs. efficiency.

While looking closer at the above, I noticed something in the stats that I hadn't seen before. These are the 4 results that regress on this PR:

Original:
task:write,/tmp/null/c0/thread  0.103228   0.165090   0.268318 (  0.396482)
task:write,/tmp/null/c1/thread  0.153062   0.140472   0.293534 (  0.405410)
task:write,/tmp/null/c2/thread  0.213940   0.106830   0.320770 (  0.395409)
task:write,/tmp/null/c3/thread  0.240354   0.072061   0.312415 (  0.361468)

With fix:
task:write,/tmp/null/c0/thread  0.083031   0.167252   0.250283 (  0.573075)
task:write,/tmp/null/c1/thread  0.127716   0.138213   0.265929 (  0.536740)
task:write,/tmp/null/c2/thread  0.170768   0.106007   0.276775 (  0.476342)
task:write,/tmp/null/c3/thread  0.233716   0.066482   0.300198 (  0.503972)

Each is about 25% worse on wall-clock time. However, CPU-time is still improved (though not meaningfully so).

In our case, we care more about CPU efficiency than wall-clock time. Wall-clock is dominated by requests to backend services anyway, so the small scheduling contribution won't be noticeable. However any change CPU efficiency translates directly to rubies per core.

Still, I haven't seen this discrepancy in other benchmarks and can't explain it.

• An obvious guess is that the discrepancy is IO time, but that should be consistent across the two benchmarks.
• Another possibility is the filesystem is competing with ruby for CPU time, and we're seeing that interaction with the difference in ruby scheduling behavior.
• I can isolate cpuset for the benchmark process, but I haven't figured out how do to that for the filesystem process.
• One followup would be a variant of this benchmark that doesn't create quite so much filesystem load.

I spent a while debugging the gap between cpu-time and real-time and unfortunately didn't make much progress, then tried a different tack.

I implemented a native method that does nothing but spin for the given number of microseconds. It seems very useful for benchmarking stuff like this. I then substituted the fileio in your benchmark with calls to this spin function. The units in the benchmark (1, 5, 10, etc.) are in microseconds.

Here are the results: https://gist.github.com/jpl-coconut/e6d4a8792a4a84cf27cd0843efeef48e
We see a huge improvement when we spin (in a simulated syscall) for 1 usec, 5usec, 10usec, but then it evens out after 50usec. There are zero cases where the performance is worse than baseline.

This confirms the regression in wall-time (but not cpu-time) on those 4 reports is somehow related to tmpfs implementation. (It's probably quite CPU intensive to keep reopening these file handles, creating these kernel objects, tearing them down, reopening them, etc.) but most of that is happening on separate threads with unknown synchronization behaviors.

Using the a/b/c framing, this shows that the 'b' domain is actually empty.

I'm curious what you think is a good next step, @ko1?

I pushed a small update, adding a hint to bypass the sched lock drop/reacquire that concerned @luke-gru. The benchmarks I posted previously are all unchanged, but I did verify that most of them do fewer drop/acquire steps.

Thank you for more investigation.
It is summarized your data for user/real.

it seems no regression on 1, 5, 10, 50, 100, 500 usec (nanosleep).

BTW, on my machine (WSL on Windows), nanosleep() doesn't wakeup immediately in a given time. If my memory is correct, it takes +20usec for each nanosleep(). How about on your machine?

How about to try spin loop like loop{ break if current_usec() > deadline_usec} to check busy case?

I'm glad this is making progress! Sorry for my lack of involvement, but we had a hard time getting ruby 4 on our apps due to gem incompatibilities. I got pulled into other work, but this week I'm looking into running this with shadow traffic on one of our apps again. I will let you know the results when/if I get them, but don't wait on me please.

In my branch I added two methods, sleep_usec and spin_usec.

spin_usec is implemented exactly as you described, and those are the results I posted, since they're the interesting ones.

The sleep_usec results were identical across implementations, due to the nanosleep behavior that you called out. (It always deschedules the thread, and so the optimization in this PR does nothing.) I originally didn't bother posting the results, but they are here.

Any thoughts on next steps, @ko1?

We finally got some numbers we can trust from our environment. It's not the production environment, but it's similar. The numbers look really good 👍

Request time (mean)

P99: 4-5% improvement

P90: 4-5% improvement

P75: 3-4% improvement

P50: 4-5% improvement

Overall: 9-10% improvement

The overall improvement is higher due to the bigger absolute difference in ~P90-P99 (wider gap between the numbers).

We also use the gvltools gem, and the wait time from READY to RESUMED has improved across the board! 🎉

I will add to the dev meeting agenda as suggested by @p8

Are we making any progress?

We concluded to accept this PR.
Still I'm not sure the reason and impact of tmpfile regression, but we can revisit it if there was another report.

As on your comment of the dev-meeting:

How to better facilitate future contributions/discussions related to concurrency/perf, where tradeoffs might be more subtle.

is essential problem.

@ko1 We're seeing the contention in https://bugs.ruby-lang.org/issues/21685 as a critical regression in our Sidekiq deployments with 20 threads, causing Sidekiq worker processes to silently deregister from Redis under load after we upgraded to Ruby 3.3.9.

Would you consider backporting this to the 3.3.x and 3.4.x releases?

@stanhu A regression from what previous version of Ruby?

@luke-gruber Ruby 3.2.8. I think it relates to this bug in https://bugs.ruby-lang.org/issues/20816, as I think you mentioned in https://bugs.ruby-lang.org/issues/20816#note-4.