Skip to content
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

Rewrite PessimisticLockingTest#test_no_locks_no_wait to avoid costly sleeping #47503

Merged
merged 1 commit into from Jul 19, 2023

Conversation

fatkodima
Copy link
Member

Follow up to #47499.

PessimisticLockingTest#test_no_locks_no_wait test uses costly sleeping for no reason and relies on the correct order of threads activating via sleeping. It wastes 5 seconds just for this test case.

Made a profile of this change.

Before

     TOTAL    (pct)     SAMPLES    (pct)     FRAME
     45395   (8.3%)       45395   (8.3%)     PG::Connection#exec_params
===> 34741   (6.4%)       34741   (6.4%)     Thread#join
     20520   (3.8%)       20520   (3.8%)     Thread::Mutex#sleep
     10580   (1.9%)       10580   (1.9%)     (marking)
      8755   (1.6%)        8755   (1.6%)     (sweeping)
      7380   (1.4%)        7378   (1.4%)     PG::Connection#exec_prepared
      6252   (1.1%)        6252   (1.1%)     IO#wait
      5662   (1.0%)        5662   (1.0%)     Kernel#sleep
      3989   (0.7%)        3989   (0.7%)     Process.waitpid
    164585  (30.2%)        3066   (0.6%)     Thread.handle_interrupt
      3036   (0.6%)        3035   (0.6%)     String#sub!

After

     TOTAL    (pct)     SAMPLES    (pct)     FRAME
     49190   (9.2%)       49185   (9.2%)     PG::Connection#exec_params
     20507   (3.8%)       20507   (3.8%)     Thread::Mutex#sleep
     10680   (2.0%)       10680   (2.0%)     (marking)
===> 10007   (1.9%)       10007   (1.9%)     Thread#join
      8952   (1.7%)        8952   (1.7%)     (sweeping)
      7134   (1.3%)        7134   (1.3%)     PG::Connection#exec_prepared
      6785   (1.3%)        6785   (1.3%)     IO#wait
      5665   (1.1%)        5665   (1.1%)     Kernel#sleep
      3992   (0.7%)        3992   (0.7%)     Process.waitpid
      3153   (0.6%)        3153   (0.6%)     String#sub!

end
t1 = Time.now
end

b = Thread.new do
sleep zzz / 2.0 # ensure thread 1 tx starts first
b_wakeup.pop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, nice. I wasn't familiar with Thread::Queue#pop

If the queue is empty, the calling thread is suspended until data is pushed onto the queue.
https://rubyapi.org/3.2/o/thread/queue#method-i-pop

So this waits until Thread a pushes true into b_wakeup.

@matthewd
Copy link
Member

matthewd commented Mar 5, 2023

Nice find!

We prefer Concurrent::Event, Concurrent::CyclicBarrier, and friends where appropriate, just for the extra clarity that we're not actually passing values through queues.

@fatkodima
Copy link
Member Author

@matthewd Thank you for the suggestion! I rewrote it to use Concurrent::Event.

@matthewd matthewd merged commit 3a8b9ba into rails:main Jul 19, 2023
@fatkodima fatkodima deleted the no-sleeping-on-duel branch July 19, 2023 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants