New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Quicklist (linked list + ziplist) #2143

Merged
merged 29 commits into from Jan 8, 2015

Conversation

Projects
None yet
6 participants
@mattsta
Contributor

mattsta commented Nov 18, 2014

What

My implementation of what Twitter has previously described as ziplists in a linked list for better memory efficiency.

Longer writeup with improvement graphs at https://matt.sh/redis-quicklist

How

Redis currently uses two list encodings. This replace both. The code in this PR/branch removes the two current list encodings of REDIS_ENCODING_ZIPLIST and REDIS_ENCODING_LINKEDLIST and replaces them with REDIS_ENCODING_QUICKLIST.

The only parameter for the new list type is the existing list-max-ziplist-entries field which tells Redis how many elements to allow per ziplist node before creating a new node. The other param of list-max-ziplist-value is now deleted.

The RDB format remains the same. A quicklist is written to the RDB as a linked list, so the RDB is still usable by prior Redis versions. The existing RDB format for saved ziplists and linkedlists both get converted to quicklists when the RDB is loaded. Quicklists are saved as RDB linkedlist types so on reload, Redis will re-create a new compact quicklist with full interior ziplists even if the saved quicklist had not-full interior ziplists.

Tests

All test pass for me. You should try to break it too. There is one spurious timing error in Redis tests (waiting too long for RDB loading), but I don't think it's new. (?)

The quicklist code also includes ~1,000 lines of internal tests.

I added a new compile parameter of "REDIS_TEST" — if you compile with -DREDIS_TEST you can run Redis code tests with redis-server test <module>. So, to test quicklist with code-level tests, compile with that define and run redis-server test quicklist. Also supported: ziplist, util, intset, and more.

Extras

Also includes DEBUG JEMALLOC INFO to get internal jemalloc stats.

Figuring out what any of that means is left as an exercise for the reader.

Future Work

This approach could be applied to some other areas too. Redis currently converts hashes from zipmaps to full hash tables when they get too big, but we could make a hash table of zipmaps to save a lot of space too.

@badboy

This comment has been minimized.

Show comment
Hide comment
@badboy

badboy Nov 18, 2014

Contributor

Sir, this is one hell of a Pull Request. I will definitely read the code and test it tomorrow while flying 35000ft above the ground.

respect

Contributor

badboy commented Nov 18, 2014

Sir, this is one hell of a Pull Request. I will definitely read the code and test it tomorrow while flying 35000ft above the ground.

respect

