Correct the actual allocated size from allocator when call sdsRedize …

…to align the logic with sdsnewlen function. (#476) This patch try to correct the actual allocated size from allocator when call sdsRedize to align the logic with sdsnewlen function. Maybe the #453 optimization should depend on this. Signed-off-by: Lipeng Zhu <[email protected]>
valkey-io · May 16, 2024 · 7a9951f · 7a9951f
1 parent 3de5c71
commit 7a9951f
Show file tree

Hide file tree

Showing 3 changed files with 38 additions and 26 deletions.
diff --git a/src/sds.c b/src/sds.c
@@ -349,6 +349,7 @@ 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;
 
     if (use_realloc) {
         int alloc_already_optimal = 0;
@@ -357,24 +358,27 @@ 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. */
-            alloc_already_optimal = (je_nallocx(newlen, 0) == zmalloc_size(sh));
+            newsize = zmalloc_size(sh);
+            alloc_already_optimal = (je_nallocx(newlen, 0) == newsize);
         #endif
         if (!alloc_already_optimal) {
-            newsh = s_realloc(sh, newlen);
+            newsh = s_realloc_usable(sh, newlen, &newsize);
             if (newsh == NULL) return NULL;
             s = (char*)newsh+oldhdrlen;
+            newsize -= (oldhdrlen + 1);
         }
     } else {
-        newsh = s_malloc(newlen);
+        newsh = s_malloc_usable(newlen, &newsize);
         if (newsh == NULL) return NULL;
         memcpy((char*)newsh+hdrlen, s, len);
         s_free(sh);
         s = (char*)newsh+hdrlen;
         s[-1] = type;
+        newsize -= (hdrlen + 1);
     }
-    s[len] = 0;
+    s[len] = '\0';
     sdssetlen(s, len);
-    sdssetalloc(s, size);
+    sdssetalloc(s, newsize);
     return s;
 }
 

diff --git a/src/unit/test_sds.c b/src/unit/test_sds.c
@@ -227,32 +227,40 @@ int test_sds(int argc, char **argv, int flags) {
                 memcmp(x, "v1={value1} {} v2=value2", 24) == 0);
     sdsfree(x);
 
-    /* Test sdsresize - extend */
+    /* Test sdsResize - extend */
     x = sdsnew("1234567890123456789012345678901234567890");
     x = sdsResize(x, 200, 1);
-    TEST_ASSERT_MESSAGE("sdsrezie() expand len", sdslen(x) == 40);
-    TEST_ASSERT_MESSAGE("sdsrezie() expand strlen", strlen(x) == 40);
-    TEST_ASSERT_MESSAGE("sdsrezie() expand alloc", sdsalloc(x) == 200);
-    /* Test sdsresize - trim free space */
+    TEST_ASSERT_MESSAGE("sdsReszie() expand type", x[-1] == SDS_TYPE_8);
+    TEST_ASSERT_MESSAGE("sdsReszie() expand len", sdslen(x) == 40);
+    TEST_ASSERT_MESSAGE("sdsReszie() expand strlen", strlen(x) == 40);
+    /* Different allocator allocates at least as large as requested size,
+     * to confirm the allocator won't waste too much,
+     * we add a largest size checker here. */
+    TEST_ASSERT_MESSAGE("sdsReszie() expand alloc", sdsalloc(x) >= 200 && sdsalloc(x) < 400);
+    /* Test sdsResize - trim free space */
     x = sdsResize(x, 80, 1);
-    TEST_ASSERT_MESSAGE("sdsrezie() shrink len", sdslen(x) == 40);
-    TEST_ASSERT_MESSAGE("sdsrezie() shrink strlen", strlen(x) == 40);
-    TEST_ASSERT_MESSAGE("sdsrezie() shrink alloc", sdsalloc(x) == 80);
-    /* Test sdsresize - crop used space */
+    TEST_ASSERT_MESSAGE("sdsReszie() shrink type", x[-1] == SDS_TYPE_8);
+    TEST_ASSERT_MESSAGE("sdsReszie() shrink len", sdslen(x) == 40);
+    TEST_ASSERT_MESSAGE("sdsReszie() shrink strlen", strlen(x) == 40);
+    TEST_ASSERT_MESSAGE("sdsReszie() shrink alloc", sdsalloc(x) >= 80);
+    /* Test sdsResize - crop used space */
     x = sdsResize(x, 30, 1);
-    TEST_ASSERT_MESSAGE("sdsrezie() crop len", sdslen(x) == 30);
-    TEST_ASSERT_MESSAGE("sdsrezie() crop strlen", strlen(x) == 30);
-    TEST_ASSERT_MESSAGE("sdsrezie() crop alloc", sdsalloc(x) == 30);
-    /* Test sdsresize - extend to different class */
+    TEST_ASSERT_MESSAGE("sdsReszie() crop type", x[-1] == SDS_TYPE_8);
+    TEST_ASSERT_MESSAGE("sdsReszie() crop len", sdslen(x) == 30);
+    TEST_ASSERT_MESSAGE("sdsReszie() crop strlen", strlen(x) == 30);
+    TEST_ASSERT_MESSAGE("sdsReszie() crop alloc", sdsalloc(x) >= 30);
+    /* Test sdsResize - extend to different class */
     x = sdsResize(x, 400, 1);
-    TEST_ASSERT_MESSAGE("sdsrezie() expand len", sdslen(x) == 30);
-    TEST_ASSERT_MESSAGE("sdsrezie() expand strlen", strlen(x) == 30);
-    TEST_ASSERT_MESSAGE("sdsrezie() expand alloc", sdsalloc(x) == 400);
-    /* Test sdsresize - shrink to different class */
+    TEST_ASSERT_MESSAGE("sdsReszie() expand type", x[-1] == SDS_TYPE_16);
+    TEST_ASSERT_MESSAGE("sdsReszie() expand len", sdslen(x) == 30);
+    TEST_ASSERT_MESSAGE("sdsReszie() expand strlen", strlen(x) == 30);
+    TEST_ASSERT_MESSAGE("sdsReszie() expand alloc", sdsalloc(x) >= 400);
+    /* Test sdsResize - shrink to different class */
     x = sdsResize(x, 4, 1);
-    TEST_ASSERT_MESSAGE("sdsrezie() crop len", sdslen(x) == 4);
-    TEST_ASSERT_MESSAGE("sdsrezie() crop strlen", strlen(x) == 4);
-    TEST_ASSERT_MESSAGE("sdsrezie() crop alloc", sdsalloc(x) == 4);
+    TEST_ASSERT_MESSAGE("sdsReszie() crop type", x[-1] == SDS_TYPE_8);
+    TEST_ASSERT_MESSAGE("sdsReszie() crop len", sdslen(x) == 4);
+    TEST_ASSERT_MESSAGE("sdsReszie() crop strlen", strlen(x) == 4);
+    TEST_ASSERT_MESSAGE("sdsReszie() crop alloc", sdsalloc(x) >= 4);
     sdsfree(x);
     return 0;
 }
diff --git a/tests/unit/querybuf.tcl b/tests/unit/querybuf.tcl
@@ -33,7 +33,7 @@ start_server {tags {"querybuf slow"}} {
 
         # Check that the initial query buffer is resized after 2 sec
         wait_for_condition 1000 10 {
-            [client_idle_sec test_client] >= 3 && [client_query_buffer test_client] == 0
+            [client_idle_sec test_client] >= 3 && [client_query_buffer test_client] < $orig_test_client_qbuf
         } else {
             fail "query buffer was not resized"
         }