Skip to content

Attempt acquiring semaphore before releasing GVL #573

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 3, 2025
Merged

Conversation

jhawthorn
Copy link
Member

Particularly when there are multiple threads waiting to run, there is a cost to releasing the GVL that outweighs the benefit for short system calls.

This commit changes acquire_semaphore_without_gvl to attempt to acquire the semaphore while also including IPC_NOWAIT in flags, which should return immediately if the request isn't available.

Discovered from looking at profiles with @rwstauner and @tenderworks

jhawthorn and others added 2 commits March 28, 2025 11:58
Particularly when there are multiple threads waiting to run, there is a cost to releasing the GVL that outweighs the
benefit for short system calls.

This commit changes acquire_semaphore_without_gvl to attempt to acquire the semaphore while also including IPC_NOWAIT in
flags, which should return immediately if the request isn't available.
@tenderlove
Copy link

LGTM

@tenderworks
Copy link
Contributor

Just for extra context: we believe that semaphores are not contended most of the time. Which means that acquiring the lock on the semaphore is very fast. Fast enough that even if the GVL is released, no other work would actually be able to be scheduled. Since it takes time to unlock the GVL, but no other work can be scheduled, we're actually just slowing down the semaphore lock.

@jhawthorn's change does a non-blocking acquire without releasing the GVL. If the lock fails, then we know the lock is contended and should take more time to lock. So in that case we'll acquire the GVL and try acquiring the lock again.

@tenderworks tenderworks merged commit 15ea01c into main Apr 3, 2025
42 checks passed
@tenderworks tenderworks deleted the try_acquire branch April 3, 2025 15:49
@jhawthorn
Copy link
Member Author

Here's a benchmark for posterity:

benchmark
require "semian"
require "benchmark"
require "benchmark/ips"

def fib n
  (n <= 1) ? n : (fib(n-1) + fib(n-2))
end

raise "unsupported" unless Semian.sysv_semaphores_supported?

Semian.register(
  :testing,
  bulkhead: true,
  tickets: 5,
  timeout: 1,
  circuit_breaker: false
)

@resource = Semian[:testing]

Benchmark.ips do |x|
  x.report "1 thread" do
    10_000.times do
      @resource.acquire do
      end
    end
  end

  x.report "10 threads" do
    10.times.map do
      Thread.new do
        1000.times do
          @resource.acquire do
          end
        end
      end
    end.each(&:join)
  end
end

Before

ruby 3.4.2 (2025-02-15 revision d2930f8e7a) +PRISM [x86_64-linux]
Warming up --------------------------------------
            1 thread     2.000 i/100ms
          10 threads     1.000 i/100ms
Calculating -------------------------------------
            1 thread     29.772 (± 0.0%) i/s -    150.000 in   5.038469s
          10 threads      7.149 (± 0.0%) i/s -     36.000 in   5.038535s

After

ruby 3.4.2 (2025-02-15 revision d2930f8e7a) +PRISM [x86_64-linux]
Warming up --------------------------------------
            1 thread     3.000 i/100ms
          10 threads     2.000 i/100ms
Calculating -------------------------------------
            1 thread     30.948 (± 0.0%) i/s -    156.000 in   5.040870s
          10 threads     29.519 (± 0.0%) i/s -    148.000 in   5.014481s

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants