Implement defragmentation for pubsub kvstore #13058

sundb · 2024-02-18T02:44:02Z

This PR make effort to defrag the pubsub kvstore in the following ways:

Till now server.pubsub(shard)_channels only share channel name obj with the first subscribed client, now change it so that the clients and the pubsub kvstore share the channel name robj.
This would save a lot of memory when there are many subscribers to the same channel.
It also means that we only need to defrag the channel name robj in the pubsub kvstore, and then update
all client references for the current channel, avoiding the need to iterate through all the clients to do the same things.
Refactor the code to defragment pubsub(shard) in the same way as defragment of keys and EXPIRES, with the exception that we only defragment pubsub(without shard) when slot is zero.

Other

Fix an overlook in #11695, if defragment doesn't reach the end time, we should wait for the current
db's keys and expires, pubsub and pubsubshard to finish before leaving, now it's possible to exit
early when the keys are defragmented.

src/defrag.c

tests/unit/memefficiency.tcl

src/defrag.c

src/pubsub.c

src/defrag.c

…annel name

the reason for returning NULL is that we don't need to update the value of clients dictionary when `dictDefragTables(clients)` is NULL.

oranagra

i wonder if we now need a better test? (one that grantees we really did re-locate something in pubsub) or do you think the one we have is sufficient?

src/defrag.c

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

tests/unit/memefficiency.tcl

without timeout Fix an overlook in redis#11695, if defragment doesn't reach the end time, we should wait for the current db's keys and expires to finish before leaving, now it's possible to exit early when the keys are defragmented.

src/defrag.c

Before this PR we are always interating from stage 0 to 3, but now we're starting from the last stage, so when defrag_stage is not equal i, it means that the last stage isn't finished, so there is no point in iterating further.

oranagra

LGTM

sundb · 2024-03-02T11:43:03Z

@oranagra recent changes from last review:

always process defrag stages from slot 0, since we need to process pubsub with slot 0 and db 0.
when all defrag stages are finished, look for the next cloest slot from all stages, instead of looking from keys as before.
optimize defrag pubsub test, using the same client to intermittently populate the pubsub and pubsubshard.

src/defrag.c

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

oranagra

are you happy with the new approach? (scanning all the slots in each step before moving to the next one)?
[edit] only now i see your comment above. i'll read and respond.

src/defrag.c

sundb · 2024-03-04T13:38:03Z

@oranagra It's ready.

… reset (#13315) this PR fixes two crashes: 1. Fix missing slotToKeyInit() when using `flushdb async` under cluster mode. #13205 2. Fix missing expires_cursor reset when stopping active defrag in the middle of defragment. #13307 If we stop active defrag in the middle of defragging db->expires, if `expires_cursor` is not reset to 0, the next time we enable active defrag again, defragLaterStep(db, ...) will be entered. However, at this time, `db` has been reset to NULL, which results in crash. The affected code were removed by #11695 and #13058 in usntable, so we just need backport this to 7.2.

Fix #13612 This bug was introduced by #11465 This PR added the incremental defragmentation for db->expires. If the time for the defragmentation cycle has not arrived, it will exit unless both cursor and expires_cursor are 0, meaning both keys and expires have been fragmented. However, the expires_cursor check has now been overlooked, which leads to we will exit immediately even if the defragmentation doesn't reach the end time, which will cause the efficiency of defragmentation to become very low. Note that this bug was already fixed in version 7.4(#13058)

This PR fixes two defrag issues. 1. Fix a use-after-free issue caused by updating dictionary keys after a pubsub channel is reallocated. This issue was introduced by #13058 1. Fix potential use-after-free for lua during AOF loading with defrag This issue was introduced by #13058 This fix follows #14319 This PR updates the LuaScript LRU list before script execution to prevent accessing a potentially invalidated pointer after long-running scripts.

After redis#13013 ### This PR make effort to defrag the pubsub kvstore in the following ways: 1. Till now server.pubsub(shard)_channels only share channel name obj with the first subscribed client, now change it so that the clients and the pubsub kvstore share the channel name robj. This would save a lot of memory when there are many subscribers to the same channel. It also means that we only need to defrag the channel name robj in the pubsub kvstore, and then update all client references for the current channel, avoiding the need to iterate through all the clients to do the same things. 2. Refactor the code to defragment pubsub(shard) in the same way as defragment of keys and EXPIRES, with the exception that we only defragment pubsub(without shard) when slot is zero. ### Other Fix an overlook in redis#11695, if defragment doesn't reach the end time, we should wait for the current db's keys and expires, pubsub and pubsubshard to finish before leaving, now it's possible to exit early when the keys are defragmented. --------- Co-authored-by: oranagra <oran@redislabs.com>

This PR fixes two defrag issues. 1. Fix a use-after-free issue caused by updating dictionary keys after a pubsub channel is reallocated. This issue was introduced by redis#13058 1. Fix potential use-after-free for lua during AOF loading with defrag This issue was introduced by redis#13058 This fix follows redis#14319 This PR updates the LuaScript LRU list before script execution to prevent accessing a potentially invalidated pointer after long-running scripts.

This PR fixes two defrag issues. 1. Fix a use-after-free issue caused by updating dictionary keys after a pubsub channel is reallocated. This issue was introduced by redis#13058 2. Fix potential use-after-free for lua during AOF loading with defrag This issue was introduced by redis#13058 This fix follows redis#14319 This PR updates the LuaScript LRU list before script execution to prevent accessing a potentially invalidated pointer after long-running scripts.

Implement defragmentation for pubsub kvstore

75ec9ed

sundb requested a review from oranagra February 18, 2024 02:44

oranagra reviewed Feb 18, 2024

View reviewed changes

src/defrag.c Outdated Show resolved Hide resolved

src/defrag.c Outdated Show resolved Hide resolved

tests/unit/memefficiency.tcl Outdated Show resolved Hide resolved

tests/unit/memefficiency.tcl Outdated Show resolved Hide resolved

shahsb approved these changes Feb 18, 2024

View reviewed changes

sundb added 2 commits February 20, 2024 16:32

imcrement defrag pubsub

7cc43d6

Optimize test for pubsub defragmentation and add some comments

501160c

oranagra reviewed Feb 21, 2024

View reviewed changes

src/defrag.c Outdated Show resolved Hide resolved

src/defrag.c Show resolved Hide resolved

src/pubsub.c Outdated Show resolved Hide resolved

src/pubsub.c Outdated Show resolved Hide resolved

src/defrag.c Outdated Show resolved Hide resolved

sundb added 7 commits February 23, 2024 17:13

Make server's pubsub(shard) and clients' pubsub(shard) sharing the ch…

d8f6755

…annel name

Revert some code

9f8b1b1

Format code

36fb006

Use if else instead of switch

56de49e

dictDefragTables return NULL when the allocation wan't moved

9cde127

the reason for returning NULL is that we don't need to update the value of clients dictionary when `dictDefragTables(clients)` is NULL.

Add comment for activeDefragStringObEx

91cd3ae

Update comment for dictDefragTables()

ce36e9f

oranagra reviewed Feb 25, 2024

View reviewed changes

src/defrag.c Outdated Show resolved Hide resolved

src/defrag.c Outdated Show resolved Hide resolved

src/defrag.c Outdated Show resolved Hide resolved

sundb and others added 2 commits February 26, 2024 23:42

Add test for pubsub defragment

8cd3aff

determine cursor based on defrag_state

389499f

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

sundb commented Feb 26, 2024

View reviewed changes

tests/unit/memefficiency.tcl Outdated Show resolved Hide resolved

sundb added 5 commits February 27, 2024 13:54

Refract the defrag stages of defragment

eed971f

Improve the test to make sure we didn't break the pubsub data structure

87d3689

Fix outdated comment

563d177

Improve comments

364c738

sundb commented Feb 27, 2024

View reviewed changes

src/defrag.c Show resolved Hide resolved

sundb requested a review from oranagra February 28, 2024 01:17

break loop if defrag_state != i

e6d5b8e

Before this PR we are always interating from stage 0 to 3, but now we're starting from the last stage, so when defrag_stage is not equal i, it means that the last stage isn't finished, so there is no point in iterating further.

oranagra approved these changes Feb 28, 2024

View reviewed changes

sundb added 3 commits March 1, 2024 15:53

Caculate the next slot from all defag stages instead of db->key

dda6799

Improve test

cf8694e

Remove test config in test

e99d534

sundb added 6 commits March 2, 2024 15:23

Improve test to Fix failed test in 32bit

558991b

Revert unnecessary change

dc91022

Revert unnecessary change

f8361bf

Merge branch 'unstable' into defrag_pubsub

1ae72ef

Cleanup

e427156

Cleanup test

45bc313

sundb requested a review from oranagra March 2, 2024 11:21

oranagra reviewed Mar 2, 2024

View reviewed changes

src/defrag.c Outdated Show resolved Hide resolved

sundb commented Mar 3, 2024

View reviewed changes

src/defrag.c Outdated Show resolved Hide resolved

sundb and others added 2 commits March 4, 2024 11:51

draft: defrag by stage, not by slot

4209c51

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

Cleanup and comment

3de261d

oranagra approved these changes Mar 4, 2024

View reviewed changes

src/defrag.c Outdated Show resolved Hide resolved

sundb added 3 commits March 4, 2024 19:22

Add kvstoreGetFirstNonEmptyDictIndex()

47b9468

Merge branch 'unstable' into defrag_pubsub

90ba2fc

Fix comment

beaf94d

oranagra merged commit ad12730 into redis:unstable Mar 4, 2024

sundb deleted the defrag_pubsub branch March 4, 2024 15:06

sundb mentioned this pull request Jun 3, 2024

Fixed crashes due to missed slotToKeyInit() and missed expires_cursor reset #13315

Merged

This was referenced Aug 7, 2025

[BUG] High RSS Memory Usage Even with Active Defrag #13612

Closed

Fix missing expires_cursor check when existing defrag cycle #14270

Merged

sundb mentioned this pull request Sep 4, 2025

Fix defrag issues for pubsub and lua #14330

Merged

Implement defragmentation for pubsub kvstore #13058

Implement defragmentation for pubsub kvstore #13058

Uh oh!

Conversation

sundb commented Feb 18, 2024 • edited by oranagra Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

This PR make effort to defrag the pubsub kvstore in the following ways:

Other

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

sundb commented Mar 2, 2024

Uh oh!

Uh oh!

Uh oh!

oranagra left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sundb commented Mar 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sundb commented Feb 18, 2024 •

edited by oranagra

Loading

oranagra left a comment •

edited

Loading