Show outdated Hide outdated src/quicklist.c
Show outdated Hide outdated src/quicklist.c
Show outdated Hide outdated src/quicklist.c
initEntry(entry);
entry->quicklist = quicklist;
if (!forward) {

This comment has been minimized.

@badboy

badboy Nov 20, 2014

Contributor

Why negate here? For consistency (as I think it's easier to read and done further down anyway) I would swap the if/else conditions.

if (forward) { 
        index = idx;
        n = quicklist->head;
} else {
        index = (-idx) - 1;
        n = quicklist->tail;
}
@badboy

badboy Nov 20, 2014

Contributor

Why negate here? For consistency (as I think it's easier to read and done further down anyway) I would swap the if/else conditions.

if (forward) { 
        index = idx;
        n = quicklist->head;
} else {
        index = (-idx) - 1;
        n = quicklist->tail;
}

This comment has been minimized.

@mattsta

mattsta Nov 20, 2014

Contributor

I think it was inspired by https://github.com/antirez/redis/blob/unstable/src/ziplist.c#L680-L681

For readability, showing the first special case of "negate index, subtract one" helps the reader more quickly see something strange is happening since it's the first case.

@mattsta

mattsta Nov 20, 2014

Contributor

I think it was inspired by https://github.com/antirez/redis/blob/unstable/src/ziplist.c#L680-L681

For readability, showing the first special case of "negate index, subtract one" helps the reader more quickly see something strange is happening since it's the first case.

Show outdated Hide outdated src/quicklist.c
Show outdated Hide outdated src/quicklist.c
Show outdated Hide outdated src/quicklist.c
@badboy

This comment has been minimized.

Show comment
Hide comment
@badboy

badboy Nov 20, 2014

Contributor

Impressive work. I read the code all in once and it was quite clear and concise. I saw no immediate bugs. Also great you have extensive test code in there (nearly half the new code is tests).

I'm sure we can get this into unstable soon. ✈️

Contributor

badboy commented Nov 20, 2014

Impressive work. I read the code all in once and it was quite clear and concise. I saw no immediate bugs. Also great you have extensive test code in there (nearly half the new code is tests).

I'm sure we can get this into unstable soon. ✈️

@mattsta

This comment has been minimized.

Show comment
Hide comment
@mattsta

mattsta Nov 20, 2014

Contributor

Thanks for the review! I incorporated your fixes (plus a few more cleanups) into the existing commit. The diff is below since it's difficult to see changes between rebased forced pushes.

I don't see any warnings against clang or gcc-4.9.2 anymore, but feel free to try more compilers and tests. The more problems we find now the fewer cleanup commits we have to apply later. :)

matt@ununoctium:~/repos/redis/src% git diff
diff --git a/src/quicklist.c b/src/quicklist.c
index bb29121..081296d 100644
--- a/src/quicklist.c
+++ b/src/quicklist.c
@@ -28,12 +28,15 @@
  * POSSIBILITY OF SUCH DAMAGE.
  */

-#include <stdlib.h>
-#include <stdio.h>  /* for snprintf */
 #include <string.h> /* for memcpy */
 #include "quicklist.h"
 #include "zmalloc.h"
 #include "ziplist.h"
+#include "util.h" /* for ll2string */
+
+#if defined(REDIS_TEST) || defined(REDIS_TEST_VERBOSE)
+#include <stdio.h> /* for printf (debug printing), snprintf (genstr) */
+#endif

 /* If not verbose testing, remove all debug printing. */
 #ifndef REDIS_TEST_VERBOSE
@@ -323,15 +326,14 @@ static quicklistNode *_quicklistZiplistMerge(quicklist *quicklist,
       target->count);

     int where;
-
     unsigned char *p = NULL;
     if (target == a) {
         /* If target is node a, we append node b to node a, in-order */
         where = ZIPLIST_TAIL;
         p = ziplistIndex(b->zl, 0);
         D("WILL TRAVERSE B WITH LENGTH: %u, %u", b->count, ziplistLen(b->zl));
-    } else if (target == b) {
-        /* If target b, we pre-pend node a to node b, in reverse order of a */
+    } else {
+        /* If target b, we prepend node a to node b, in reverse order of a */
         where = ZIPLIST_HEAD;
         p = ziplistIndex(a->zl, -1);
         D("WILL TRAVERSE A WITH LENGTH: %u, %u", a->count, ziplistLen(a->zl));
@@ -347,7 +349,7 @@ static quicklistNode *_quicklistZiplistMerge(quicklist *quicklist,
      * complex than using the existing ziplist API to read/push as below. */
     while (ziplistGet(p, &val, &sz, &longval)) {
         if (!val) {
-            sz = snprintf(lv, sizeof(lv), "%lld", longval);
+            sz = ll2string(lv, sizeof(lv), longval);
             val = (unsigned char *)lv;
         }
         target->zl = ziplistPush(target->zl, val, sz, where);
@@ -747,7 +749,7 @@ int quicklistNext(quicklistIter *iter, quicklistEntry *entry) {
         return 0;
     }

-    unsigned char *(*nextFn)(unsigned char *, unsigned char*) = NULL;
+    unsigned char *(*nextFn)(unsigned char *, unsigned char *) = NULL;
     int offset_update = 0;

     if (!iter->zi) {
@@ -842,7 +844,7 @@ int quicklistIndex(const quicklist *quicklist, const long long idx,
     quicklistNode *n;
     unsigned long long accum = 0;
     unsigned long long index;
-    int forward = idx < 0 ? 0 : 1; /* < 0 -> reverse, positive -> forward */
+    int forward = idx < 0 ? 0 : 1; /* < 0 -> reverse, 0+ -> forward */

     initEntry(entry);
     entry->quicklist = quicklist;
@@ -862,7 +864,8 @@ int quicklistIndex(const quicklist *quicklist, const long long idx,
         if ((accum + n->count) > index) {
             break;
         } else {
-            D("Skipping over (%p) %u at accum %lld", n, n->count, accum);
+            D("Skipping over (%p) %u at accum %lld", (void *)n, n->count,
+              accum);
             accum += n->count;
             n = forward ? n->next : n->prev;
         }
@@ -871,8 +874,8 @@ int quicklistIndex(const quicklist *quicklist, const long long idx,
     if (!n)
         return 0;

-    D("Found node: %p at accum %llu, idx %llu, sub+ %llu, sub- %llu", n, accum,
-      index, index - accum, (-index) - 1 + accum);
+    D("Found node: %p at accum %llu, idx %llu, sub+ %llu, sub- %llu", (void *)n,
+      accum, index, index - accum, (-index) - 1 + accum);

     entry->node = n;
     if (forward) {
@@ -906,7 +909,7 @@ void quicklistRotate(quicklist *quicklist, const size_t fill) {
     /* If value found is NULL, then ziplistGet populated longval instead */
     if (!value) {
         /* Write the longval as a string so we can re-add it */
-        int wrote = snprintf(longstr, sizeof(longstr), "%lld", longval);
+        int wrote = ll2string(longstr, sizeof(longstr), longval);
         value = (unsigned char *)longval;
         sz = wrote;
     }
@@ -1017,15 +1020,16 @@ void quicklistPush(quicklist *quicklist, const size_t fill, void *value,

 /* The rest of this file is test cases and test helpers. */
 #ifdef REDIS_TEST
-#include <stdio.h>
 #include <stdint.h>

 #define assert(_e)                                                             \
-    ((_e) ? (void)0 : (_assert(#_e, __FILE__, __LINE__), exit(1)))
-static void _assert(char *estr, char *file, int line) {
-    printf("\n\n=== ASSERTION FAILED ===\n");
-    printf("==> %s:%d '%s' is not true\n", file, line, estr);
-}
+    do {                                                                       \
+        if (!(_e)) {                                                           \
+            printf("\n\n=== ASSERTION FAILED ===\n");                          \
+            printf("==> %s:%d '%s' is not true\n", __FILE__, __LINE__, #_e);   \
+            err++;                                                             \
+        }                                                                      \
+    } while (0)

 #define yell(str, ...) printf("ERROR! " str "\n\n", __VA_ARGS__)

@@ -1718,7 +1722,7 @@ int quicklistTest(int argc, char *argv[]) {
         long long nums[5000];
         for (int i = 0; i < 5000; i++) {
             nums[i] = -5157318210846258176 + i;
-            int sz = snprintf(num, sizeof(num), "%lld", nums[i]);
+            int sz = ll2string(num, sizeof(num), nums[i]);
             quicklistPushTail(ql, F, num, sz);
         }
         quicklistPushTail(ql, F, "xxxxxxxxxxxxxxxxxxxx", 20);
@@ -1876,7 +1880,7 @@ int quicklistTest(int argc, char *argv[]) {
             long long nums[5000];
             for (int i = 0; i < 760; i++) {
                 nums[i] = -5157318210846258176 + i;
-                int sz = snprintf(num, sizeof(num), "%lld", nums[i]);
+                int sz = ll2string(num, sizeof(num), nums[i]);
                 quicklistPushTail(ql, f, num, sz);
             }

@@ -1901,7 +1905,7 @@ int quicklistTest(int argc, char *argv[]) {
             long long nums[5000];
             for (int i = 0; i < 32; i++) {
                 nums[i] = -5157318210846258176 + i;
-                int sz = snprintf(num, sizeof(num), "%lld", nums[i]);
+                int sz = ll2string(num, sizeof(num), nums[i]);
                 quicklistPushTail(ql, f, num, sz);
             }
             if (f == 32)
@@ -1929,7 +1933,7 @@ int quicklistTest(int argc, char *argv[]) {
             long long nums[5000];
             for (int i = 0; i < 33; i++) {
                 nums[i] = i;
-                int sz = snprintf(num, sizeof(num), "%lld", nums[i]);
+                int sz = ll2string(num, sizeof(num), nums[i]);
                 quicklistPushTail(ql, f, num, sz);
             }
             if (f == 32)
@@ -1973,7 +1977,7 @@ int quicklistTest(int argc, char *argv[]) {
             long long nums[5000];
             for (int i = 0; i < 33; i++) {
                 nums[i] = -5157318210846258176 + i;
-                int sz = snprintf(num, sizeof(num), "%lld", nums[i]);
+                int sz = ll2string(num, sizeof(num), nums[i]);
                 quicklistPushTail(ql, f, num, sz);
             }
             if (f == 32)
@@ -1999,7 +2003,7 @@ int quicklistTest(int argc, char *argv[]) {
             long long nums[5000];
             for (int i = 0; i < 33; i++) {
                 nums[i] = -5157318210846258176 + i;
-                int sz = snprintf(num, sizeof(num), "%lld", nums[i]);
+                int sz = ll2string(num, sizeof(num), nums[i]);
                 quicklistPushTail(ql, f, num, sz);
             }
             if (f == 32)
Contributor

mattsta commented Nov 20, 2014

Thanks for the review! I incorporated your fixes (plus a few more cleanups) into the existing commit. The diff is below since it's difficult to see changes between rebased forced pushes.

I don't see any warnings against clang or gcc-4.9.2 anymore, but feel free to try more compilers and tests. The more problems we find now the fewer cleanup commits we have to apply later. :)

matt@ununoctium:~/repos/redis/src% git diff
diff --git a/src/quicklist.c b/src/quicklist.c
index bb29121..081296d 100644
--- a/src/quicklist.c
+++ b/src/quicklist.c
@@ -28,12 +28,15 @@
  * POSSIBILITY OF SUCH DAMAGE.
  */

-#include <stdlib.h>
-#include <stdio.h>  /* for snprintf */
 #include <string.h> /* for memcpy */
 #include "quicklist.h"
 #include "zmalloc.h"
 #include "ziplist.h"
+#include "util.h" /* for ll2string */
+
+#if defined(REDIS_TEST) || defined(REDIS_TEST_VERBOSE)
+#include <stdio.h> /* for printf (debug printing), snprintf (genstr) */
+#endif

 /* If not verbose testing, remove all debug printing. */
 #ifndef REDIS_TEST_VERBOSE
@@ -323,15 +326,14 @@ static quicklistNode *_quicklistZiplistMerge(quicklist *quicklist,
       target->count);

     int where;
-
     unsigned char *p = NULL;
     if (target == a) {
         /* If target is node a, we append node b to node a, in-order */
         where = ZIPLIST_TAIL;
         p = ziplistIndex(b->zl, 0);
         D("WILL TRAVERSE B WITH LENGTH: %u, %u", b->count, ziplistLen(b->zl));
-    } else if (target == b) {
-        /* If target b, we pre-pend node a to node b, in reverse order of a */
+    } else {
+        /* If target b, we prepend node a to node b, in reverse order of a */
         where = ZIPLIST_HEAD;
         p = ziplistIndex(a->zl, -1);
         D("WILL TRAVERSE A WITH LENGTH: %u, %u", a->count, ziplistLen(a->zl));
@@ -347,7 +349,7 @@ static quicklistNode *_quicklistZiplistMerge(quicklist *quicklist,
      * complex than using the existing ziplist API to read/push as below. */
     while (ziplistGet(p, &val, &sz, &longval)) {
         if (!val) {
-            sz = snprintf(lv, sizeof(lv), "%lld", longval);
+            sz = ll2string(lv, sizeof(lv), longval);
             val = (unsigned char *)lv;
         }
         target->zl = ziplistPush(target->zl, val, sz, where);
@@ -747,7 +749,7 @@ int quicklistNext(quicklistIter *iter, quicklistEntry *entry) {
         return 0;
     }

-    unsigned char *(*nextFn)(unsigned char *, unsigned char*) = NULL;
+    unsigned char *(*nextFn)(unsigned char *, unsigned char *) = NULL;
     int offset_update = 0;

     if (!iter->zi) {
@@ -842,7 +844,7 @@ int quicklistIndex(const quicklist *quicklist, const long long idx,
     quicklistNode *n;
     unsigned long long accum = 0;
     unsigned long long index;
-    int forward = idx < 0 ? 0 : 1; /* < 0 -> reverse, positive -> forward */
+    int forward = idx < 0 ? 0 : 1; /* < 0 -> reverse, 0+ -> forward */

     initEntry(entry);
     entry->quicklist = quicklist;
@@ -862,7 +864,8 @@ int quicklistIndex(const quicklist *quicklist, const long long idx,
         if ((accum + n->count) > index) {
             break;
         } else {
-            D("Skipping over (%p) %u at accum %lld", n, n->count, accum);
+            D("Skipping over (%p) %u at accum %lld", (void *)n, n->count,
+              accum);
             accum += n->count;
             n = forward ? n->next : n->prev;
         }
@@ -871,8 +874,8 @@ int quicklistIndex(const quicklist *quicklist, const long long idx,
     if (!n)
         return 0;

-    D("Found node: %p at accum %llu, idx %llu, sub+ %llu, sub- %llu", n, accum,
-      index, index - accum, (-index) - 1 + accum);
+    D("Found node: %p at accum %llu, idx %llu, sub+ %llu, sub- %llu", (void *)n,
+      accum, index, index - accum, (-index) - 1 + accum);

     entry->node = n;
     if (forward) {
@@ -906,7 +909,7 @@ void quicklistRotate(quicklist *quicklist, const size_t fill) {
     /* If value found is NULL, then ziplistGet populated longval instead */
     if (!value) {
         /* Write the longval as a string so we can re-add it */
-        int wrote = snprintf(longstr, sizeof(longstr), "%lld", longval);
+        int wrote = ll2string(longstr, sizeof(longstr), longval);
         value = (unsigned char *)longval;
         sz = wrote;
     }
@@ -1017,15 +1020,16 @@ void quicklistPush(quicklist *quicklist, const size_t fill, void *value,

 /* The rest of this file is test cases and test helpers. */
 #ifdef REDIS_TEST
-#include <stdio.h>
 #include <stdint.h>

 #define assert(_e)                                                             \
-    ((_e) ? (void)0 : (_assert(#_e, __FILE__, __LINE__), exit(1)))
-static void _assert(char *estr, char *file, int line) {
-    printf("\n\n=== ASSERTION FAILED ===\n");
-    printf("==> %s:%d '%s' is not true\n", file, line, estr);
-}
+    do {                                                                       \
+        if (!(_e)) {                                                           \
+            printf("\n\n=== ASSERTION FAILED ===\n");                          \
+            printf("==> %s:%d '%s' is not true\n", __FILE__, __LINE__, #_e);   \
+            err++;                                                             \
+        }                                                                      \
+    } while (0)

 #define yell(str, ...) printf("ERROR! " str "\n\n", __VA_ARGS__)

@@ -1718,7 +1722,7 @@ int quicklistTest(int argc, char *argv[]) {
         long long nums[5000];
         for (int i = 0; i < 5000; i++) {
             nums[i] = -5157318210846258176 + i;
-            int sz = snprintf(num, sizeof(num), "%lld", nums[i]);
+            int sz = ll2string(num, sizeof(num), nums[i]);
             quicklistPushTail(ql, F, num, sz);
         }
         quicklistPushTail(ql, F, "xxxxxxxxxxxxxxxxxxxx", 20);
@@ -1876,7 +1880,7 @@ int quicklistTest(int argc, char *argv[]) {
             long long nums[5000];
             for (int i = 0; i < 760; i++) {
                 nums[i] = -5157318210846258176 + i;
-                int sz = snprintf(num, sizeof(num), "%lld", nums[i]);
+                int sz = ll2string(num, sizeof(num), nums[i]);
                 quicklistPushTail(ql, f, num, sz);
             }

@@ -1901,7 +1905,7 @@ int quicklistTest(int argc, char *argv[]) {
             long long nums[5000];
             for (int i = 0; i < 32; i++) {
                 nums[i] = -5157318210846258176 + i;
-                int sz = snprintf(num, sizeof(num), "%lld", nums[i]);
+                int sz = ll2string(num, sizeof(num), nums[i]);
                 quicklistPushTail(ql, f, num, sz);
             }
             if (f == 32)
@@ -1929,7 +1933,7 @@ int quicklistTest(int argc, char *argv[]) {
             long long nums[5000];
             for (int i = 0; i < 33; i++) {
                 nums[i] = i;
-                int sz = snprintf(num, sizeof(num), "%lld", nums[i]);
+                int sz = ll2string(num, sizeof(num), nums[i]);
                 quicklistPushTail(ql, f, num, sz);
             }
             if (f == 32)
@@ -1973,7 +1977,7 @@ int quicklistTest(int argc, char *argv[]) {
             long long nums[5000];
             for (int i = 0; i < 33; i++) {
                 nums[i] = -5157318210846258176 + i;
-                int sz = snprintf(num, sizeof(num), "%lld", nums[i]);
+                int sz = ll2string(num, sizeof(num), nums[i]);
                 quicklistPushTail(ql, f, num, sz);
             }
             if (f == 32)
@@ -1999,7 +2003,7 @@ int quicklistTest(int argc, char *argv[]) {
             long long nums[5000];
             for (int i = 0; i < 33; i++) {
                 nums[i] = -5157318210846258176 + i;
-                int sz = snprintf(num, sizeof(num), "%lld", nums[i]);
+                int sz = ll2string(num, sizeof(num), nums[i]);
                 quicklistPushTail(ql, f, num, sz);
             }
             if (f == 32)
Show outdated Hide outdated src/quicklist.c
Show outdated Hide outdated src/quicklist.c
@mattsta

This comment has been minimized.

Show comment
Hide comment
@mattsta

mattsta Nov 22, 2014

Contributor

Updated with below diff thanks to sunheehnus at #2143 (comment)!

The fix also caught an invalid check in a test case. Now that we're merging quicklist nodes more properly, we use fewer nodes in a test (26 -> 25) and that's a good thing.

diff --git a/src/quicklist.c b/src/quicklist.c
index dbb2dae..c27827b 100644
--- a/src/quicklist.c
+++ b/src/quicklist.c
@@ -380,6 +380,9 @@ static void _quicklistMergeNodes(quicklist *quicklist, const size_t fill,
     if (center->prev && (center->count + center->prev->count) <= fill) {
         target = _quicklistZiplistMerge(quicklist, center->prev, center);
         center = NULL; /* center could have been deleted, invalidate it. */
+    } else {
+        /* If can't merge here, target still needs to be valid for below. */
+        target = center;
     }

     /* Use result of center merge to try and merge result with next node. */
@@ -1467,7 +1470,7 @@ int quicklistTest(int argc, char *argv[]) {
                 quicklistInsertBefore(ql, f, &entry, genstr("abc", i), 32);
             }
             if (f == 32)
-                ql_verify(ql, 26, 750, 32, 20);
+                ql_verify(ql, 25, 750, 32, 20);
             quicklistRelease(ql);
         }
     }
Contributor

mattsta commented Nov 22, 2014

Updated with below diff thanks to sunheehnus at #2143 (comment)!

The fix also caught an invalid check in a test case. Now that we're merging quicklist nodes more properly, we use fewer nodes in a test (26 -> 25) and that's a good thing.

diff --git a/src/quicklist.c b/src/quicklist.c
index dbb2dae..c27827b 100644
--- a/src/quicklist.c
+++ b/src/quicklist.c
@@ -380,6 +380,9 @@ static void _quicklistMergeNodes(quicklist *quicklist, const size_t fill,
     if (center->prev && (center->count + center->prev->count) <= fill) {
         target = _quicklistZiplistMerge(quicklist, center->prev, center);
         center = NULL; /* center could have been deleted, invalidate it. */
+    } else {
+        /* If can't merge here, target still needs to be valid for below. */
+        target = center;
     }

     /* Use result of center merge to try and merge result with next node. */
@@ -1467,7 +1470,7 @@ int quicklistTest(int argc, char *argv[]) {
                 quicklistInsertBefore(ql, f, &entry, genstr("abc", i), 32);
             }
             if (f == 32)
-                ql_verify(ql, 26, 750, 32, 20);
+                ql_verify(ql, 25, 750, 32, 20);
             quicklistRelease(ql);
         }
     }
@antirez

This comment has been minimized.

Show comment
Hide comment
@antirez

antirez Nov 23, 2014

Owner

Love the feature and that there are even reviews here. Thank you. Tomorrow I'll be back in Sicily from Bracelona, and Monday I'll review the code as well.

Owner

antirez commented Nov 23, 2014

Love the feature and that there are even reviews here. Thank you. Tomorrow I'll be back in Sicily from Bracelona, and Monday I'll review the code as well.

Show outdated Hide outdated src/quicklist.c
Show outdated Hide outdated src/quicklist.c
Show outdated Hide outdated src/quicklist.c
@mattsta

This comment has been minimized.

Show comment
Hide comment
@mattsta

mattsta Nov 23, 2014

Contributor

Updated:

diff --git a/src/quicklist.c b/src/quicklist.c
index 664a6c6..7a2490c 100644
--- a/src/quicklist.c
+++ b/src/quicklist.c
@@ -393,10 +393,22 @@ static void _quicklistMergeNodes(quicklist *quicklist, const size_t fill,
     }
 }

-/* Split 'node' at 'offset' into two parts.
+/* Split 'node' into two parts, parameterized by 'offset' and 'after'.
+ *
+ * The 'after' argument controls which quicklistNode gets returned.
+ * If 'after'==1, returned node has elements after 'offset'.
+ *                input node keeps elements up to 'offset', including 'offset'.
+ * If 'after'==0, returned node has elements up to 'offset', including 'offset'.
+ *                input node keeps elements after 'offset'.
+ *
+ * If 'after'==1, returned node will have elements _after_ 'offset'.
+ *                The returned node will have elements [OFFSET+1, END].
+ *                The input node keeps elements [0, OFFSET].
+ *
+ * If 'after'==0, returned node will keep elements up to and including 'offset'.
+ *                The returned node will have elements [0, OFFSET].
+ *                The input node keeps elements [OFFSET+1, END].
  *
- * If after==1, then the returned node has elements [OFFSET, END].
- * Otherwise if after==0, then the new node has [0, OFFSET)
  * The input node keeps all elements not taken by the returned node.
  *
  * Returns newly created node or NULL if split not possible. */
@@ -409,8 +421,10 @@ static quicklistNode *_quicklistSplitNode(quicklistNode *node, int offset,
         return NULL;

     new_node->zl = zmalloc(zl_sz);
-    if (!new_node->zl)
+    if (!new_node->zl) {
+        zfree(new_node);
         return NULL;
+    }

     /* Copy original ziplist so we can split it */
     memcpy(new_node->zl, node->zl, zl_sz);

Thanks for the ongoing reviews!

Contributor

mattsta commented Nov 23, 2014

Updated:

diff --git a/src/quicklist.c b/src/quicklist.c
index 664a6c6..7a2490c 100644
--- a/src/quicklist.c
+++ b/src/quicklist.c
@@ -393,10 +393,22 @@ static void _quicklistMergeNodes(quicklist *quicklist, const size_t fill,
     }
 }

-/* Split 'node' at 'offset' into two parts.
+/* Split 'node' into two parts, parameterized by 'offset' and 'after'.
+ *
+ * The 'after' argument controls which quicklistNode gets returned.
+ * If 'after'==1, returned node has elements after 'offset'.
+ *                input node keeps elements up to 'offset', including 'offset'.
+ * If 'after'==0, returned node has elements up to 'offset', including 'offset'.
+ *                input node keeps elements after 'offset'.
+ *
+ * If 'after'==1, returned node will have elements _after_ 'offset'.
+ *                The returned node will have elements [OFFSET+1, END].
+ *                The input node keeps elements [0, OFFSET].
+ *
+ * If 'after'==0, returned node will keep elements up to and including 'offset'.
+ *                The returned node will have elements [0, OFFSET].
+ *                The input node keeps elements [OFFSET+1, END].
  *
- * If after==1, then the returned node has elements [OFFSET, END].
- * Otherwise if after==0, then the new node has [0, OFFSET)
  * The input node keeps all elements not taken by the returned node.
  *
  * Returns newly created node or NULL if split not possible. */
@@ -409,8 +421,10 @@ static quicklistNode *_quicklistSplitNode(quicklistNode *node, int offset,
         return NULL;

     new_node->zl = zmalloc(zl_sz);
-    if (!new_node->zl)
+    if (!new_node->zl) {
+        zfree(new_node);
         return NULL;
+    }

     /* Copy original ziplist so we can split it */
     memcpy(new_node->zl, node->zl, zl_sz);

Thanks for the ongoing reviews!

Show outdated Hide outdated src/quicklist.c
iter->offset = -1;
}
iter->zi = NULL;
return quicklistNext(iter, entry);

This comment has been minimized.

@sunheehnus

sunheehnus Nov 24, 2014

Contributor

We can just skip to the head to avoid tail recursion. :)

@sunheehnus

sunheehnus Nov 24, 2014

Contributor

We can just skip to the head to avoid tail recursion. :)

This comment has been minimized.

@mattsta

mattsta Nov 24, 2014

Contributor

We can just skip to the head

How?

to avoid tail recursion.

Why would we want to avoid?

@mattsta

mattsta Nov 24, 2014

Contributor

We can just skip to the head

How?

to avoid tail recursion.

Why would we want to avoid?

This comment has been minimized.

@sunheehnus

sunheehnus Nov 24, 2014

Contributor

How?

Add goto statement to the start of this quicklistNext function.

Why would we want to avoid?

It is just a little trick, decrease the depth of system stack, save the time system
pushes parameters on stack and allocate stack space to run this function again.

This function can at most be called one more again, this little trick doesn't save
much space & time. Of course at the same time goto is harmful.
I didn't think this special situation over. My bad. :)

@sunheehnus

sunheehnus Nov 24, 2014

Contributor

How?

Add goto statement to the start of this quicklistNext function.

Why would we want to avoid?

It is just a little trick, decrease the depth of system stack, save the time system
pushes parameters on stack and allocate stack space to run this function again.

This function can at most be called one more again, this little trick doesn't save
much space & time. Of course at the same time goto is harmful.
I didn't think this special situation over. My bad. :)

Show outdated Hide outdated src/quicklist.c
*
* Returns 1 if entries were deleted, 0 if nothing was deleted. */
int quicklistDelRange(quicklist *quicklist, const long start,
const long count) {

This comment has been minimized.

@sunheehnus

sunheehnus Nov 24, 2014

Contributor

We should limit extent within the rest of list.
And the 2nd if branch (line 601) else if (entry.offset >= 0 && extent > node->count)
should add '=' to match all the cases such as, entry.offset == 2 ; extent == node->count
should also run this branch.
See mattsta#5 :)

@sunheehnus

sunheehnus Nov 24, 2014

Contributor

We should limit extent within the rest of list.
And the 2nd if branch (line 601) else if (entry.offset >= 0 && extent > node->count)
should add '=' to match all the cases such as, entry.offset == 2 ; extent == node->count
should also run this branch.
See mattsta#5 :)

Show outdated Hide outdated src/quicklist.c
Show outdated Hide outdated src/quicklist.c
@sunheehnus

This comment has been minimized.

Show comment
Hide comment
@sunheehnus

sunheehnus Nov 24, 2014

Contributor

Finish my review.
You did impressive work. Learn a lot from you. :) 👍 👍 👍

Contributor

sunheehnus commented Nov 24, 2014

Finish my review.
You did impressive work. Learn a lot from you. :) 👍 👍 👍

@badboy

This comment has been minimized.

Show comment
Hide comment
@badboy

badboy Nov 24, 2014

Contributor

Wow, @sunheehnus, your work is great as well.

Contributor

badboy commented Nov 24, 2014

Wow, @sunheehnus, your work is great as well.

@antirez

This comment has been minimized.

Show comment
Hide comment
@antirez

antirez Nov 24, 2014

Owner

Thank you, after Matt changes the code according to the reviews I'll do mine as well... waiting because there is already too many good things ongoing here :-)

Owner

antirez commented Nov 24, 2014

Thank you, after Matt changes the code according to the reviews I'll do mine as well... waiting because there is already too many good things ongoing here :-)

mattsta added a commit to mattsta/redis that referenced this pull request Nov 24, 2014

Add quicklist implementation
This replaces individual ziplist vs. linkedlist representations
for Redis list operations.

Big thanks for all the reviews and feedback from everybody in
antirez#2143
@mattsta

This comment has been minimized.

Show comment
Hide comment
@mattsta

mattsta Nov 24, 2014

Contributor

Update: 3 files changed, 124 insertions(+), 43 deletions(-)

New changes:

  • Imported ziplists now get appended element-by-element instead of just added as a single block of elements (see quicklistAppendValuesFromZiplist() and wrapper for it quicklistCreateFromZiplist()).
  • Fixed incorrect iterator updating during a delete; turned that section of code inside out
  • Incorporated suggested fixes from mattsta#5
    • delete range fixes when given crazy arguments + tests for the crazy arguments
    • use proper string value instead of casting a long long to a char* incorrectly
    • during rotate, update ziplist index pointer during when appropriate
  • added tests to trigger some previously untraversed code paths (see the number pushes in test rotate 500 val 5000 — now triggers the longval segfault and the head-ziplist-moves-itself segfault (before they were fixed)).
  • improved test cases to accumulate errors when ql_verify fails instead of just printing a failure message in the middle of 10,000 line test output.
  • changed RDB ziplist import to use quicklistCreateFromZiplist()
diff --git a/src/quicklist.c b/src/quicklist.c
index 7a2490c..6d89927 100644
--- a/src/quicklist.c
+++ b/src/quicklist.c
@@ -194,18 +194,39 @@ quicklist *quicklistPushTail(quicklist *quicklist, const size_t fill,
     return quicklist;
 }

-/* Create new node consisting of an entire pre-formed ziplist.
- * Used for loading old RDBs where entire ziplists have been stored
- * to be restored later. */
-void quicklistPushTailZiplist(quicklist *quicklist, unsigned char *zl) {
-    unsigned int sz = ziplistLen(zl);
-
-    quicklistNode *node = quicklistCreateNode();
-    _quicklistInsertNodeAfter(quicklist, quicklist->tail, node);
-
-    node->zl = zl;
-    node->count = sz;
-    quicklist->count += sz;
+/* Append all values of ziplist 'zl' individually into 'quicklist'.
+ *
+ * This allows us to restore old RDB ziplists into new quicklists
+ * with smaller ziplist sizes than the saved RDB ziplist.
+ *
+ * Returns 'quicklist' argument. Frees passed-in ziplist 'zl' */
+quicklist *quicklistAppendValuesFromZiplist(quicklist *quicklist,
+                                            const size_t fill,
+                                            unsigned char *zl) {
+    unsigned char *value;
+    unsigned int sz;
+    long long longval;
+    char longstr[32] = { 0 };
+
+    unsigned char *p = ziplistIndex(zl, 0);
+    while (ziplistGet(p, &value, &sz, &longval)) {
+        if (!value) {
+            /* Write the longval as a string so we can re-add it */
+            sz = ll2string(longstr, sizeof(longstr), longval);
+            value = (unsigned char *)longstr;
+        }
+        quicklistPushTail(quicklist, fill, value, sz);
+        p = ziplistNext(zl, p);
+    }
+    zfree(zl);
+    return quicklist;
+}
+
+/* Create new (potentially multi-node) quicklist from a single existing ziplist.
+ *
+ * Returns new quicklist.  Frees passed-in ziplist 'zl'. */
+quicklist *quicklistCreateFromZiplist(size_t fill, unsigned char *zl) {
+    return quicklistAppendValuesFromZiplist(quicklistCreate(), fill, zl);
 }

 #define quicklistDeleteIfEmpty(ql, n)                                          \
@@ -265,27 +286,24 @@ void quicklistDelEntry(quicklistIter *iter, quicklistEntry *entry) {
     /* after delete, the zi is now invalid for any future usage. */
     iter->zi = NULL;

-    if (iter->direction == AL_START_HEAD) {
-        if (deleted_node) {
-            /* Current node was deleted.  Assign saved next to current. */
-            /* Also re-init zi to the first position in new current. */
+    /* If current node is deleted, we must update iterator node and offset. */
+    if (deleted_node) {
+        if (iter->direction == AL_START_HEAD) {
             iter->current = next;
             iter->offset = 0;
-        } else {
-            /* Current node remains.  Replace iterator zi with next zi. */
-            iter->zi = entry->zi;
-            iter->offset++;
-        }
-    } else if (iter->direction == AL_START_TAIL) {
-        if (deleted_node) {
-            /* Current node was deleted.  Assign saved prev to current. */
-            /* Also re-init zi to the last position in new current. */
+        } else if (iter->direction == AL_START_TAIL) {
             iter->current = prev;
             iter->offset = -1;
-        } else {
-            /* Current node still exists. */
         }
     }
+    /* else if (!deleted_node), no changes needed.
+     * we already reset iter->zi above, and the existing iter->offset
+     * doesn't move again because:
+     *   - [1, 2, 3] => delete offset 1 => [1, 3]: next element still offset 1
+     *   - [1, 2, 3] => delete offset 0 => [2, 3]: next element still offset 0
+     *  if we deleted the last element at offet N and now
+     *  length of this ziplist is N-1, the next call into
+     *  quicklistNext() will jump to the next node. */
 }

 /* Replace quicklist entry at offset 'index' by 'data' with length 'sz'.
@@ -573,8 +591,8 @@ int quicklistDelRange(quicklist *quicklist, const long start,

     if (start >= 0 && extent > (quicklist->count - start)) {
         /* if requesting delete more elements than exist, limit to list size. */
-        extent = quicklist->count;
-    } else if (start < 0 && extent > (quicklist->count - (-start) + 1)) {
+        extent = quicklist->count - start;
+    } else if (start < 0 && extent > (unsigned long)(-start)) {
         /* else, if at negative offset, limit max size to rest of list. */
         extent = -start; /* c.f. LREM -29 29; just delete until end. */
     }
@@ -598,7 +616,7 @@ int quicklistDelRange(quicklist *quicklist, const long start,
              * can just delete the entire node without ziplist math. */
             delete_entire_node = 1;
             del = node->count;
-        } else if (entry.offset >= 0 && extent > node->count) {
+        } else if (entry.offset >= 0 && extent >= node->count) {
             /* If deleting more nodes after this one, calculate delete based
              * on size of current node. */
             del = node->count - entry.offset;
@@ -890,14 +908,19 @@ void quicklistRotate(quicklist *quicklist, const size_t fill) {
     /* If value found is NULL, then ziplistGet populated longval instead */
     if (!value) {
         /* Write the longval as a string so we can re-add it */
-        int wrote = ll2string(longstr, sizeof(longstr), longval);
-        value = (unsigned char *)longval;
-        sz = wrote;
+        sz = ll2string(longstr, sizeof(longstr), longval);
+        value = (unsigned char *)longstr;
     }

     /* Add tail entry to head (must happen before tail is deleted). */
     quicklistPushHead(quicklist, fill, value, sz);

+    /* If quicklist has only one node, the head ziplist is also the
+     * tail ziplist and PushHead() could have reallocated our single ziplist,
+     * which would make our pre-existing 'p' unusable. */
+    if (quicklist->len == 1)
+        p = ziplistIndex(tail->zl, -1);
+
     /* Remove tail entry. */
     quicklistDelIndex(quicklist, tail, &p);
 }
@@ -1083,8 +1106,13 @@ static int itrprintr_rev(quicklist *ql, int print) {
     return _itrprintr(ql, print, 0);
 }

+#define ql_verify(a, b, c, d, e)                                               \
+    do {                                                                       \
+        err += _ql_verify((a), (b), (c), (d), (e));                            \
+    } while (0)
+
 /* Verify list metadata matches physical list contents. */
-static void ql_verify(quicklist *ql, uint32_t len, uint32_t count,
+static int _ql_verify(quicklist *ql, uint32_t len, uint32_t count,
                       uint32_t head_count, uint32_t tail_count) {
     int ok = 1;

@@ -1117,7 +1145,7 @@ static void ql_verify(quicklist *ql, uint32_t len, uint32_t count,

     if (ql->len == 0 && ok) {
         OK;
-        return;
+        return !ok;
     }

     if (head_count != ql->head->count &&
@@ -1137,6 +1165,7 @@ static void ql_verify(quicklist *ql, uint32_t len, uint32_t count,
     }
     if (ok)
         OK;
+    return !ok;
 }

 /* Generate new string concatenating integer i against string 'prefix' */
@@ -1150,7 +1179,7 @@ static char *genstr(char *prefix, int i) {
 #define F 32
 /* main test, but callable from other files */
 int quicklistTest(int argc, char *argv[]) {
-    int err = 0;
+    unsigned int err = 0;

     UNUSED(argc);
     UNUSED(argv);
@@ -1249,6 +1278,10 @@ int quicklistTest(int argc, char *argv[]) {
     for (size_t f = 0; f < 1024; f++) {
         TEST_DESC("rotate 500 val 5000 times at fill %lu", f) {
             quicklist *ql = quicklistCreate();
+            quicklistPushHead(ql, f, "900", 3);
+            quicklistPushHead(ql, f, "7000", 4);
+            quicklistPushHead(ql, f, "-1200", 5);
+            quicklistPushHead(ql, f, "42", 2);
             for (int i = 0; i < 500; i++)
                 quicklistPushHead(ql, f, genstr("hello", i), 32);
             ql_info(ql);
@@ -1256,8 +1289,12 @@ int quicklistTest(int argc, char *argv[]) {
                 ql_info(ql);
                 quicklistRotate(ql, f);
             }
-            if (f == 32)
-                ql_verify(ql, 16, 500, 28, 24);
+            if (f == 1)
+                ql_verify(ql, 504, 504, 1, 1);
+            else if (f == 2)
+                ql_verify(ql, 252, 504, 2, 2);
+            else if (f == 32)
+                ql_verify(ql, 16, 504, 32, 24);
             quicklistRelease(ql);
         }
     }
@@ -1622,6 +1659,15 @@ int quicklistTest(int argc, char *argv[]) {
         quicklistRelease(ql);
     }

+    TEST("delete range of entire node with overflow counts") {
+        quicklist *ql = quicklistCreate();
+        for (int i = 0; i < 32; i++)
+            quicklistPushHead(ql, F, genstr("hello", i), 32);
+        quicklistDelRange(ql, 0, 128);
+        ql_verify(ql, 0, 0, 0, 0);
+        quicklistRelease(ql);
+    }
+
     TEST("delete middle 100 of 500 list") {
         quicklist *ql = quicklistCreate();
         for (int i = 0; i < 500; i++)
@@ -1640,6 +1686,15 @@ int quicklistTest(int argc, char *argv[]) {
         quicklistRelease(ql);
     }

+    TEST("delete negative 1 from 500 list with overflow counts") {
+        quicklist *ql = quicklistCreate();
+        for (int i = 0; i < 500; i++)
+            quicklistPushTail(ql, F, genstr("hello", i + 1), 32);
+        quicklistDelRange(ql, -1, 128);
+        ql_verify(ql, 16, 499, 32, 19);
+        quicklistRelease(ql);
+    }
+
     TEST("delete negative 100 from 500 list") {
         quicklist *ql = quicklistCreate();
         for (int i = 0; i < 500; i++)
@@ -1997,6 +2052,30 @@ int quicklistTest(int argc, char *argv[]) {
         }
     }

+    for (size_t f = 0; f < 72; f++) {
+        TEST_DESC("create quicklist from ziplist at fill %lu", f) {
+            unsigned char *zl = ziplistNew();
+            long long nums[32];
+            char num[32];
+            for (int i = 0; i < 33; i++) {
+                nums[i] = -5157318210846258176 + i;
+                int sz = ll2string(num, sizeof(num), nums[i]);
+                zl = ziplistPush(zl, (unsigned char *)num, sz, ZIPLIST_TAIL);
+            }
+            for (int i = 0; i < 33; i++) {
+                zl = ziplistPush(zl, (unsigned char *)genstr("hello", i), 32,
+                                 ZIPLIST_TAIL);
+            }
+            quicklist *ql = quicklistCreateFromZiplist(f, zl);
+            if (f == 1)
+                ql_verify(ql, 66, 66, 1, 1);
+            else if (f == 32)
+                ql_verify(ql, 3, 66, 32, 2);
+            else if (f == 66)
+                ql_verify(ql, 1, 66, 66, 66);
+            quicklistRelease(ql);
+        }
+    }
     if (!err)
         printf("ALL TESTS PASSED!\n");
     else
diff --git a/src/t_list.c b/src/t_list.c
index a7201fd..e740524 100644
--- a/src/t_list.c
+++ b/src/t_list.c
@@ -185,11 +185,10 @@ void listTypeConvert(robj *subject, int enc) {
     redisAssertWithInfo(NULL,subject,subject->encoding==REDIS_ENCODING_ZIPLIST);

     if (enc == REDIS_ENCODING_QUICKLIST) {
-        quicklist *ql = quicklistCreate();
-        quicklistPushTailZiplist(ql, subject->ptr);
+        size_t zlen = server.list_max_ziplist_entries;

         subject->encoding = REDIS_ENCODING_QUICKLIST;
-        subject->ptr = ql;
+        subject->ptr = quicklistCreateFromZiplist(zlen, subject->ptr);
     } else {
         redisPanic("Unsupported list conversion");
     }
Contributor

mattsta commented Nov 24, 2014

Update: 3 files changed, 124 insertions(+), 43 deletions(-)

New changes:

  • Imported ziplists now get appended element-by-element instead of just added as a single block of elements (see quicklistAppendValuesFromZiplist() and wrapper for it quicklistCreateFromZiplist()).
  • Fixed incorrect iterator updating during a delete; turned that section of code inside out
  • Incorporated suggested fixes from mattsta#5
    • delete range fixes when given crazy arguments + tests for the crazy arguments
    • use proper string value instead of casting a long long to a char* incorrectly
    • during rotate, update ziplist index pointer during when appropriate
  • added tests to trigger some previously untraversed code paths (see the number pushes in test rotate 500 val 5000 — now triggers the longval segfault and the head-ziplist-moves-itself segfault (before they were fixed)).
  • improved test cases to accumulate errors when ql_verify fails instead of just printing a failure message in the middle of 10,000 line test output.
  • changed RDB ziplist import to use quicklistCreateFromZiplist()
diff --git a/src/quicklist.c b/src/quicklist.c
index 7a2490c..6d89927 100644
--- a/src/quicklist.c
+++ b/src/quicklist.c
@@ -194,18 +194,39 @@ quicklist *quicklistPushTail(quicklist *quicklist, const size_t fill,
     return quicklist;
 }

-/* Create new node consisting of an entire pre-formed ziplist.
- * Used for loading old RDBs where entire ziplists have been stored
- * to be restored later. */
-void quicklistPushTailZiplist(quicklist *quicklist, unsigned char *zl) {
-    unsigned int sz = ziplistLen(zl);
-
-    quicklistNode *node = quicklistCreateNode();
-    _quicklistInsertNodeAfter(quicklist, quicklist->tail, node);
-
-    node->zl = zl;
-    node->count = sz;
-    quicklist->count += sz;
+/* Append all values of ziplist 'zl' individually into 'quicklist'.
+ *
+ * This allows us to restore old RDB ziplists into new quicklists
+ * with smaller ziplist sizes than the saved RDB ziplist.
+ *
+ * Returns 'quicklist' argument. Frees passed-in ziplist 'zl' */
+quicklist *quicklistAppendValuesFromZiplist(quicklist *quicklist,
+                                            const size_t fill,
+                                            unsigned char *zl) {
+    unsigned char *value;
+    unsigned int sz;
+    long long longval;
+    char longstr[32] = { 0 };
+
+    unsigned char *p = ziplistIndex(zl, 0);
+    while (ziplistGet(p, &value, &sz, &longval)) {
+        if (!value) {
+            /* Write the longval as a string so we can re-add it */
+            sz = ll2string(longstr, sizeof(longstr), longval);
+            value = (unsigned char *)longstr;
+        }
+        quicklistPushTail(quicklist, fill, value, sz);
+        p = ziplistNext(zl, p);
+    }
+    zfree(zl);
+    return quicklist;
+}
+
+/* Create new (potentially multi-node) quicklist from a single existing ziplist.
+ *
+ * Returns new quicklist.  Frees passed-in ziplist 'zl'. */
+quicklist *quicklistCreateFromZiplist(size_t fill, unsigned char *zl) {
+    return quicklistAppendValuesFromZiplist(quicklistCreate(), fill, zl);
 }

 #define quicklistDeleteIfEmpty(ql, n)                                          \
@@ -265,27 +286,24 @@ void quicklistDelEntry(quicklistIter *iter, quicklistEntry *entry) {
     /* after delete, the zi is now invalid for any future usage. */
     iter->zi = NULL;

-    if (iter->direction == AL_START_HEAD) {
-        if (deleted_node) {
-            /* Current node was deleted.  Assign saved next to current. */
-            /* Also re-init zi to the first position in new current. */
+    /* If current node is deleted, we must update iterator node and offset. */
+    if (deleted_node) {
+        if (iter->direction == AL_START_HEAD) {
             iter->current = next;
             iter->offset = 0;
-        } else {
-            /* Current node remains.  Replace iterator zi with next zi. */
-            iter->zi = entry->zi;
-            iter->offset++;
-        }
-    } else if (iter->direction == AL_START_TAIL) {
-        if (deleted_node) {
-            /* Current node was deleted.  Assign saved prev to current. */
-            /* Also re-init zi to the last position in new current. */
+        } else if (iter->direction == AL_START_TAIL) {
             iter->current = prev;
             iter->offset = -1;
-        } else {
-            /* Current node still exists. */
         }
     }
+    /* else if (!deleted_node), no changes needed.
+     * we already reset iter->zi above, and the existing iter->offset
+     * doesn't move again because:
+     *   - [1, 2, 3] => delete offset 1 => [1, 3]: next element still offset 1
+     *   - [1, 2, 3] => delete offset 0 => [2, 3]: next element still offset 0
+     *  if we deleted the last element at offet N and now
+     *  length of this ziplist is N-1, the next call into
+     *  quicklistNext() will jump to the next node. */
 }

 /* Replace quicklist entry at offset 'index' by 'data' with length 'sz'.
@@ -573,8 +591,8 @@ int quicklistDelRange(quicklist *quicklist, const long start,

     if (start >= 0 && extent > (quicklist->count - start)) {
         /* if requesting delete more elements than exist, limit to list size. */
-        extent = quicklist->count;
-    } else if (start < 0 && extent > (quicklist->count - (-start) + 1)) {
+        extent = quicklist->count - start;
+    } else if (start < 0 && extent > (unsigned long)(-start)) {
         /* else, if at negative offset, limit max size to rest of list. */
         extent = -start; /* c.f. LREM -29 29; just delete until end. */
     }
@@ -598,7 +616,7 @@ int quicklistDelRange(quicklist *quicklist, const long start,
              * can just delete the entire node without ziplist math. */
             delete_entire_node = 1;
             del = node->count;
-        } else if (entry.offset >= 0 && extent > node->count) {
+        } else if (entry.offset >= 0 && extent >= node->count) {
             /* If deleting more nodes after this one, calculate delete based
              * on size of current node. */
             del = node->count - entry.offset;
@@ -890,14 +908,19 @@ void quicklistRotate(quicklist *quicklist, const size_t fill) {
     /* If value found is NULL, then ziplistGet populated longval instead */
     if (!value) {
         /* Write the longval as a string so we can re-add it */
-        int wrote = ll2string(longstr, sizeof(longstr), longval);
-        value = (unsigned char *)longval;
-        sz = wrote;
+        sz = ll2string(longstr, sizeof(longstr), longval);
+        value = (unsigned char *)longstr;
     }

     /* Add tail entry to head (must happen before tail is deleted). */
     quicklistPushHead(quicklist, fill, value, sz);

+    /* If quicklist has only one node, the head ziplist is also the
+     * tail ziplist and PushHead() could have reallocated our single ziplist,
+     * which would make our pre-existing 'p' unusable. */
+    if (quicklist->len == 1)
+        p = ziplistIndex(tail->zl, -1);
+
     /* Remove tail entry. */
     quicklistDelIndex(quicklist, tail, &p);
 }
@@ -1083,8 +1106,13 @@ static int itrprintr_rev(quicklist *ql, int print) {
     return _itrprintr(ql, print, 0);
 }

+#define ql_verify(a, b, c, d, e)                                               \
+    do {                                                                       \
+        err += _ql_verify((a), (b), (c), (d), (e));                            \
+    } while (0)
+
 /* Verify list metadata matches physical list contents. */
-static void ql_verify(quicklist *ql, uint32_t len, uint32_t count,
+static int _ql_verify(quicklist *ql, uint32_t len, uint32_t count,
                       uint32_t head_count, uint32_t tail_count) {
     int ok = 1;

@@ -1117,7 +1145,7 @@ static void ql_verify(quicklist *ql, uint32_t len, uint32_t count,

     if (ql->len == 0 && ok) {
         OK;
-        return;
+        return !ok;
     }

     if (head_count != ql->head->count &&
@@ -1137,6 +1165,7 @@ static void ql_verify(quicklist *ql, uint32_t len, uint32_t count,
     }
     if (ok)
         OK;
+    return !ok;
 }

 /* Generate new string concatenating integer i against string 'prefix' */
@@ -1150,7 +1179,7 @@ static char *genstr(char *prefix, int i) {
 #define F 32
 /* main test, but callable from other files */
 int quicklistTest(int argc, char *argv[]) {
-    int err = 0;
+    unsigned int err = 0;

     UNUSED(argc);
     UNUSED(argv);
@@ -1249,6 +1278,10 @@ int quicklistTest(int argc, char *argv[]) {
     for (size_t f = 0; f < 1024; f++) {
         TEST_DESC("rotate 500 val 5000 times at fill %lu", f) {
             quicklist *ql = quicklistCreate();
+            quicklistPushHead(ql, f, "900", 3);
+            quicklistPushHead(ql, f, "7000", 4);
+            quicklistPushHead(ql, f, "-1200", 5);
+            quicklistPushHead(ql, f, "42", 2);
             for (int i = 0; i < 500; i++)
                 quicklistPushHead(ql, f, genstr("hello", i), 32);
             ql_info(ql);
@@ -1256,8 +1289,12 @@ int quicklistTest(int argc, char *argv[]) {
                 ql_info(ql);
                 quicklistRotate(ql, f);
             }
-            if (f == 32)
-                ql_verify(ql, 16, 500, 28, 24);
+            if (f == 1)
+                ql_verify(ql, 504, 504, 1, 1);
+            else if (f == 2)
+                ql_verify(ql, 252, 504, 2, 2);
+            else if (f == 32)
+                ql_verify(ql, 16, 504, 32, 24);
             quicklistRelease(ql);
         }
     }
@@ -1622,6 +1659,15 @@ int quicklistTest(int argc, char *argv[]) {
         quicklistRelease(ql);
     }

+    TEST("delete range of entire node with overflow counts") {
+        quicklist *ql = quicklistCreate();
+        for (int i = 0; i < 32; i++)
+            quicklistPushHead(ql, F, genstr("hello", i), 32);
+        quicklistDelRange(ql, 0, 128);
+        ql_verify(ql, 0, 0, 0, 0);
+        quicklistRelease(ql);
+    }
+
     TEST("delete middle 100 of 500 list") {
         quicklist *ql = quicklistCreate();
         for (int i = 0; i < 500; i++)
@@ -1640,6 +1686,15 @@ int quicklistTest(int argc, char *argv[]) {
         quicklistRelease(ql);
     }

+    TEST("delete negative 1 from 500 list with overflow counts") {
+        quicklist *ql = quicklistCreate();
+        for (int i = 0; i < 500; i++)
+            quicklistPushTail(ql, F, genstr("hello", i + 1), 32);
+        quicklistDelRange(ql, -1, 128);
+        ql_verify(ql, 16, 499, 32, 19);
+        quicklistRelease(ql);
+    }
+
     TEST("delete negative 100 from 500 list") {
         quicklist *ql = quicklistCreate();
         for (int i = 0; i < 500; i++)
@@ -1997,6 +2052,30 @@ int quicklistTest(int argc, char *argv[]) {
         }
     }

+    for (size_t f = 0; f < 72; f++) {
+        TEST_DESC("create quicklist from ziplist at fill %lu", f) {
+            unsigned char *zl = ziplistNew();
+            long long nums[32];
+            char num[32];
+            for (int i = 0; i < 33; i++) {
+                nums[i] = -5157318210846258176 + i;
+                int sz = ll2string(num, sizeof(num), nums[i]);
+                zl = ziplistPush(zl, (unsigned char *)num, sz, ZIPLIST_TAIL);
+            }
+            for (int i = 0; i < 33; i++) {
+                zl = ziplistPush(zl, (unsigned char *)genstr("hello", i), 32,
+                                 ZIPLIST_TAIL);
+            }
+            quicklist *ql = quicklistCreateFromZiplist(f, zl);
+            if (f == 1)
+                ql_verify(ql, 66, 66, 1, 1);
+            else if (f == 32)
+                ql_verify(ql, 3, 66, 32, 2);
+            else if (f == 66)
+                ql_verify(ql, 1, 66, 66, 66);
+            quicklistRelease(ql);
+        }
+    }
     if (!err)
         printf("ALL TESTS PASSED!\n");
     else
diff --git a/src/t_list.c b/src/t_list.c
index a7201fd..e740524 100644
--- a/src/t_list.c
+++ b/src/t_list.c
@@ -185,11 +185,10 @@ void listTypeConvert(robj *subject, int enc) {
     redisAssertWithInfo(NULL,subject,subject->encoding==REDIS_ENCODING_ZIPLIST);

     if (enc == REDIS_ENCODING_QUICKLIST) {
-        quicklist *ql = quicklistCreate();
-        quicklistPushTailZiplist(ql, subject->ptr);
+        size_t zlen = server.list_max_ziplist_entries;

         subject->encoding = REDIS_ENCODING_QUICKLIST;
-        subject->ptr = ql;
+        subject->ptr = quicklistCreateFromZiplist(zlen, subject->ptr);
     } else {
         redisPanic("Unsupported list conversion");
     }

mattsta added a commit to mattsta/redis that referenced this pull request Nov 24, 2014

Add quicklist implementation
This replaces individual ziplist vs. linkedlist representations
for Redis list operations.

Big thanks for all the reviews and feedback from everybody in
antirez#2143
@antirez

This comment has been minimized.

Show comment
Hide comment
@antirez

antirez Dec 23, 2014

Owner

@mattsta Not sure what is the problem with unstable, I guess spopCommand() was broken and later fixed into unstable but your code is still rebased with a broken version? So this would mean -> we have no problems with quicklist at all, hopefully.

About the CI, it should definitely detect it, and it can be configured to send emails, currently is disabled because it is a bit annoying... since there are many false positives when you run the test so many times. However I can improve recidiv in order to send only if there is a given pattern (valgrind for example).

However normally I go to check the CI manually very often, but since I'm in my home town, this is what happened: I'm here without access to the box, and the kind person that helps us to keep our house clean apparently powered off the computer for error... :-) No longer CI until I return back first days of January.

About LZF:

  1. Changelog of LZF is at their official site: just download the latest tar.gz and read the Changelog file. Website is here: http://oldhome.schmorp.de/marc/liblzf.html
  2. About our version, I believe we are just one version behind:
3.6  Mon Feb  7 17:37:31 CET 2011
        - fixed hash calculation in C♯ version (Tiago Freitas Leal).
        - unroll copy for small sizes, use memcpy for larger sizes,
          greatly speeding up decompression in most cases.
        - finally disable rep movsb - it's a big loss on modern intel cpus,
          and only a small win on amd cpus.
        - improve C++ compatibility of the code.
        - slightly improve compressor speed.
        - halved memory requirements for compressor on 64 bit architectures,
          which can improve the speed quite a bit on older cpus.

3.5  Fri May  1 02:28:42 CEST 2009
        - lzf_compress did sometimes write one octet past the given output
          buffer (analyzed and nice testcase by Salvatore Sanfilippo).

3.5 mentions my bug report, so I'm pretty sure I updated as soon as the fix was released.
However 3.6 uses less memory and more speed, so may be worth it assuming it's stable.

Owner

antirez commented Dec 23, 2014

@mattsta Not sure what is the problem with unstable, I guess spopCommand() was broken and later fixed into unstable but your code is still rebased with a broken version? So this would mean -> we have no problems with quicklist at all, hopefully.

About the CI, it should definitely detect it, and it can be configured to send emails, currently is disabled because it is a bit annoying... since there are many false positives when you run the test so many times. However I can improve recidiv in order to send only if there is a given pattern (valgrind for example).

However normally I go to check the CI manually very often, but since I'm in my home town, this is what happened: I'm here without access to the box, and the kind person that helps us to keep our house clean apparently powered off the computer for error... :-) No longer CI until I return back first days of January.

About LZF:

  1. Changelog of LZF is at their official site: just download the latest tar.gz and read the Changelog file. Website is here: http://oldhome.schmorp.de/marc/liblzf.html
  2. About our version, I believe we are just one version behind:
3.6  Mon Feb  7 17:37:31 CET 2011
        - fixed hash calculation in C♯ version (Tiago Freitas Leal).
        - unroll copy for small sizes, use memcpy for larger sizes,
          greatly speeding up decompression in most cases.
        - finally disable rep movsb - it's a big loss on modern intel cpus,
          and only a small win on amd cpus.
        - improve C++ compatibility of the code.
        - slightly improve compressor speed.
        - halved memory requirements for compressor on 64 bit architectures,
          which can improve the speed quite a bit on older cpus.

3.5  Fri May  1 02:28:42 CEST 2009
        - lzf_compress did sometimes write one octet past the given output
          buffer (analyzed and nice testcase by Salvatore Sanfilippo).

3.5 mentions my bug report, so I'm pretty sure I updated as soon as the fix was released.
However 3.6 uses less memory and more speed, so may be worth it assuming it's stable.

@mattsta

This comment has been minimized.

Show comment
Hide comment
@mattsta

mattsta Dec 23, 2014

Contributor

I believe we are just one version behind:

Not anymore! I've updated this branch to include the new version. All my tests pass with no crashes or memory leaks. YMMV. :)

🎄

Contributor

mattsta commented Dec 23, 2014

I believe we are just one version behind:

Not anymore! I've updated this branch to include the new version. All my tests pass with no crashes or memory leaks. YMMV. :)

🎄

@antirez

This comment has been minimized.

Show comment
Hide comment
@antirez

antirez Dec 24, 2014

Owner

Thanks for the update of LZF, the only thing to check is if the previous version was actually not modified compared to the original sources, I totally don't recall but that's easy to check.

About the more interesting problem of sdsnative(), actually I see what you are doing here (cit.). I think there is an even better solution that avoids the double-copy and reallocation, I'm sure you thought about it but avoided to implement it to don't put too much stuff into the same PR.

So basically I'm working at a branch (originated from the quicklist branch) that refactors rdb.c in order to have functions that return plain zmalloc allocated objects instead of robjects. It was not a huge change at all and should improve loading times. At the same time I'm reviewing more code from quicklist. In the first days of January at max everything will get merged: in order to merge I've also to do my RDB changes otherwise I need to re-increment the RDB number again, or risk that users have RDB files with same version but different format.

Thanks again! 🎄

Owner

antirez commented Dec 24, 2014

Thanks for the update of LZF, the only thing to check is if the previous version was actually not modified compared to the original sources, I totally don't recall but that's easy to check.

About the more interesting problem of sdsnative(), actually I see what you are doing here (cit.). I think there is an even better solution that avoids the double-copy and reallocation, I'm sure you thought about it but avoided to implement it to don't put too much stuff into the same PR.

So basically I'm working at a branch (originated from the quicklist branch) that refactors rdb.c in order to have functions that return plain zmalloc allocated objects instead of robjects. It was not a huge change at all and should improve loading times. At the same time I'm reviewing more code from quicklist. In the first days of January at max everything will get merged: in order to merge I've also to do my RDB changes otherwise I need to re-increment the RDB number again, or risk that users have RDB files with same version but different format.

Thanks again! 🎄

mattsta added some commits Nov 13, 2014

Add quicklist implementation
This replaces individual ziplist vs. linkedlist representations
for Redis list operations.

Big thanks for all the reviews and feedback from everybody in
#2143
Add ziplistMerge()
This started out as #2158 by sunheehnus, but I kept rewriting it
until I could understand things more easily and get a few more
correctness guarantees out of the readability flow.

The original commit created and returned a new ziplist with the contents of
both input ziplists, but I prefer to grow one of the input ziplists
and destroy the other one.

So, instead of malloc+copy as in #2158, the merge now reallocs one of
the existing ziplists and copies the other ziplist into the new space.

Also added merge test cases to ziplistTest()
Free ziplist test lists during tests
Freeing our test lists helps keep valgrind output clean
Add adaptive quicklist fill factor
Fill factor now has two options:
  - negative (1-5) for size-based ziplist filling
  - positive for length-based ziplist filling with implicit size cap.

Negative offsets define ziplist size limits of:
  -1: 4k
  -2: 8k
  -3: 16k
  -4: 32k
  -5: 64k

Positive offsets now automatically limit their max size to 8k.  Any
elements larger than 8k will be in individual nodes.

Positive ziplist fill factors will keep adding elements
to a ziplist until one of:
  - ziplist has FILL number of elements
    - or -
  - ziplist grows above our ziplist max size (currently 8k)

When using positive fill factors, if you insert a large
element (over 8k), that element will automatically allocate
an individual quicklist node with one element and no other elements will be
in the same ziplist inside that quicklist node.

When using negative fill factors, elements up to the size
limit can be added to one quicklist node.  If an element
is added larger than the max ziplist size, that element
will be allocated an individual ziplist in a new quicklist node.

Tests also updated to start testing at fill factor -5.
Add sdsnative()
Use the existing memory space for an SDS to convert it to a regular
character buffer so we don't need to allocate duplicate space just
to extract a usable buffer for native operations.
Convert quicklist RDB to store ziplist nodes
Turns out it's a huge improvement during save/reload/migrate/restore
because, with compression enabled, we're compressing 4k or 8k
chunks of data consisting of multiple elements in one ziplist
instead of compressing series of smaller individual elements.
Convert RDB ziplist loading to sdsnative()
This saves us an unnecessary zmalloc, memcpy, and two frees.
Increase test size for migrating large values
Previously, the old test ran 5,000 loops and used about 500k.

With quicklist, storing those same 5,000 loops takes up 24k, so the
"large value check" failed!

This increases the test to 20,000 loops which makes the object dump 96k.
Remove malloc failure checks
We trust zmalloc to kill the whole process on memory failure
Allow compression of interior quicklist nodes
Let user set how many nodes to *not* compress.

We can specify a compression "depth" of how many nodes
to leave uncompressed on each end of the quicklist.

Depth 0 = disable compression.
Depth 1 = only leave head/tail uncompressed.
  - (read as: "skip 1 node on each end of the list before compressing")
Depth 2 = leave head, head->next, tail->prev, tail uncompressed.
  - ("skip 2 nodes on each end of the list before compressing")
Depth 3 = Depth 2 + head->next->next + tail->prev->prev
  - ("skip 3 nodes...")
etc.

This also:
  - updates RDB storage to use native quicklist compression (if node is
    already compressed) instead of uncompressing, generating the RDB string,
    then re-compressing the quicklist node.
  - internalizes the "fill" parameter for the quicklist so we don't
    need to pass it to _every_ function.  Now it's just a property of
    the list.
  - allows a runtime-configurable compression option, so we can
    expose a compresion parameter in the configuration file if people
    want to trade slight request-per-second performance for up to 90%+
    memory savings in some situations.
  - updates the quicklist tests to do multiple passes: 200k+ tests now.
Add quicklist info to DEBUG OBJECT
Added field 'ql_nodes' and 'ql_avg_per_node'.

ql_nodes is the number of quicklist nodes in the quicklist.
ql_avg_node is the average fill level in each quicklist node. (LLEN / QL_NODES)

Sample output:
127.0.0.1:6379> DEBUG object b
Value at:0x7fa42bf2fed0 refcount:1 encoding:quicklist serializedlength:18489 lru:8983768 lru_seconds_idle:3 ql_nodes:430 ql_avg_per_node:511.73
127.0.0.1:6379> llen b
(integer) 220044
Cleanup quicklist style
Small fixes due to a new version of clang-format (it's less
crazy than the older version).
Add branch prediction hints to quicklist
Actually makes a noticeable difference.

Branch hints were selected based on profiler hotspots.
Config: Add quicklist, remove old list options
This removes:
  - list-max-ziplist-entries
  - list-max-ziplist-value

This adds:
  - list-max-ziplist-size
  - list-compress-depth

Also updates config file with new sections and updates
tests to use quicklist settings instead of old list settings.
Add more quicklist info to DEBUG OBJECT
Adds: ql_compressed (boolean, 1 if compression enabled for list, 0
otherwise)
Adds: ql_uncompressed_size (actual uncompressed size of all quicklistNodes)
Adds: ql_ziplist_max (quicklist max ziplist fill factor)

Compression ratio of the list is then ql_uncompressed_size / serializedlength

We report ql_uncompressed_size for all quicklists because serializedlength
is a _compressed_ representation anyway.

Sample output from a large list:
127.0.0.1:6379> llen abc
(integer) 38370061
127.0.0.1:6379> debug object abc
Value at:0x7ff97b51d140 refcount:1 encoding:quicklist serializedlength:19878335 lru:9718164 lru_seconds_idle:5 ql_nodes:21945 ql_avg_node:1748.46 ql_ziplist_max:-2 ql_compressed:0 ql_uncompressed_size:1643187761
(1.36s)

The 1.36s result time is because rdbSavedObjectLen() is serializing the
object, not because of any new stats reporting.

If we run DEBUG OBJECT on a compressed list, DEBUG OBJECT takes almost *zero*
time because rdbSavedObjectLen() reuses already-compressed ziplists:
127.0.0.1:6379> debug object abc
Value at:0x7fe5c5800040 refcount:1 encoding:quicklist serializedlength:19878335 lru:9718109 lru_seconds_idle:5 ql_nodes:21945 ql_avg_node:1748.46 ql_ziplist_max:-2 ql_compressed:1 ql_uncompressed_size:1643187761
Set optional 'static' for Quicklist+Redis
This also defines REDIS_STATIC='' for building everything
inside src/ and everything inside deps/lua/.
@antirez

This comment has been minimized.

Show comment
Hide comment
@antirez

antirez Jan 7, 2015

Owner

Hello Matt, in order to merge I'm doing my changes to RDB here:

https://github.com/antirez/redis/commits/rdbchanges

I see that we diverged a bit, you mostly changed spacing and did a force update: trivial to rebase my changes upon yours, but please for future change add commits instead of force-updating so that it will be simpler to merge. Thx!

Owner

antirez commented Jan 7, 2015

Hello Matt, in order to merge I'm doing my changes to RDB here:

https://github.com/antirez/redis/commits/rdbchanges

I see that we diverged a bit, you mostly changed spacing and did a force update: trivial to rebase my changes upon yours, but please for future change add commits instead of force-updating so that it will be simpler to merge. Thx!

antirez added a commit that referenced this pull request Jan 8, 2015

Merge pull request #2143 from mattsta/quicklist
Quicklist (linked list + ziplist)

@antirez antirez merged commit 05ba119 into antirez:unstable Jan 8, 2015

@antirez

This comment has been minimized.

Show comment
Hide comment
@antirez

antirez Jan 8, 2015

Owner

Merged! Applying my commits from the other branch to master as well, and RDB v7 is done...

Owner

antirez commented Jan 8, 2015

Merged! Applying my commits from the other branch to master as well, and RDB v7 is done...

@mattsta

This comment has been minimized.

Show comment
Hide comment
@mattsta

mattsta Jan 9, 2015

Contributor

I see that we diverged a bit,

Sorry for the confusion! Thanks for getting this added (and thanks for the new K/V RDB format)!

trivial to rebase my changes upon yours, but please for future change add commits instead of force-updating so that it will be simpler to merge.

That's the problem with distributed revision control (and github PRs)... the code lives in the author's repo and can be updated at any time.

If I did only add new commits after this PR was first created, it would have 100 cleanup commits sitting here. :)

The real problem is git has a bad default interface. The default should be git pull -r to automatically fix any upstream changes. Without git pull -r, git thinks everything changed on top of existing changes, and it can't deal with it.

So, if git pull freaks out, run git merge --abort then git pull -r.

Or, worst case, git checkout unstable; git branch -D [messed up branch]; git checkout [clean branch] (assuming the branch is only the PR branch without any local changes on it).

:shipit:

Contributor

mattsta commented Jan 9, 2015

I see that we diverged a bit,

Sorry for the confusion! Thanks for getting this added (and thanks for the new K/V RDB format)!

trivial to rebase my changes upon yours, but please for future change add commits instead of force-updating so that it will be simpler to merge.

That's the problem with distributed revision control (and github PRs)... the code lives in the author's repo and can be updated at any time.

If I did only add new commits after this PR was first created, it would have 100 cleanup commits sitting here. :)

The real problem is git has a bad default interface. The default should be git pull -r to automatically fix any upstream changes. Without git pull -r, git thinks everything changed on top of existing changes, and it can't deal with it.

So, if git pull freaks out, run git merge --abort then git pull -r.

Or, worst case, git checkout unstable; git branch -D [messed up branch]; git checkout [clean branch] (assuming the branch is only the PR branch without any local changes on it).

:shipit:

@hey-jude

This comment has been minimized.

Show comment
Hide comment
@hey-jude

hey-jude Dec 11, 2015

👍
Which version can I use this feature?

hey-jude commented Dec 11, 2015

👍
Which version can I use this feature?

@badboy

This comment has been minimized.

Show comment
Hide comment
@badboy

badboy Dec 11, 2015

Contributor

It's currently in the testing branch and will thus end up in the upcoming 3.2 release.

Contributor

badboy commented Dec 11, 2015

It's currently in the testing branch and will thus end up in the upcoming 3.2 release.

JackieXie168 pushed a commit to JackieXie168/redis that referenced this pull request Aug 29, 2016

Add quicklist implementation
This replaces individual ziplist vs. linkedlist representations
for Redis list operations.

Big thanks for all the reviews and feedback from everybody in
antirez#2143
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment