-
-
Notifications
You must be signed in to change notification settings - Fork 279
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
Reduce reaper threads #576
Conversation
Hi @mhenrixon I've tested this PR in production:
I've used this config to see if threads will be leaking. config.reaper_interval = 10 # Reap every X seconds
config.reaper_timeout = 5 # Give the reaper X seconds to finish I see in the sidekiq output:
Good news: Reaper is started every 10s and it removes digests. Previously when I used: config.reaper_interval = 600 # Reap every X seconds
config.reaper_timeout = 595 then the reaper timeouted, sometimes it took a few minutes to delete digests. Did you improve something about how fast reaper is? Maybe it accidental behavior because after a while I see longer than 10s intervals in logs:
Probably for safety reasons I will keep using 10 minutes intervals for reaper instead of 10s. BTW Regarding the root problem with leaking threads. For the previous config, I had 12-13 threads on average. |
I believe a number of threads is more or less stable after an hour of running your PR fix in production. I noticed also lower RAM usage and higher GC frequency. I guess that's good because I was about to start working on the issue with too much RAM consumption but it seems like your PR also helps with that. I even tried to run on purpose some heavy workers to see if the RAM usage will increase but it seems to be stable. EDITED: I guess more GC frequency is because I run more workers. I'll keep the PR fix for the night to see what's tomorrow morning. |
@ArturT while I am generally pleased with things being slightly better, the increased GC worries me a little. I will dig in and see if I can spot something. In general, concurrent ruby violates some performance problems. For example, using That said, in this PR I also nullify the executor and create a new one, that could be the reason for more GC. I'll do some more digging first. Glad I'm on the right path though |
Please note I'm note sure what's root reason of more GC frequency. The previous week I used 10 minutes reaper interval. Now while I was testing this PR I used 10 seconds reaper interval. It could be just smaller interval a main reason of more GC frequency. We don't compare apple to apple. My thinking is if this PR will be merged then I will stick with 10 minutes reaper interval just in case. It seems like I don't need more reaper calls. I did not see in output removed 1000 keys. It was always below 1000 when reaper 10 minutes interval. So I guess 10 seconds reaper interval would not add value. Am I missing something? |
10 minutes was the intended interval, I typed it up wrong in the readme. Ok, I will fix the documentation, the examples and release a new version today. |
Alright @ArturT, released as |
config.reaper = :ruby # :ruby, :lua or :none/nil | ||
config.reaper_count = 1000 # Stop reaping after this many keys | ||
config.reaper_interval = 600 # Reap orphans every 10 minutes | ||
config.reaper_timeout = 150 # Timeout reaper after 1,5 minutes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
150/60 = 2.5 minutes
I keep in my own config 595 seconds as timeout. It's close to interval 600s.
|
||
def schedule_next_task(interval = execution_interval) | ||
exec_task = ->(completion) { execute_task(completion) } | ||
ScheduledTask.execute(interval, args: [Concurrent::Event.new], &exec_task) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should there be prefix Concurrent::ScheduledTask
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added fix in this PR #577
Thanks @mhenrixon I noticed a random problem on sidekiq start. When I start sidekiq in development with
I guess the problem is missing EDITED: I added fix in PR #577 |
Unfortunately, due to a bug in concurrent-ruby, the existing implementation of the TimerTask was bleeding threads and there for memory. This PR attempts to fix that by limiting the number of threads available using
RubySingleThreadExecutor
Should address the feedback from @ArturT in #571