gvsafronov/futriix
1
0
Fork 0
Code Issues Pull Requests Actions 4 Packages Projects Releases Wiki Activity

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

Browse Source 
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 <oran@redislabs.com>
This commit is contained in:
uriyage 2023-01-31 17:26:35 +02:00 committed by GitHub
parent e74a1f3bd9
commit 46393f9819
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 51 additions and 55 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
src/hyperloglog.c
View File

@ -676,7 +676,7 @@ int hllSparseSet(robj *o, long index, uint8_t count) {
newlen += min(newlen, 300); /* Greediness: double 'newlen' if it is smaller than 300, or add 300 to it when it exceeds 300 */
if (newlen > server.hll_sparse_max_bytes)
newlen = server.hll_sparse_max_bytes;
o->ptr = sdsResize(o->ptr, newlen);
o->ptr = sdsResize(o->ptr, newlen, 1);
}
/* Step 1: we need to locate the opcode we need to modify to check

2
src/module.c
View File

@ -4113,7 +4113,7 @@ int RM_StringTruncate(RedisModuleKey *key, size_t newlen) {
sdssubstr(key->value->ptr,0,newlen);
/* If the string is too wasteful, reallocate it. */
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;

2
src/object.c
View File

@ -616,7 +616,7 @@ void trimStringObjectIfNeeded(robj *o) {
if (o->encoding == OBJ_ENCODING_RAW &&
sdsavail(o->ptr) > sdslen(o->ptr)/10)
{
o->ptr = sdsRemoveFreeSpace(o->ptr);
o->ptr = sdsRemoveFreeSpace(o->ptr, 0);
}
}

83
src/sds.c
View File

@ -306,46 +306,20 @@ sds sdsMakeRoomForNonGreedy(sds s, size_t addlen) {
*
* 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. */
sds sdsRemoveFreeSpace(sds s) {
void *sh, *newsh;
char type, oldtype = s[-1] & SDS_TYPE_MASK;
int hdrlen, oldhdrlen = sdsHdrSize(oldtype);
size_t len = sdslen(s);
size_t avail = sdsavail(s);
sh = (char*)s-oldhdrlen;
/* Return ASAP if there is no space left. */
if (avail == 0) return s;
/* Check what would be the minimum SDS header that is just good enough to
* fit this string. */
type = sdsReqType(len);
hdrlen = sdsHdrSize(type);
/* If the type is the same, or at least a large enough type is still
* required, we just realloc(), letting the allocator to do the copy
* only if really needed. Otherwise if the change is huge, we manually
* reallocate the string to use the different header type. */
if (oldtype==type || type > SDS_TYPE_8) {
newsh = s_realloc(sh, oldhdrlen+len+1);
if (newsh == NULL) return NULL;
s = (char*)newsh+oldhdrlen;
} else {
newsh = s_malloc(hdrlen+len+1);
if (newsh == NULL) return NULL;
memcpy((char*)newsh+hdrlen, s, len+1);
s_free(sh);
s = (char*)newsh+hdrlen;
s[-1] = type;
sdssetlen(s, len);
}
sdssetalloc(s, len);
return 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 */
sds sdsResize(sds s, size_t size) {
* 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;
char type, oldtype = s[-1] & SDS_TYPE_MASK;
int hdrlen, oldhdrlen = sdsHdrSize(oldtype);
@ -361,8 +335,10 @@ sds sdsResize(sds s, size_t size) {
/* Check what would be the minimum SDS header that is just good enough to
* fit this string. */
type = sdsReqType(size);
/* Don't use type 5, it is not good for strings that are resized. */
if (type == SDS_TYPE_5) type = SDS_TYPE_8;
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);
/* If the type is the same, or can hold the size in it with low overhead
@ -370,12 +346,23 @@ sds sdsResize(sds s, size_t size) {
* to do the copy only if really needed. Otherwise if the change is
* huge, we manually reallocate the string to use the different header
* type. */
if (oldtype==type || (type < oldtype && type > SDS_TYPE_8)) {
newsh = s_realloc(sh, oldhdrlen+size+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;
s = (char*)newsh+oldhdrlen;
} else {
newsh = s_malloc(hdrlen+size+1);
} else if (!alloc_already_optimal) {
newsh = s_malloc(newlen);
if (newsh == NULL) return NULL;
memcpy((char*)newsh+hdrlen, s, len);
s_free(sh);
@ -1552,27 +1539,27 @@ int sdsTest(int argc, char **argv, int flags) {
/* Test sdsresize - extend */
x = sdsnew("1234567890123456789012345678901234567890");
x = sdsResize(x, 200);
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);
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);
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);
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);
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);

4
src/sds.h
View File

@ -267,8 +267,8 @@ sds sdstemplate(const char *template, sdstemplate_callback_t cb_func, void *cb_a
sds sdsMakeRoomFor(sds s, size_t addlen);
sds sdsMakeRoomForNonGreedy(sds s, size_t addlen);
void sdsIncrLen(sds s, ssize_t incr);
sds sdsRemoveFreeSpace(sds s);
sds sdsResize(sds s, size_t size);
sds sdsRemoveFreeSpace(sds s, int would_regrow);
sds sdsResize(sds s, size_t size, int would_regrow);
size_t sdsAllocSize(sds s);
void *sdsAllocPtr(sds s);

4
src/server.c
View File

@ -739,7 +739,7 @@ int clientsCronResizeQueryBuffer(client *c) {
/* There are two conditions to resize the query buffer: */
if (idletime > 2) {
/* 1) Query is idle for a long time. */
c->querybuf = sdsRemoveFreeSpace(c->querybuf);
c->querybuf = sdsRemoveFreeSpace(c->querybuf, 1);
} else if (querybuf_size > PROTO_RESIZE_THRESHOLD && querybuf_size/2 > c->querybuf_peak) {
/* 2) Query buffer is too big for latest peak and is larger than
* resize threshold. Trim excess space but only up to a limit,
@ -749,7 +749,7 @@ int clientsCronResizeQueryBuffer(client *c) {
size_t resize = sdslen(c->querybuf);
if (resize < c->querybuf_peak) resize = c->querybuf_peak;
if (c->bulklen != -1 && resize < (size_t)c->bulklen) resize = c->bulklen;
c->querybuf = sdsResize(c->querybuf, resize);
c->querybuf = sdsResize(c->querybuf, resize, 1);
}
}

9
tests/unit/type/string.tcl
View File

@ -459,6 +459,15 @@ start_server {tags {"string"}} {
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} {
set e {}