Skip to content

Optimization: sdsRemoveFreeSpace to avoid realloc on noop#11766

Merged
oranagra merged 3 commits intoredis:unstablefrom
uriyage:avoid_realloc_on_noop
Jan 31, 2023
Merged

Optimization: sdsRemoveFreeSpace to avoid realloc on noop#11766
oranagra merged 3 commits intoredis:unstablefrom
uriyage:avoid_realloc_on_noop

Conversation

@uriyage
Copy link
Contributor

@uriyage uriyage commented Jan 30, 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.

@oranagra
Copy link
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.
i also added a commit with minor cleanup and improved comments.

@uriyage
Copy link
Contributor Author

uriyage commented Jan 31, 2023

@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. i also added a commit with minor cleanup and improved comments.

Thanks @oranagra, the top comment (sorry for forgetting it) and the changes LGTM.

@oranagra oranagra changed the title sdsRemoveFreeSpace to avoid realloc on noop Optimization: sdsRemoveFreeSpace to avoid realloc on noop Jan 31, 2023
@oranagra oranagra merged commit 46393f9 into redis:unstable Jan 31, 2023
@oranagra
Copy link
Member

@yossigo do you think we wanna backport this one to 6.2 (uses je_nallocx)?
or maybe we should start by backporting it into 7.0, and decide about 6.2 in the next release?
(note that it fixes a regression in 6.2)

This was referenced Feb 27, 2023
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants