Adjust sds types (#502)

sds type should be determined based on the size of the underlying buffer, not the logical length of the sds. Currently we truncate the alloc field in case buffer is larger than we can handle. It leads to a mismatch between alloc field and the actual size of the buffer. Even considering that alloc doesn't include header size and the null terminator. It also leads to a waste of memory with jemalloc. For example, let's consider creation of sds of length 253. According to the length, the appropriate type is SDS_TYPE_8. But we allocate `253 + sizeof(struct sdshdr8) + 1` bytes, which sums to 257 bytes. In this case jemalloc allocates buffer from the next size bucket. With current configuration on Linux it's 320 bytes. So we end up with 320 bytes buffer, while we can't address more than 255. The same happens with other types and length close enough to the appropriate powers of 2. The downside of the adjustment is that with allocators that do not allocate larger than requested chunks (like GNU allocator), we switch to a larger type "too early". It leads to small waste of memory. Specifically: sds of length 31 takes 35 bytes instead of 33 (2 bytes wasted) sds of length 255 takes 261 bytes instead of 259 (2 bytes wasted) sds of length 65,535 takes 65,545 bytes instead of 65,541 (4 bytes wasted) sds of length 4,294,967,295 takes 4,294,967,313 bytes instead of 4,294,967,305 (8 bytes wasted) --------- Signed-off-by: Vadym Khoptynets <[email protected]>
valkey-io · Jun 3, 2024 · 4176604 · 4176604
1 parent 28e055a
commit 4176604
Show file tree

Hide file tree

Showing 4 changed files with 145 additions and 24 deletions.
diff --git a/src/sds.c b/src/sds.c
@@ -55,10 +55,10 @@ static inline int sdsHdrSize(char type) {
 
 static inline char sdsReqType(size_t string_size) {
     if (string_size < 1 << 5) return SDS_TYPE_5;
-    if (string_size < 1 << 8) return SDS_TYPE_8;
-    if (string_size < 1 << 16) return SDS_TYPE_16;
+    if (string_size <= (1 << 8) - sizeof(struct sdshdr8) - 1) return SDS_TYPE_8;
+    if (string_size <= (1 << 16) - sizeof(struct sdshdr16) - 1) return SDS_TYPE_16;
 #if (LONG_MAX == LLONG_MAX)
-    if (string_size < 1ll << 32) return SDS_TYPE_32;
+    if (string_size <= (1ll << 32) - sizeof(struct sdshdr32) - 1) return SDS_TYPE_32;
     return SDS_TYPE_64;
 #else
     return SDS_TYPE_32;
@@ -75,6 +75,16 @@ static inline size_t sdsTypeMaxSize(char type) {
     return -1; /* this is equivalent to the max SDS_TYPE_64 or SDS_TYPE_32 */
 }
 
+static inline int adjustTypeIfNeeded(char *type, int *hdrlen, size_t bufsize) {
+    size_t usable = bufsize - *hdrlen - 1;
+    if (*type != SDS_TYPE_5 && usable > sdsTypeMaxSize(*type)) {
+        *type = sdsReqType(usable);
+        *hdrlen = sdsHdrSize(*type);
+        return 1;
+    }
+    return 0;
+}
+
 /* Create a new sds string with the content specified by the 'init' pointer
  * and 'initlen'.
  * If NULL is used for 'init' the string is initialized with zero bytes.
@@ -97,19 +107,23 @@ sds _sdsnewlen(const void *init, size_t initlen, int trymalloc) {
     if (type == SDS_TYPE_5 && initlen == 0) type = SDS_TYPE_8;
     int hdrlen = sdsHdrSize(type);
     unsigned char *fp; /* flags pointer. */
-    size_t usable;
+    size_t bufsize, usable;
 
     assert(initlen + hdrlen + 1 > initlen); /* Catch size_t overflow */
-    sh = trymalloc ? s_trymalloc_usable(hdrlen + initlen + 1, &usable) : s_malloc_usable(hdrlen + initlen + 1, &usable);
+    sh = trymalloc ? s_trymalloc_usable(hdrlen + initlen + 1, &bufsize)
+                   : s_malloc_usable(hdrlen + initlen + 1, &bufsize);
     if (sh == NULL) return NULL;
     if (init == SDS_NOINIT)
         init = NULL;
     else if (!init)
         memset(sh, 0, hdrlen + initlen + 1);
+
+    adjustTypeIfNeeded(&type, &hdrlen, bufsize);
+    usable = bufsize - hdrlen - 1;
+
     s = (char *)sh + hdrlen;
     fp = ((unsigned char *)s) - 1;
-    usable = usable - hdrlen - 1;
-    if (usable > sdsTypeMaxSize(type)) usable = sdsTypeMaxSize(type);
+
     switch (type) {
     case SDS_TYPE_5: {
         *fp = type | (initlen << SDS_TYPE_BITS);
@@ -118,27 +132,31 @@ sds _sdsnewlen(const void *init, size_t initlen, int trymalloc) {
     case SDS_TYPE_8: {
         SDS_HDR_VAR(8, s);
         sh->len = initlen;
+        assert(usable <= sdsTypeMaxSize(type));
         sh->alloc = usable;
         *fp = type;
         break;
     }
     case SDS_TYPE_16: {
         SDS_HDR_VAR(16, s);
         sh->len = initlen;
+        assert(usable <= sdsTypeMaxSize(type));
         sh->alloc = usable;
         *fp = type;
         break;
     }
     case SDS_TYPE_32: {
         SDS_HDR_VAR(32, s);
         sh->len = initlen;
+        assert(usable <= sdsTypeMaxSize(type));
         sh->alloc = usable;
         *fp = type;
         break;
     }
     case SDS_TYPE_64: {
         SDS_HDR_VAR(64, s);
         sh->len = initlen;
+        assert(usable <= sdsTypeMaxSize(type));
         sh->alloc = usable;
         *fp = type;
         break;
@@ -226,7 +244,8 @@ sds _sdsMakeRoomFor(sds s, size_t addlen, int greedy) {
     size_t len, newlen, reqlen;
     char type, oldtype = s[-1] & SDS_TYPE_MASK;
     int hdrlen;
-    size_t usable;
+    size_t bufsize, usable;
+    int use_realloc;
 
     /* Return ASAP if there is enough space left. */
     if (avail >= addlen) return s;
@@ -251,23 +270,32 @@ sds _sdsMakeRoomFor(sds s, size_t addlen, int greedy) {
 
     hdrlen = sdsHdrSize(type);
     assert(hdrlen + newlen + 1 > reqlen); /* Catch size_t overflow */
-    if (oldtype == type) {
-        newsh = s_realloc_usable(sh, hdrlen + newlen + 1, &usable);
+    use_realloc = (oldtype == type);
+    if (use_realloc) {
+        newsh = s_realloc_usable(sh, hdrlen + newlen + 1, &bufsize);
         if (newsh == NULL) return NULL;
         s = (char *)newsh + hdrlen;
+
+        if (adjustTypeIfNeeded(&type, &hdrlen, bufsize)) {
+            memmove((char *)newsh + hdrlen, s, len + 1);
+            s = (char *)newsh + hdrlen;
+            s[-1] = type;
+            sdssetlen(s, len);
+        }
     } else {
         /* Since the header size changes, need to move the string forward,
          * and can't use realloc */
-        newsh = s_malloc_usable(hdrlen + newlen + 1, &usable);
+        newsh = s_malloc_usable(hdrlen + newlen + 1, &bufsize);
         if (newsh == NULL) return NULL;
+        adjustTypeIfNeeded(&type, &hdrlen, bufsize);
         memcpy((char *)newsh + hdrlen, s, len + 1);
         s_free(sh);
         s = (char *)newsh + hdrlen;
         s[-1] = type;
         sdssetlen(s, len);
     }
-    usable = usable - hdrlen - 1;
-    if (usable > sdsTypeMaxSize(type)) usable = sdsTypeMaxSize(type);
+    usable = bufsize - hdrlen - 1;
+    assert(type == SDS_TYPE_5 || usable <= sdsTypeMaxSize(type));
     sdssetalloc(s, usable);
     return s;
 }
@@ -303,7 +331,7 @@ sds sdsRemoveFreeSpace(sds s, int would_regrow) {
  * 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 = NULL;
     char type, oldtype = s[-1] & SDS_TYPE_MASK;
     int hdrlen, oldhdrlen = sdsHdrSize(oldtype);
     size_t len = sdslen(s);
@@ -331,7 +359,8 @@ sds sdsResize(sds s, size_t size, int would_regrow) {
      * type. */
     int use_realloc = (oldtype == type || (type < oldtype && type > SDS_TYPE_8));
     size_t newlen = use_realloc ? oldhdrlen + size + 1 : hdrlen + size + 1;
-    size_t newsize = 0;
+    size_t bufsize = 0;
+    size_t newsize;
 
     if (use_realloc) {
         int alloc_already_optimal = 0;
@@ -340,27 +369,37 @@ sds sdsResize(sds s, size_t size, int would_regrow) {
          * 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. */
-        newsize = zmalloc_size(sh);
-        alloc_already_optimal = (je_nallocx(newlen, 0) == newsize);
+        bufsize = zmalloc_size(sh);
+        alloc_already_optimal = (je_nallocx(newlen, 0) == bufsize);
 #endif
         if (!alloc_already_optimal) {
-            newsh = s_realloc_usable(sh, newlen, &newsize);
+            newsh = s_realloc_usable(sh, newlen, &bufsize);
             if (newsh == NULL) return NULL;
             s = (char *)newsh + oldhdrlen;
-            newsize -= (oldhdrlen + 1);
+
+            if (adjustTypeIfNeeded(&oldtype, &oldhdrlen, bufsize)) {
+                memmove((char *)newsh + oldhdrlen, s, len + 1);
+                s = (char *)newsh + oldhdrlen;
+                s[-1] = oldtype;
+                sdssetlen(s, len);
+            }
         }
+        newsize = bufsize - oldhdrlen - 1;
+        assert(oldtype == SDS_TYPE_5 || newsize <= sdsTypeMaxSize(oldtype));
     } else {
-        newsh = s_malloc_usable(newlen, &newsize);
+        newsh = s_malloc_usable(newlen, &bufsize);
         if (newsh == NULL) return NULL;
-        memcpy((char *)newsh + hdrlen, s, len);
+        adjustTypeIfNeeded(&type, &hdrlen, bufsize);
+        memcpy((char *)newsh + hdrlen, s, len + 1);
         s_free(sh);
         s = (char *)newsh + hdrlen;
         s[-1] = type;
-        newsize -= (hdrlen + 1);
+        newsize = bufsize - hdrlen - 1;
+        assert(type == SDS_TYPE_5 || newsize <= sdsTypeMaxSize(type));
     }
+
     s[len] = '\0';
     sdssetlen(s, len);
-    if (newsize > sdsTypeMaxSize(s[-1])) newsize = sdsTypeMaxSize(s[-1]);
     sdssetalloc(s, newsize);
     return s;
 }

diff --git a/src/sdsalloc.h b/src/sdsalloc.h
@@ -49,5 +49,6 @@
 #define s_realloc_usable zrealloc_usable
 #define s_trymalloc_usable ztrymalloc_usable
 #define s_tryrealloc_usable ztryrealloc_usable
+#define s_malloc_size zmalloc_size
 
 #endif
diff --git a/src/unit/test_files.h b/src/unit/test_files.h
@@ -24,6 +24,8 @@ int test_kvstoreIteratorRemoveAllKeysDeleteEmptyDict(int argc, char **argv, int
 int test_kvstoreDictIteratorRemoveAllKeysNoDeleteEmptyDict(int argc, char **argv, int flags);
 int test_kvstoreDictIteratorRemoveAllKeysDeleteEmptyDict(int argc, char **argv, int flags);
 int test_sds(int argc, char **argv, int flags);
+int test_typesAndAllocSize(int argc, char **argv, int flags);
+int test_sdsHeaderSizes(int argc, char **argv, int flags);
 int test_sha1(int argc, char **argv, int flags);
 int test_string2ll(int argc, char **argv, int flags);
 int test_string2l(int argc, char **argv, int flags);
@@ -77,7 +79,7 @@ unitTest __test_crc64combine_c[] = {{"test_crc64combine", test_crc64combine}, {N
 unitTest __test_endianconv_c[] = {{"test_endianconv", test_endianconv}, {NULL, NULL}};
 unitTest __test_intset_c[] = {{"test_intsetValueEncodings", test_intsetValueEncodings}, {"test_intsetBasicAdding", test_intsetBasicAdding}, {"test_intsetLargeNumberRandomAdd", test_intsetLargeNumberRandomAdd}, {"test_intsetUpgradeFromint16Toint32", test_intsetUpgradeFromint16Toint32}, {"test_intsetUpgradeFromint16Toint64", test_intsetUpgradeFromint16Toint64}, {"test_intsetUpgradeFromint32Toint64", test_intsetUpgradeFromint32Toint64}, {"test_intsetStressLookups", test_intsetStressLookups}, {"test_intsetStressAddDelete", test_intsetStressAddDelete}, {NULL, NULL}};
 unitTest __test_kvstore_c[] = {{"test_kvstoreAdd16Keys", test_kvstoreAdd16Keys}, {"test_kvstoreIteratorRemoveAllKeysNoDeleteEmptyDict", test_kvstoreIteratorRemoveAllKeysNoDeleteEmptyDict}, {"test_kvstoreIteratorRemoveAllKeysDeleteEmptyDict", test_kvstoreIteratorRemoveAllKeysDeleteEmptyDict}, {"test_kvstoreDictIteratorRemoveAllKeysNoDeleteEmptyDict", test_kvstoreDictIteratorRemoveAllKeysNoDeleteEmptyDict}, {"test_kvstoreDictIteratorRemoveAllKeysDeleteEmptyDict", test_kvstoreDictIteratorRemoveAllKeysDeleteEmptyDict}, {NULL, NULL}};
-unitTest __test_sds_c[] = {{"test_sds", test_sds}, {NULL, NULL}};
+unitTest __test_sds_c[] = {{"test_sds", test_sds}, {"test_typesAndAllocSize", test_typesAndAllocSize}, {"test_sdsHeaderSizes", test_sdsHeaderSizes}, {NULL, NULL}};
 unitTest __test_sha1_c[] = {{"test_sha1", test_sha1}, {NULL, NULL}};
 unitTest __test_util_c[] = {{"test_string2ll", test_string2ll}, {"test_string2l", test_string2l}, {"test_ll2string", test_ll2string}, {"test_ld2string", test_ld2string}, {"test_fixedpoint_d2string", test_fixedpoint_d2string}, {"test_version2num", test_version2num}, {"test_reclaimFilePageCache", test_reclaimFilePageCache}, {NULL, NULL}};
 unitTest __test_ziplist_c[] = {{"test_ziplistCreateIntList", test_ziplistCreateIntList}, {"test_ziplistPop", test_ziplistPop}, {"test_ziplistGetElementAtIndex3", test_ziplistGetElementAtIndex3}, {"test_ziplistGetElementOutOfRange", test_ziplistGetElementOutOfRange}, {"test_ziplistGetLastElement", test_ziplistGetLastElement}, {"test_ziplistGetFirstElement", test_ziplistGetFirstElement}, {"test_ziplistGetElementOutOfRangeReverse", test_ziplistGetElementOutOfRangeReverse}, {"test_ziplistIterateThroughFullList", test_ziplistIterateThroughFullList}, {"test_ziplistIterateThroughListFrom1ToEnd", test_ziplistIterateThroughListFrom1ToEnd}, {"test_ziplistIterateThroughListFrom2ToEnd", test_ziplistIterateThroughListFrom2ToEnd}, {"test_ziplistIterateThroughStartOutOfRange", test_ziplistIterateThroughStartOutOfRange}, {"test_ziplistIterateBackToFront", test_ziplistIterateBackToFront}, {"test_ziplistIterateBackToFrontDeletingAllItems", test_ziplistIterateBackToFrontDeletingAllItems}, {"test_ziplistDeleteInclusiveRange0To0", test_ziplistDeleteInclusiveRange0To0}, {"test_ziplistDeleteInclusiveRange0To1", test_ziplistDeleteInclusiveRange0To1}, {"test_ziplistDeleteInclusiveRange1To2", test_ziplistDeleteInclusiveRange1To2}, {"test_ziplistDeleteWithStartIndexOutOfRange", test_ziplistDeleteWithStartIndexOutOfRange}, {"test_ziplistDeleteWithNumOverflow", test_ziplistDeleteWithNumOverflow}, {"test_ziplistDeleteFooWhileIterating", test_ziplistDeleteFooWhileIterating}, {"test_ziplistReplaceWithSameSize", test_ziplistReplaceWithSameSize}, {"test_ziplistReplaceWithDifferentSize", test_ziplistReplaceWithDifferentSize}, {"test_ziplistRegressionTestForOver255ByteStrings", test_ziplistRegressionTestForOver255ByteStrings}, {"test_ziplistRegressionTestDeleteNextToLastEntries", test_ziplistRegressionTestDeleteNextToLastEntries}, {"test_ziplistCreateLongListAndCheckIndices", test_ziplistCreateLongListAndCheckIndices}, {"test_ziplistCompareStringWithZiplistEntries", test_ziplistCompareStringWithZiplistEntries}, {"test_ziplistMergeTest", test_ziplistMergeTest}, {"test_ziplistStressWithRandomPayloadsOfDifferentEncoding", test_ziplistStressWithRandomPayloadsOfDifferentEncoding}, {"test_ziplistCascadeUpdateEdgeCases", test_ziplistCascadeUpdateEdgeCases}, {"test_ziplistInsertEdgeCase", test_ziplistInsertEdgeCase}, {"test_ziplistStressWithVariableSize", test_ziplistStressWithVariableSize}, {"test_BenchmarkziplistFind", test_BenchmarkziplistFind}, {"test_BenchmarkziplistIndex", test_BenchmarkziplistIndex}, {"test_BenchmarkziplistValidateIntegrity", test_BenchmarkziplistValidateIntegrity}, {"test_BenchmarkziplistCompareWithString", test_BenchmarkziplistCompareWithString}, {"test_BenchmarkziplistCompareWithNumber", test_BenchmarkziplistCompareWithNumber}, {"test_ziplistStress__ziplistCascadeUpdate", test_ziplistStress__ziplistCascadeUpdate}, {NULL, NULL}};

diff --git a/src/unit/test_sds.c b/src/unit/test_sds.c
@@ -4,6 +4,7 @@
 #include "test_help.h"
 
 #include "../sds.h"
+#include "../sdsalloc.h"
 
 static sds sdsTestTemplateCallback(sds varname, void *arg) {
     UNUSED(arg);
@@ -247,5 +248,83 @@ int test_sds(int argc, char **argv, int flags) {
     TEST_ASSERT_MESSAGE("sdsReszie() crop strlen", strlen(x) == 4);
     TEST_ASSERT_MESSAGE("sdsReszie() crop alloc", sdsalloc(x) >= 4);
     sdsfree(x);
+
+    return 0;
+}
+
+int test_typesAndAllocSize(int argc, char **argv, int flags) {
+    UNUSED(argc);
+    UNUSED(argv);
+    UNUSED(flags);
+
+    sds x = sdsnewlen(NULL, 31);
+    TEST_ASSERT_MESSAGE("len 31 type", (x[-1] & SDS_TYPE_MASK) == SDS_TYPE_5);
+    sdsfree(x);
+
+    x = sdsnewlen(NULL, 32);
+    TEST_ASSERT_MESSAGE("len 32 type", (x[-1] & SDS_TYPE_MASK) >= SDS_TYPE_8);
+    TEST_ASSERT_MESSAGE("len 32 sdsAllocSize", sdsAllocSize(x) == s_malloc_size(sdsAllocPtr(x)));
+    sdsfree(x);
+
+    x = sdsnewlen(NULL, 252);
+    TEST_ASSERT_MESSAGE("len 252 type", (x[-1] & SDS_TYPE_MASK) >= SDS_TYPE_8);
+    TEST_ASSERT_MESSAGE("len 252 sdsAllocSize", sdsAllocSize(x) == s_malloc_size(sdsAllocPtr(x)));
+    sdsfree(x);
+
+    x = sdsnewlen(NULL, 253);
+    TEST_ASSERT_MESSAGE("len 253 type", (x[-1] & SDS_TYPE_MASK) == SDS_TYPE_16);
+    TEST_ASSERT_MESSAGE("len 253 sdsAllocSize", sdsAllocSize(x) == s_malloc_size(sdsAllocPtr(x)));
+    sdsfree(x);
+
+    x = sdsnewlen(NULL, 65530);
+    TEST_ASSERT_MESSAGE("len 65530 type", (x[-1] & SDS_TYPE_MASK) >= SDS_TYPE_16);
+    TEST_ASSERT_MESSAGE("len 65530 sdsAllocSize", sdsAllocSize(x) == s_malloc_size(sdsAllocPtr(x)));
+    sdsfree(x);
+
+    x = sdsnewlen(NULL, 65531);
+    TEST_ASSERT_MESSAGE("len 65531 type", (x[-1] & SDS_TYPE_MASK) >= SDS_TYPE_32);
+    TEST_ASSERT_MESSAGE("len 65531 sdsAllocSize", sdsAllocSize(x) == s_malloc_size(sdsAllocPtr(x)));
+    sdsfree(x);
+
+#if (LONG_MAX == LLONG_MAX)
+    if (flags & UNIT_TEST_LARGE_MEMORY) {
+        x = sdsnewlen(NULL, 4294967286);
+        TEST_ASSERT_MESSAGE("len 4294967286 type", (x[-1] & SDS_TYPE_MASK) >= SDS_TYPE_32);
+        TEST_ASSERT_MESSAGE("len 4294967286 sdsAllocSize", sdsAllocSize(x) == s_malloc_size(sdsAllocPtr(x)));
+        sdsfree(x);
+
+        x = sdsnewlen(NULL, 4294967287);
+        TEST_ASSERT_MESSAGE("len 4294967287 type", (x[-1] & SDS_TYPE_MASK) == SDS_TYPE_64);
+        TEST_ASSERT_MESSAGE("len 4294967287 sdsAllocSize", sdsAllocSize(x) == s_malloc_size(sdsAllocPtr(x)));
+        sdsfree(x);
+    }
+#endif
+
+    return 0;
+}
+
+/* The test verifies that we can adjust SDS types if an allocator returned
+ * larger buffer. The maximum length for type SDS_TYPE_X is
+ * 2^X - header_size(SDS_TYPE_X) - 1. The maximum value to be stored in alloc
+ * field is 2^X - 1. When allocated buffer is larger than
+ * 2^X + header_size(SDS_TYPE_X), we "move" to a larger type SDS_TYPE_Y. To be
+ * sure SDS_TYPE_Y header fits into 2^X + header_size(SDS_TYPE_X) + 1 bytes, the
+ * difference between header sizes must be smaller than
+ * header_size(SDS_TYPE_X) + 1.
+ * We ignore SDS_TYPE_5 as it doesn't have alloc field. */
+int test_sdsHeaderSizes(int argc, char **argv, int flags) {
+    UNUSED(argc);
+    UNUSED(argv);
+    UNUSED(flags);
+
+    TEST_ASSERT_MESSAGE("can't always adjust SDS_TYPE_8 with SDS_TYPE_16",
+                        sizeof(struct sdshdr16) <= 2 * sizeof(struct sdshdr8) + 1);
+    TEST_ASSERT_MESSAGE("can't always adjust SDS_TYPE_16 with SDS_TYPE_32",
+                        sizeof(struct sdshdr32) <= 2 * sizeof(struct sdshdr16) + 1);
+#if (LONG_MAX == LLONG_MAX)
+    TEST_ASSERT_MESSAGE("can't always adjust SDS_TYPE_32 with SDS_TYPE_64",
+                        sizeof(struct sdshdr64) <= 2 * sizeof(struct sdshdr32) + 1);
+#endif
+
     return 0;
 }