Shrink dict when deleting dictEntry #12850

lyq2333 · 2023-12-08T10:19:55Z

When we insert entries into dict, it may autonomously expand if needed. However, when we delete entries from dict, it doesn't shrink to the proper size. If there are few entries in a very large dict, it may cause huge waste of memory and inefficiency when iterating.

The main keyspace dicts (keys and expires), are shrinked by cron (tryResizeHashTables calls htNeedsResize and dictResize),
And some data structures such as zset and hash also do that (call htNeedsResize) right after a loop of calls to dictDelete,
But many other dicts are completely missing that call (they can only expand).

In this PR, we provide the ability to automatically shrink the dict when deleting. The conditions triggering the shrinking is the same as htNeedsResize used to have. i.e. we expand when we're over 100% utilization, and shrink when we're below 10% utilization.

Additionally:

Add dictPauseAutoResize so that flows that do mass deletions, will only trigger shrinkage at the end.
Rename dictResize to dictShrinkToFit (same logic as it used to have, but better name describing it)
Rename _dictExpand to _dictResize (same logic as it used to have, but better name describing it)

related to discussion #12819 (comment)

oranagra

generally, i'd like to proceed with this direction, but i have some concerns.

src/dict.c

src/t_set.c

src/dict.c

…sizeAllowed

oranagra

code LGTM, minor comment edit.

src/dict.c

Co-authored-by: Oran Agra <oran@redislabs.com>

src/t_zset.c

src/dict.c

src/t_zset.c

oranagra · 2024-01-02T16:30:38Z

this PR was discussed in a core-team meeting and we agreed we can proceed and merged it.

oranagra

@lyq2333 i edited the top comment, please check if it looks ok to you.

src/t_zset.c

lyq2333 · 2024-01-04T02:24:57Z

@lyq2333 i edited the top comment, please check if it looks ok to you.

@oranagra Thanks. Looks good to me.

src/dict.c

oranagra · 2024-01-08T08:29:12Z

@soloestoy waiting for your ack and response in one of the above threads.

src/dict.c

The new shrink was added in redis#12850. Also updated outdated comments, see redis#11692.

The new shrink was added in #12850. Also updated outdated comments, see #11692.

Before redis#12850, we will only try to shrink the dict in serverCron, which we can control by using a child process, but now every time we delete a key, the shrink check will be called. In these test (added in redis#12802), we meant to disable the resizing, but druing the delete, the dict will meet the force shrink, like 2 / 128 = 0.015 < 0.2, the delete will trigger a force resize and will cause the test to fail. In this commit, we try to keep the load factor at 3 / 128 = 0.023, that is, do not meet the force shrink.

