diff --git a/src/hyperloglog.c b/src/hyperloglog.c index a8f7ebd13..ffe740fab 100644 --- a/src/hyperloglog.c +++ b/src/hyperloglog.c @@ -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 diff --git a/src/module.c b/src/module.c index 481553f12..2eab04b2d 100644 --- a/src/module.c +++ b/src/module.c @@ -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; diff --git a/src/object.c b/src/object.c index 67e372032..f64695061 100644 --- a/src/object.c +++ b/src/object.c @@ -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); } } diff --git a/src/sds.c b/src/sds.c index e0e726dbb..563f1f0fe 100644 --- a/src/sds.c +++ b/src/sds.c @@ -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); diff --git a/src/sds.h b/src/sds.h index 548ba231d..208eaa210 100644 --- a/src/sds.h +++ b/src/sds.h @@ -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); diff --git a/src/server.c b/src/server.c index 8b4ec28e4..0d8d057bd 100644 --- a/src/server.c +++ b/src/server.c @@ -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); } } diff --git a/tests/unit/type/string.tcl b/tests/unit/type/string.tcl index 36b349e09..f32f72368 100644 --- a/tests/unit/type/string.tcl +++ b/tests/unit/type/string.tcl @@ -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 {}