Change the threshold of dict expand, shrink and rehash #12948
Conversation
|
that's actually a bug in #12276, (downsize rehashing at 20% instead of 2%), right? @soloestoy @lyq2333 isn't it better to have dictRehash use the same conditions as in _dictExpandIfNeeded and _dictSrinkIfNeeded? i.e. if we decided to expand or shrink in a specific scenario, we should also do the rehashing (and eventually release the extra LUT) |
yes, that is right. i also feel the same conditions need to be used. That is to avoid introducing some other conditions. |
We can't, |
I agree with @soloestoy. Because we trigger a shrinking at 2% or 10%, no shrinking will be stopped by this condition which is 20%. I think it's better that this condition could limit some shrinking just like expanding. |
|
@soloestoy |
src/dict.c
Outdated
| /* If dict_can_resize is DICT_RESIZE_AVOID, we want to avoid rehashing. | ||
| * We can restrict the condition because both s0 and s1 are powers of 2. | ||
| * - If expanding, the _dictNextExp of dict_force_resize_ratio is 8. | ||
| * - If shrinking, the _dictNextExp of (HASHTABLE_MIN_FILL / dict_force_resize_ratio) is 1/32. */ |
There was a problem hiding this comment.
I've just been trying to understand why it's 32, not 64, and I thinks that others will have that understanding cost if we don't make both sides consistent.
|
|
you're right.
i agree. but still i don't understand why we need different thresholds, and not re-use the same ones. |
Yes, the reason is as follows: for expand, the next power of 2 after 5 is 8, and for shrink, 2% of the previous power of 2 is 1/32. |
|
so let's change then avoid introducing the 2 new thresholds this PR currently adds, and the formulas and comments will make much more sense? |
|
How about change |
|
isn't that what i said..? |
I thought you wanna |
|
@oranagra @soloestoy I have changed @enjoy-binbin Because of the change of |
|
@lyq2333 thanks. please update the title and top comment to match the new code and make sure they explain the problem and solution. |
|
@lyq2333 we have a build failure in the 32bit builds: https://github.com/redis/redis/actions/runs/7584286940/job/20657641562 |
lyq2333
changed the title
|
@lyq2333 i edited the top comment. please ack or edit |
Before this change (most recently modified in redis#12850 (comment)), The trigger for normal expand threshold was 100% utilization and the trigger for normal shrink threshold was 10% (HASHTABLE_MIN_FILL). While during fork (DICT_RESIZE_AVOID), when we want to avoid rehash, the trigger thresholds were multiplied by 5 (`dict_force_resize_ratio`), meaning 500% for expand and 2% (100/10/5) for shrink. However, in `dictRehash` (the incremental rehashing), the rehashing threshold for shrinking during fork (DICT_RESIZE_AVOID) was 20% by mistake. This meant that if a shrinking is triggered when `dict_can_resize` is `DICT_RESIZE_ENABLE` which the threshold is 10%, the rehashing can continue when `dict_can_resize` is `DICT_RESIZE_AVOID`. This would cause unwanted CopyOnWrite damage. It'll make sense to change the thresholds of the rehash trigger and the thresholds of the incremental rehashing the same, however, in one we compare the size of the hash table to the number of records, and in the other we compare the size of ht[0] to the size of ht[1], so the formula is not exactly the same. to make things easier we change all the thresholds to powers of 2, so the normal shrinking threshold is changed from 100/10 (i.e. 10%) to 100/8 (i.e. 12.5%), and we change the threshold during forks from 5 to 4, i.e. from 500% to 400% for expand, and from 2% (100/10/5) to 3.125% (100/8/4)
Fail CI: https://github.com/redis/redis/actions/runs/7837608438/job/21387609715 ## Why defragment tests only failed under 32-bit First of all, under 32-bit jemalloc will allocate more small bins and less large bins, which will also lead to more external fragmentation, therefore, the fragmentation ratio is higher in 32-bit than in 64-bit, so the defragment tests(`Active defrag eval scripts: cluster` and `Active defrag big keys: cluster`) always fails in 32-bit. ## Why defragment tests only failed with cluster The fowllowing is the result of `Active defrag eval scripts: cluster` test. 1) Before #11695, the fragmentation ratio is 3.11%. 2) After #11695, the fragmentation ratio grew to 4.58%. Since we are using per-slot dictionary to manage slots, we will only defragment the contents of these dictionaries (keys, values), but not the dictionaries' struct and ht_table, which means that frequent shrinking and expanding of the dictionaries, will make more fragments. 3) After #12850 and #12948, In cluster mode, a large number of cluster slot dicts will be shrunk, creating additional fragmention, and the dictionary will not be defragged. ## Solution * Add defragmentation of the per-slot dictionary's own structures, dict struct and ht_table. ## Other change * Increase floating point print precision of `frags` and `rss` in debug logs for defrag --------- Co-authored-by: Oran Agra <oran@redislabs.com>
Before this change (most recently modified in #12850 (comment)), The trigger for normal expand threshold was 100% utilization and the trigger for normal shrink threshold was 10% (HASHTABLE_MIN_FILL).
While during fork (DICT_RESIZE_AVOID), when we want to avoid rehash, the trigger thresholds were multiplied by 5 (
dict_force_resize_ratio), meaning 500% for expand and 2% (100/10/5) for shrink.However, in
dictRehash(the incremental rehashing), the rehashing threshold for shrinking during fork (DICT_RESIZE_AVOID) was 20% by mistake.This meant that if a shrinking is triggered when
dict_can_resizeisDICT_RESIZE_ENABLEwhich the threshold is 10%, the rehashing can continue whendict_can_resizeisDICT_RESIZE_AVOID.This would cause unwanted CopyOnWrite damage.
It'll make sense to change the thresholds of the rehash trigger and the thresholds of the incremental rehashing the same, however, in one we compare the size of the hash table to the number of records, and in the other we compare the size of ht[0] to the size of ht[1], so the formula is not exactly the same.
to make things easier we change all the thresholds to powers of 2, so the normal shrinking threshold is changed from 100/10 (i.e. 10%) to 100/8 (i.e. 12.5%), and we change the threshold during forks from 5 to 4, i.e. from 500% to 400% for expand, and from 2% (100/10/5) to 3.125% (100/8/4)