Commit f084778c authored by uriyage's avatar uriyage Committed by Oran Agra
Browse files

Optimization: sdsRemoveFreeSpace to avoid realloc on noop (#11766)



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: default avatarOran Agra <oran@redislabs.com>
(cherry picked from commit 46393f98)
(cherry picked from commit b12eeccddd9318a5d97a5aee2dad88999dfad53f)
parent 658f9bf8
...@@ -2558,7 +2558,7 @@ int RM_StringTruncate(RedisModuleKey *key, size_t newlen) { ...@@ -2558,7 +2558,7 @@ int RM_StringTruncate(RedisModuleKey *key, size_t newlen) {
sdssubstr(key->value->ptr,0,newlen); sdssubstr(key->value->ptr,0,newlen);
/* If the string is too wasteful, reallocate it. */ /* If the string is too wasteful, reallocate it. */
if (sdslen(key->value->ptr) < sdsavail(key->value->ptr)) if (sdslen(key->value->ptr) < sdsavail(key->value->ptr))
key->value->ptr = sdsRemoveFreeSpace(key->value->ptr); key->value->ptr = sdsRemoveFreeSpace(key->value->ptr, 0);
} }
} }
return REDISMODULE_OK; return REDISMODULE_OK;
......
...@@ -430,7 +430,7 @@ void trimStringObjectIfNeeded(robj *o) { ...@@ -430,7 +430,7 @@ void trimStringObjectIfNeeded(robj *o) {
if (o->encoding == OBJ_ENCODING_RAW && if (o->encoding == OBJ_ENCODING_RAW &&
sdsavail(o->ptr) > sdslen(o->ptr)/10) sdsavail(o->ptr) > sdslen(o->ptr)/10)
{ {
o->ptr = sdsRemoveFreeSpace(o->ptr); o->ptr = sdsRemoveFreeSpace(o->ptr, 0);
} }
} }
......
...@@ -287,40 +287,72 @@ sds sdsMakeRoomFor(sds s, size_t addlen) { ...@@ -287,40 +287,72 @@ sds sdsMakeRoomFor(sds s, size_t addlen) {
* *
* After the call, the passed sds string is no longer valid and all the * After the call, the passed sds string is no longer valid and all the
* references must be substituted with the new pointer returned by the call. */ * references must be substituted with the new pointer returned by the call. */
sds sdsRemoveFreeSpace(sds s) { sds sdsRemoveFreeSpace(sds s, int would_regrow) {
return sdsResize(s, sdslen(s), would_regrow);
}
/* Resize the allocation, this can make the allocation bigger or smaller,
* if the size is smaller than currently used len, the data will be truncated.
*
* The when the would_regrow argument is set to 1, it prevents the use of
* SDS_TYPE_5, which is desired when the sds is likely to be changed again.
*
* The sdsAlloc size will be set to the requested size regardless of the actual
* allocation size, this is done in order to avoid repeated calls to this
* function when the caller detects that it has excess space. */
sds sdsResize(sds s, size_t size, int would_regrow) {
void *sh, *newsh; void *sh, *newsh;
char type, oldtype = s[-1] & SDS_TYPE_MASK; char type, oldtype = s[-1] & SDS_TYPE_MASK;
int hdrlen, oldhdrlen = sdsHdrSize(oldtype); int hdrlen, oldhdrlen = sdsHdrSize(oldtype);
size_t len = sdslen(s); size_t len = sdslen(s);
size_t avail = sdsavail(s);
sh = (char*)s-oldhdrlen; sh = (char*)s-oldhdrlen;
/* Return ASAP if there is no space left. */ /* Return ASAP if the size is already good. */
if (avail == 0) return s; if (sdsalloc(s) == size) return s;
/* Truncate len if needed. */
if (size < len) len = size;
/* Check what would be the minimum SDS header that is just good enough to /* Check what would be the minimum SDS header that is just good enough to
* fit this string. */ * fit this string. */
type = sdsReqType(len); type = sdsReqType(size);
if (would_regrow) {
/* Don't use type 5, it is not good for strings that are expected to grow back. */
if (type == SDS_TYPE_5) type = SDS_TYPE_8;
}
hdrlen = sdsHdrSize(type); hdrlen = sdsHdrSize(type);
/* If the type is the same, or at least a large enough type is still /* If the type is the same, or can hold the size in it with low overhead
* required, we just realloc(), letting the allocator to do the copy * (larger than SDS_TYPE_8), we just realloc(), letting the allocator
* only if really needed. Otherwise if the change is huge, we manually * to do the copy only if really needed. Otherwise if the change is
* reallocate the string to use the different header type. */ * huge, we manually reallocate the string to use the different header
if (oldtype==type || type > SDS_TYPE_8) { * type. */
newsh = s_realloc(sh, oldhdrlen+len+1); int use_realloc = (oldtype==type || (type < oldtype && type > SDS_TYPE_8));
size_t newlen = use_realloc ? oldhdrlen+size+1 : hdrlen+size+1;
int alloc_already_optimal = 0;
#if defined(USE_JEMALLOC)
/* je_nallocx returns the expected allocation size for the newlen.
* We aim to avoid calling realloc() when using Jemalloc if there is no
* change in the allocation size, as it incurs a cost even if the
* allocation size stays the same. */
alloc_already_optimal = (je_nallocx(newlen, 0) == zmalloc_size(sh));
#endif
if (use_realloc && !alloc_already_optimal) {
newsh = s_realloc(sh, newlen);
if (newsh == NULL) return NULL; if (newsh == NULL) return NULL;
s = (char*)newsh+oldhdrlen; s = (char*)newsh+oldhdrlen;
} else { } else if (!alloc_already_optimal) {
newsh = s_malloc(hdrlen+len+1); newsh = s_malloc(newlen);
if (newsh == NULL) return NULL; if (newsh == NULL) return NULL;
memcpy((char*)newsh+hdrlen, s, len+1); memcpy((char*)newsh+hdrlen, s, len);
s_free(sh); s_free(sh);
s = (char*)newsh+hdrlen; s = (char*)newsh+hdrlen;
s[-1] = type; s[-1] = type;
sdssetlen(s, len);
} }
sdssetalloc(s, len); s[len] = 0;
sdssetlen(s, len);
sdssetalloc(s, size);
return s; return s;
} }
...@@ -1455,6 +1487,34 @@ int sdsTest(int argc, char **argv, int accurate) { ...@@ -1455,6 +1487,34 @@ int sdsTest(int argc, char **argv, int accurate) {
test_cond("sdstemplate() with quoting", test_cond("sdstemplate() with quoting",
memcmp(x,"v1={value1} {} v2=value2",24) == 0); memcmp(x,"v1={value1} {} v2=value2",24) == 0);
sdsfree(x); sdsfree(x);
/* Test sdsresize - extend */
x = sdsnew("1234567890123456789012345678901234567890");
x = sdsResize(x, 200, 1);
test_cond("sdsrezie() expand len", sdslen(x) == 40);
test_cond("sdsrezie() expand strlen", strlen(x) == 40);
test_cond("sdsrezie() expand alloc", sdsalloc(x) == 200);
/* Test sdsresize - trim free space */
x = sdsResize(x, 80, 1);
test_cond("sdsrezie() shrink len", sdslen(x) == 40);
test_cond("sdsrezie() shrink strlen", strlen(x) == 40);
test_cond("sdsrezie() shrink alloc", sdsalloc(x) == 80);
/* Test sdsresize - crop used space */
x = sdsResize(x, 30, 1);
test_cond("sdsrezie() crop len", sdslen(x) == 30);
test_cond("sdsrezie() crop strlen", strlen(x) == 30);
test_cond("sdsrezie() crop alloc", sdsalloc(x) == 30);
/* Test sdsresize - extend to different class */
x = sdsResize(x, 400, 1);
test_cond("sdsrezie() expand len", sdslen(x) == 30);
test_cond("sdsrezie() expand strlen", strlen(x) == 30);
test_cond("sdsrezie() expand alloc", sdsalloc(x) == 400);
/* Test sdsresize - shrink to different class */
x = sdsResize(x, 4, 1);
test_cond("sdsrezie() crop len", sdslen(x) == 4);
test_cond("sdsrezie() crop strlen", strlen(x) == 4);
test_cond("sdsrezie() crop alloc", sdsalloc(x) == 4);
sdsfree(x);
} }
test_report(); test_report();
return 0; return 0;
......
...@@ -265,7 +265,8 @@ sds sdstemplate(const char *template, sdstemplate_callback_t cb_func, void *cb_a ...@@ -265,7 +265,8 @@ sds sdstemplate(const char *template, sdstemplate_callback_t cb_func, void *cb_a
/* Low level functions exposed to the user API */ /* Low level functions exposed to the user API */
sds sdsMakeRoomFor(sds s, size_t addlen); sds sdsMakeRoomFor(sds s, size_t addlen);
void sdsIncrLen(sds s, ssize_t incr); void sdsIncrLen(sds s, ssize_t incr);
sds sdsRemoveFreeSpace(sds s); sds sdsRemoveFreeSpace(sds s, int would_regrow);
sds sdsResize(sds s, size_t size, int would_regrow);
size_t sdsAllocSize(sds s); size_t sdsAllocSize(sds s);
void *sdsAllocPtr(sds s); void *sdsAllocPtr(sds s);
......
...@@ -1688,7 +1688,7 @@ int clientsCronResizeQueryBuffer(client *c) { ...@@ -1688,7 +1688,7 @@ int clientsCronResizeQueryBuffer(client *c) {
/* Only resize the query buffer if it is actually wasting /* Only resize the query buffer if it is actually wasting
* at least a few kbytes. */ * at least a few kbytes. */
if (sdsavail(c->querybuf) > 1024*4) { if (sdsavail(c->querybuf) > 1024*4) {
c->querybuf = sdsRemoveFreeSpace(c->querybuf); c->querybuf = sdsRemoveFreeSpace(c->querybuf,1);
} }
} }
/* Reset the peak again to capture the peak memory usage in the next /* Reset the peak again to capture the peak memory usage in the next
...@@ -1708,7 +1708,7 @@ int clientsCronResizeQueryBuffer(client *c) { ...@@ -1708,7 +1708,7 @@ int clientsCronResizeQueryBuffer(client *c) {
if(pending_querybuf_size > LIMIT_PENDING_QUERYBUF && if(pending_querybuf_size > LIMIT_PENDING_QUERYBUF &&
sdslen(c->pending_querybuf) < (pending_querybuf_size/2)) sdslen(c->pending_querybuf) < (pending_querybuf_size/2))
{ {
c->pending_querybuf = sdsRemoveFreeSpace(c->pending_querybuf); c->pending_querybuf = sdsRemoveFreeSpace(c->pending_querybuf,1);
} }
} }
return 0; return 0;
......
...@@ -457,6 +457,15 @@ start_server {tags {"string"}} { ...@@ -457,6 +457,15 @@ start_server {tags {"string"}} {
assert_equal [string range $bin $_start $_end] [r getrange bin $start $end] assert_equal [string range $bin $_start $_end] [r getrange bin $start $end]
} }
} }
test {trim on SET with big value} {
# set a big value to trigger increasing the query buf
r set key [string repeat A 100000]
# set a smaller value but > PROTO_MBULK_BIG_ARG (32*1024) Redis will try to save the query buf itself on the DB.
r set key [string repeat A 33000]
# asset the value was trimmed
assert {[r memory usage key] < 42000}; # 42K to count for Jemalloc's additional memory overhead.
}
test {Extended SET can detect syntax errors} { test {Extended SET can detect syntax errors} {
set e {} set e {}
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment