Optimization: sdsRemoveFreeSpace to avoid realloc on noop#11766
Merged
oranagra merged 3 commits intoredis:unstablefrom Jan 31, 2023
Merged
Optimization: sdsRemoveFreeSpace to avoid realloc on noop#11766oranagra merged 3 commits intoredis:unstablefrom
oranagra merged 3 commits intoredis:unstablefrom
Conversation
Member
|
@uriyage i added a detailed top comment (which was very much missing), please go over it and let me know if anything is wrong or needs improvements. |
Contributor
Author
oranagra
approved these changes
Jan 31, 2023
Member
|
@yossigo do you think we wanna backport this one to 6.2 (uses |
oranagra
added a commit
that referenced
this pull request
Feb 28, 2023
In #7875 (Redis 6.2), we changed the sds alloc to be the usable allocation size in order to: > reduce the need for realloc calls by making the sds implicitly take over the internal fragmentation This change was done most sds functions, excluding `sdsRemoveFreeSpace` and `sdsResize`, the reason is that in some places (e.g. clientsCronResizeQueryBuffer) we call sdsRemoveFreeSpace when we see excessive free space and want to trim it. so if we don't trim it exactly to size, the caller may still see excessive free space and call it again and again. However, this resulted in some excessive calls to realloc, even when there's no need and it's gonna be a no-op (e.g. when reducing 15 bytes allocation to 13). It turns out that a call for realloc with jemalloc can be expensive even if it ends up doing nothing, so this PR adds a check using `je_nallocx`, which is cheap to avoid the call for realloc. in addition to that this PR unifies sdsResize and sdsRemoveFreeSpace into common code. the difference between them was that sdsResize would avoid using SDS_TYPE_5, since it want to keep the string ready to be resized again, while sdsRemoveFreeSpace would permit using SDS_TYPE_5 and get an optimal memory consumption. now both methods take a `would_regrow` argument that makes it more explicit. the only actual impact of that is that in clientsCronResizeQueryBuffer we call both sdsResize and sdsRemoveFreeSpace for in different cases, and we now prevent the use of SDS_TYPE_5 in both. The new test that was added to cover this concern used to pass before this PR as well, this PR is just a performance optimization and cleanup. Benchmark: `redis-benchmark -c 100 -t set -d 512 -P 10 -n 100000000` on i7-9850H with jemalloc, shows improvement from 1021k ops/sec to 1067k (average of 3 runs). some 4.5% improvement. Co-authored-by: Oran Agra <[email protected]> (cherry picked from commit 46393f9) (cherry picked from commit b12eeccddd9318a5d97a5aee2dad88999dfad53f)
oranagra
added a commit
that referenced
this pull request
Feb 28, 2023
In #7875 (Redis 6.2), we changed the sds alloc to be the usable allocation size in order to: > reduce the need for realloc calls by making the sds implicitly take over the internal fragmentation This change was done most sds functions, excluding `sdsRemoveFreeSpace` and `sdsResize`, the reason is that in some places (e.g. clientsCronResizeQueryBuffer) we call sdsRemoveFreeSpace when we see excessive free space and want to trim it. so if we don't trim it exactly to size, the caller may still see excessive free space and call it again and again. However, this resulted in some excessive calls to realloc, even when there's no need and it's gonna be a no-op (e.g. when reducing 15 bytes allocation to 13). It turns out that a call for realloc with jemalloc can be expensive even if it ends up doing nothing, so this PR adds a check using `je_nallocx`, which is cheap to avoid the call for realloc. in addition to that this PR unifies sdsResize and sdsRemoveFreeSpace into common code. the difference between them was that sdsResize would avoid using SDS_TYPE_5, since it want to keep the string ready to be resized again, while sdsRemoveFreeSpace would permit using SDS_TYPE_5 and get an optimal memory consumption. now both methods take a `would_regrow` argument that makes it more explicit. the only actual impact of that is that in clientsCronResizeQueryBuffer we call both sdsResize and sdsRemoveFreeSpace for in different cases, and we now prevent the use of SDS_TYPE_5 in both. The new test that was added to cover this concern used to pass before this PR as well, this PR is just a performance optimization and cleanup. Benchmark: `redis-benchmark -c 100 -t set -d 512 -P 10 -n 100000000` on i7-9850H with jemalloc, shows improvement from 1021k ops/sec to 1067k (average of 3 runs). some 4.5% improvement. Co-authored-by: Oran Agra <[email protected]> (cherry picked from commit 46393f9)
enjoy-binbin
pushed a commit
to enjoy-binbin/redis
that referenced
this pull request
Jul 31, 2023
In redis#7875 (Redis 6.2), we changed the sds alloc to be the usable allocation size in order to: > reduce the need for realloc calls by making the sds implicitly take over the internal fragmentation This change was done most sds functions, excluding `sdsRemoveFreeSpace` and `sdsResize`, the reason is that in some places (e.g. clientsCronResizeQueryBuffer) we call sdsRemoveFreeSpace when we see excessive free space and want to trim it. so if we don't trim it exactly to size, the caller may still see excessive free space and call it again and again. However, this resulted in some excessive calls to realloc, even when there's no need and it's gonna be a no-op (e.g. when reducing 15 bytes allocation to 13). It turns out that a call for realloc with jemalloc can be expensive even if it ends up doing nothing, so this PR adds a check using `je_nallocx`, which is cheap to avoid the call for realloc. in addition to that this PR unifies sdsResize and sdsRemoveFreeSpace into common code. the difference between them was that sdsResize would avoid using SDS_TYPE_5, since it want to keep the string ready to be resized again, while sdsRemoveFreeSpace would permit using SDS_TYPE_5 and get an optimal memory consumption. now both methods take a `would_regrow` argument that makes it more explicit. the only actual impact of that is that in clientsCronResizeQueryBuffer we call both sdsResize and sdsRemoveFreeSpace for in different cases, and we now prevent the use of SDS_TYPE_5 in both. The new test that was added to cover this concern used to pass before this PR as well, this PR is just a performance optimization and cleanup. Benchmark: `redis-benchmark -c 100 -t set -d 512 -P 10 -n 100000000` on i7-9850H with jemalloc, shows improvement from 1021k ops/sec to 1067k (average of 3 runs). some 4.5% improvement. Co-authored-by: Oran Agra <[email protected]>
oranagra
added a commit
that referenced
this pull request
Jan 9, 2024
#11766 introduced a bug in sdsResize where it could forget to update the sds type in the sds header and then cause an overflow in sdsalloc. it looks like the only implication of that is a possible assertion in HLL, but it's hard to rule out possible heap corruption issues with clientsCronResizeQueryBuffer
oranagra
added a commit
that referenced
this pull request
Jan 9, 2024
#11766 introduced a bug in sdsResize where it could forget to update the sds type in the sds header and then cause an overflow in sdsalloc. it looks like the only implication of that is a possible assertion in HLL, but it's hard to rule out possible heap corruption issues with clientsCronResizeQueryBuffer
oranagra
added a commit
that referenced
this pull request
Jan 9, 2024
#11766 introduced a bug in sdsResize where it could forget to update the sds type in the sds header and then cause an overflow in sdsalloc. it looks like the only implication of that is a possible assertion in HLL, but it's hard to rule out possible heap corruption issues with clientsCronResizeQueryBuffer
roggervalf
pushed a commit
to roggervalf/redis
that referenced
this pull request
Feb 11, 2024
redis#11766 introduced a bug in sdsResize where it could forget to update the sds type in the sds header and then cause an overflow in sdsalloc. it looks like the only implication of that is a possible assertion in HLL, but it's hard to rule out possible heap corruption issues with clientsCronResizeQueryBuffer
funny-dog
pushed a commit
to funny-dog/redis
that referenced
this pull request
Sep 17, 2025
redis#11766 introduced a bug in sdsResize where it could forget to update the sds type in the sds header and then cause an overflow in sdsalloc. it looks like the only implication of that is a possible assertion in HLL, but it's hard to rule out possible heap corruption issues with clientsCronResizeQueryBuffer
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In #7875 (Redis 6.2), we changed the sds alloc to be the usable allocation size in order to:
This change was done most sds functions, excluding
sdsRemoveFreeSpaceandsdsResize, the reason is that in some places (e.g. clientsCronResizeQueryBuffer) we call sdsRemoveFreeSpace when we see excessive free space and want to trim it.so if we don't trim it exactly to size, the caller may still see excessive free space and call it again and again.
However, this resulted in some excessive calls to realloc, even when there's no need and it's gonna be a no-op (e.g. when reducing 15 bytes allocation to 13).
It turns out that a call for realloc with jemalloc can be expensive even if it ends up doing nothing, so this PR adds a check using
je_nallocx, which is cheap to avoid the call for realloc.in addition to that this PR unifies sdsResize and sdsRemoveFreeSpace into common code.
the difference between them was that sdsResize would avoid using SDS_TYPE_5, since it want to keep the string ready to be resized again, while sdsRemoveFreeSpace would permit using SDS_TYPE_5 and get an optimal memory consumption.
now both methods take a
would_regrowargument that makes it more explicit.the only actual impact of that is that in clientsCronResizeQueryBuffer we call both sdsResize and sdsRemoveFreeSpace for in different cases, and we now prevent the use of SDS_TYPE_5 in both.
The new test that was added to cover this concern used to pass before this PR as well, this PR is just a performance optimization and cleanup.
Benchmark:
redis-benchmark -c 100 -t set -d 512 -P 10 -n 100000000on i7-9850H with jemalloc, shows improvement from 1021k ops/sec to 1067k (average of 3 runs).
some 4.5% improvement.