Before #12850, we will only try to shrink the dict in serverCron, which we can control by using a child process, but now every time we delete a key, the shrink check will be called. In these test (added in #12802), we meant to disable the resizing, but druing the delete, the dict will meet the force shrink, like 2 / 128 = 0.015 < 0.2, the delete will trigger a force resize and will cause the test to fail. In this commit, we try to keep the load factor at 3 / 128 = 0.023, that is, do not meet the force shrink.

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_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)

) The function `tryResizeHashTables` only attempts to shrink the dicts that has keys (change from #11695), this was a serious problem until the change in #12850 since it meant if all keys are deleted, we won't shrink the dick. But still, both dictShrink and dictExpand may be blocked by a fork child process, therefore, the cron job needs to perform both dictShrink and dictExpand, for not just non-empty dicts, but all dicts in DBs. What this PR does: 1. Try to resize all dicts in DBs (not just non-empty ones, as it was since #12850) 2. handle both shrink and expand (not just shrink, as it was since forever) 3. Refactor some APIs about dict resizing (get rid of `htNeedsShrink` `htNeedsShrink` `dictShrinkToFit`, and expose `dictShrinkIfNeeded` `dictExpandIfNeeded` which already contains all the code of those functions we get rid of, to make APIs more neat) 4. In the `Don't rehash if redis has child process` test, now that cron would do resizing, we no longer need to write to DB after the child process got killed, and can wait for the cron to expand the hash table.

When we insert entries into dict, it may autonomously expand if needed. However, when we delete entries from dict, it doesn't shrink to the proper size. If there are few entries in a very large dict, it may cause huge waste of memory and inefficiency when iterating. The main keyspace dicts (keys and expires), are shrinked by cron (`tryResizeHashTables` calls `htNeedsResize` and `dictResize`), And some data structures such as zset and hash also do that (call `htNeedsResize`) right after a loop of calls to `dictDelete`, But many other dicts are completely missing that call (they can only expand). In this PR, we provide the ability to automatically shrink the dict when deleting. The conditions triggering the shrinking is the same as `htNeedsResize` used to have. i.e. we expand when we're over 100% utilization, and shrink when we're below 10% utilization. Additionally: * Add `dictPauseAutoResize` so that flows that do mass deletions, will only trigger shrinkage at the end. * Rename `dictResize` to `dictShrinkToFit` (same logic as it used to have, but better name describing it) * Rename `_dictExpand` to `_dictResize` (same logic as it used to have, but better name describing it) related to discussion redis#12819 (comment) --------- Co-authored-by: Oran Agra <oran@redislabs.com> Co-authored-by: zhaozhao.zz <zhaozhao.zz@alibaba-inc.com>

The new shrink was added in redis#12850. Also updated outdated comments, see redis#11692.

Before redis#12850, we will only try to shrink the dict in serverCron, which we can control by using a child process, but now every time we delete a key, the shrink check will be called. In these test (added in redis#12802), we meant to disable the resizing, but druing the delete, the dict will meet the force shrink, like 2 / 128 = 0.015 < 0.2, the delete will trigger a force resize and will cause the test to fail. In this commit, we try to keep the load factor at 3 / 128 = 0.023, that is, do not meet the force shrink.

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)

…is#12819) The function `tryResizeHashTables` only attempts to shrink the dicts that has keys (change from redis#11695), this was a serious problem until the change in redis#12850 since it meant if all keys are deleted, we won't shrink the dick. But still, both dictShrink and dictExpand may be blocked by a fork child process, therefore, the cron job needs to perform both dictShrink and dictExpand, for not just non-empty dicts, but all dicts in DBs. What this PR does: 1. Try to resize all dicts in DBs (not just non-empty ones, as it was since redis#12850) 2. handle both shrink and expand (not just shrink, as it was since forever) 3. Refactor some APIs about dict resizing (get rid of `htNeedsShrink` `htNeedsShrink` `dictShrinkToFit`, and expose `dictShrinkIfNeeded` `dictExpandIfNeeded` which already contains all the code of those functions we get rid of, to make APIs more neat) 4. In the `Don't rehash if redis has child process` test, now that cron would do resizing, we no longer need to write to DB after the child process got killed, and can wait for the cron to expand the hash table.

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>

dict shrink when entry deletes

9fd2b9b

soloestoy requested review from zuiderkwast, oranagra, soloestoy and hpatro December 8, 2023 11:14

oranagra reviewed Dec 9, 2023

View reviewed changes

src/dict.c Outdated Show resolved Hide resolved

src/t_set.c Show resolved Hide resolved

oranagra mentioned this pull request Dec 10, 2023

Optimize resizing hash table to resize not only non-empty dicts. #12819

Merged

correct the meaning of dict_force_resize_ratio

dbc68d2

soloestoy reviewed Dec 11, 2023

View reviewed changes

src/dict.c Outdated Show resolved Hide resolved

rename dictExpandAllowed to dictTypeReHashAllowed

580c284

oranagra reviewed Dec 11, 2023

View reviewed changes

src/dict.c Outdated Show resolved Hide resolved

src/dict.c Outdated Show resolved Hide resolved

src/dict.c Outdated Show resolved Hide resolved

add explanation for _dictShrinkIfNeeded && change rehashAllowed to re…

13ed6c8

…sizeAllowed

oranagra approved these changes Dec 11, 2023

View reviewed changes

src/dict.c Outdated Show resolved Hide resolved

hpatro reviewed Dec 11, 2023

View reviewed changes

src/dict.c Outdated Show resolved Hide resolved

src/dict.c Outdated Show resolved Hide resolved

Update src/dict.c

843c97f

Co-authored-by: Oran Agra <oran@redislabs.com>

zuiderkwast reviewed Dec 12, 2023

View reviewed changes

src/t_zset.c Show resolved Hide resolved

lyq2333 added 3 commits December 15, 2023 16:54

decouple dictShrink from dictExpand && add disallowResize for dict

1d9ae42

merge unstable

bc6eab3

rename disallowResize to pauseAutoResize

9768b37

oranagra approved these changes Dec 15, 2023

View reviewed changes

src/dict.c Show resolved Hide resolved

oranagra reviewed Dec 15, 2023

View reviewed changes

src/t_zset.c Show resolved Hide resolved

rename dictResize to dictShrinkToFit

9c77368

oranagra reviewed Jan 3, 2024

View reviewed changes

src/t_zset.c Outdated Show resolved Hide resolved

pause dictShrink in zdiffstore

c87257d

soloestoy reviewed Jan 4, 2024

View reviewed changes

src/dict.c Show resolved Hide resolved

src/dict.c Show resolved Hide resolved

src/dict.c Outdated Show resolved Hide resolved

lyq2333 added 2 commits January 4, 2024 16:39

ignore the return of _dictShrinkIfNeeded and _dictExpandIfNeeded

d154389

fix make err

e9261ee

oranagra approved these changes Jan 4, 2024

View reviewed changes

remove return value of _dictShrinkIfNeeded and _dictExpandIfNeeded

db67012

oranagra approved these changes Jan 4, 2024

View reviewed changes

CharlesChen888 approved these changes Jan 8, 2024

View reviewed changes

merge unstable

e9ba745

soloestoy reviewed Jan 12, 2024

View reviewed changes

src/dict.c Show resolved Hide resolved

src/dict.c Show resolved Hide resolved

src/dict.c Show resolved Hide resolved

src/dict.c Show resolved Hide resolved

oranagra merged commit e2b7932 into redis:unstable Jan 15, 2024
13 checks passed

enjoy-binbin added a commit to enjoy-binbin/redis that referenced this pull request Jan 15, 2024

Updated comments on dictResizeEnable for new dict shrink

6aae808

The new shrink was added in redis#12850. Also updated outdated comments, see redis#11692.

enjoy-binbin mentioned this pull request Jan 15, 2024

Updated comments on dictResizeEnable for new dict shrink #12946

Merged

oranagra pushed a commit that referenced this pull request Jan 15, 2024

Updated comments on dictResizeEnable for new dict shrink (#12946)

ecc31bc

The new shrink was added in #12850. Also updated outdated comments, see #11692.

lyq2333 mentioned this pull request Jan 15, 2024

Change the threshold of dict expand, shrink and rehash #12948

Merged

enjoy-binbin mentioned this pull request Jan 18, 2024

Fix unexpected resize causing test failure #12960

Merged

sundb mentioned this pull request Jan 30, 2024

Fix the failure of defrag test under 32-bit #13013

Merged

roggervalf pushed a commit to roggervalf/redis that referenced this pull request Feb 11, 2024

Updated comments on dictResizeEnable for new dict shrink (redis#12946)

b6b0884

The new shrink was added in redis#12850. Also updated outdated comments, see redis#11692.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shrink dict when deleting dictEntry #12850

Shrink dict when deleting dictEntry #12850

lyq2333 commented Dec 8, 2023 •

edited by oranagra

oranagra left a comment

oranagra left a comment

oranagra commented Jan 2, 2024

oranagra left a comment

lyq2333 commented Jan 4, 2024 •

edited

oranagra commented Jan 8, 2024

Shrink dict when deleting dictEntry #12850

Shrink dict when deleting dictEntry #12850

Conversation

lyq2333 commented Dec 8, 2023 • edited by oranagra

oranagra left a comment

Choose a reason for hiding this comment

oranagra left a comment

Choose a reason for hiding this comment

oranagra commented Jan 2, 2024

oranagra left a comment

Choose a reason for hiding this comment

lyq2333 commented Jan 4, 2024 • edited

oranagra commented Jan 8, 2024

lyq2333 commented Dec 8, 2023 •

edited by oranagra

lyq2333 commented Jan 4, 2024 •

edited