-
Notifications
You must be signed in to change notification settings - Fork 595
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
Take hz into account in activerehashing to avoid CPU spikes #977
base: unstable
Are you sure you want to change the base?
Conversation
Currently in conf we describe activerehashing as: Active rehashing uses 1 millisecond every 100 milliseconds of CPU time. This is the case for hz = 10. If we change hz, the description in conf will be inaccurate. Users may notice that the server spends some CPU (used in activerehashing) at high hz but don't know why, since our cron calls are fixed to 1ms. This PR takes hz into account and fixed the CPU usage a 1% (this may not be accurate in some cases because we do 100 step rehashing in dictRehashMicroseconds but it can avoid CPU spikes in this case). Signed-off-by: Binbin <binloveplay1314@qq.com>
Another approach is that we only update conf to mention the effect of hz. In addition, i will mention these things we've discussed in the old times:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## unstable #977 +/- ##
============================================
- Coverage 70.54% 70.39% -0.16%
============================================
Files 114 114
Lines 61644 61646 +2
============================================
- Hits 43489 43395 -94
- Misses 18155 18251 +96
|
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.
Nice! I like it.
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.
My only remaining comment before we merge this is about the documentation.
I have a suggestion for rewritten documentation for activerehashing
in the conf file:
# Active rehashing uses 1% of the CPU time to help perform incremental rehashing
# of the main server hash tables, the ones mapping top-level keys to values.
#
# If active rehashing is disabled and rehashing is needed, a hash table is
# rehashed one "step" on every operation performed on the hash table (add, find,
# etc.), so if the server is idle, the rehashing may never complete and some
# more memory is used by the hash tables. Active rehashing helps prevent this.
#
# Active rehashing runs as a background task. Depending on the value of 'hz',
# the frequency at which the server performs background tasks, active rehashing
# can cause the server to freeze for a short time. For example, if 'hz' is set
# to 10, active rehashing runs for up to one millisecond every 100 milliseconds.
# If a freeze of one millisecond is not acceptable, you can increase 'hz' to let
# active rehashing run more often. If instead 'hz' is set to 100, active
# rehashing runs up to only 100 microseconds every 10 milliseconds. The total is
# still 1% of the time.
activerehashing yes
What do you think?
If we set the active rehashing to hard-coded 1% of CPU time now, then in the future we can add I like the idea of |
The doc is great, i will take it, thanks.
there are two items, the
|
Signed-off-by: Binbin <binloveplay1314@qq.com>
Signed-off-by: Binbin <binloveplay1314@qq.com>
Signed-off-by: Binbin <binloveplay1314@qq.com>
Signed-off-by: Binbin <binloveplay1314@qq.com>
I only took a quick look at the PR. It makes sense to me. |
I'm not sure, is To me, Effort per CPU usage is a better, because the rehashing needed is relative to the traffic, which is relative to the CPU usage. Before we add the config, I think we need to investigate if we can find out some way to choose the best behaviour for the users, maybe automatic. Some possibilities are:
The
|
|
With this caluclation, we will use This is good. I like it. But the name
But we don't need to add the config now. With this PR, it is probably enough. |
yean, active-rehash-percent seems better, sorry for the confusing (bad English aha). yes, we don't need it now i guess. It is a option in case this one got merged (fixed 1%), and we are not able to adjust the speed and someone think rehashing is too slow and eating the memory. |
Merge today? Or major decision? Not bad english, just two different cycle: "cron cycle" vs "CPU cycle". |
Currently in conf we describe activerehashing as: Active rehashing
uses 1 millisecond every 100 milliseconds of CPU time. This is the
case for hz = 10.
If we change hz, the description in conf will be inaccurate. Users
may notice that the server spends some CPU (used in activerehashing)
at high hz but don't know why, since our cron calls are fixed to 1ms.
This PR takes hz into account and fixed the CPU usage at 1% (this may
not be accurate in some cases because we do 100 step rehashing in
dictRehashMicroseconds but it can avoid CPU spikes in this case).