Skip to content
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

Improve performance of hfe listpack #13279

Merged
merged 8 commits into from
May 22, 2024

Conversation

tezc
Copy link
Collaborator

@tezc tezc commented May 18, 2024

This PR contains a few optimizations for hfe listpack.

  • Hfe fields are ordered by TTL in the listpack. There are two cases that we want to search listpack according to TTLs:

    • As part of active-expiry, we need to find the fields that are expired. e.g. find fields that have smaller TTLs than given timestamp.
    • When we want to add a new field, we need to find the correct position to maintain the order by TTL. e.g. find the field that has a higher TTL than the one we want to insert.

    Iterating with lpNext() to compare TTLs has a performance cost as lpNext() calls lpValidateIntegrity() for each entry. Instead, this PR adds lpFindCb() to the listpack which accepts a comparator callback. It preserves same validation logic of lpFind() which is faster than search with lpNext().

  • We have field name, value, ttl for a single hfe field. Inserting these items one by one to listpack is costly. Especially, as we place fields according to TTL, most additions will end up in the middle of the listpack. Each insert causes realloc + memmove. This PR introduces lpBatchInsert() to add multiple items in one go.

  • For hsetf, if we are going to update value and TTL at the same time, currently, we update the value first and later update the TTL (two distinct listpack operation). This PR improves it by doing it with a single update operation.

@tezc tezc requested a review from sundb May 18, 2024 17:34
src/listpack.c Outdated Show resolved Hide resolved
src/listpack.c Outdated Show resolved Hide resolved
src/t_hash.c Outdated Show resolved Hide resolved
src/listpack.h Outdated Show resolved Hide resolved
src/listpack.c Outdated Show resolved Hide resolved
src/listpack.c Outdated Show resolved Hide resolved
src/listpack.c Outdated Show resolved Hide resolved
src/listpack.c Outdated Show resolved Hide resolved
@tezc tezc marked this pull request as ready for review May 20, 2024 06:15
src/listpack.c Outdated Show resolved Hide resolved
@tezc
Copy link
Collaborator Author

tezc commented May 21, 2024

@sundb I addressed your comments and moved lpBatchInsert() as a separate function. Let's see if we are more comfortable with this one. Maybe we should add a comment that this function might be merged with lpInsert() later in future.

@sundb
Copy link
Collaborator

sundb commented May 21, 2024

@tezc yeah, it's clear now.
actually, we can use lpDeleteRangeWithEntry instead lpInsert() to do the deletion operation, which is more efficient in the future, then we can remove lpInsert() at that time.

Copy link
Collaborator

@sundb sundb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but not look closely at the t_hash part.

src/t_hash.c Outdated Show resolved Hide resolved
src/t_hash.c Outdated Show resolved Hide resolved
src/t_hash.c Outdated Show resolved Hide resolved
src/t_hash.c Outdated Show resolved Hide resolved
Co-authored-by: debing.sun <debing.sun@redis.com>
sundb
sundb previously approved these changes May 21, 2024
src/listpack.c Outdated Show resolved Hide resolved
sundb
sundb previously approved these changes May 21, 2024
@tezc tezc merged commit a25b153 into redis:hash-field-expiry-integ May 22, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants