From 4e16d8b312d913085d70617dd75cc7550610ce72 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Mon, 7 Jun 2010 21:53:21 +0200 Subject: [PATCH 01/11] compute swappability for ziplist encoded lists --- redis.c | 33 +++++++++++++++------------------ 1 file changed, 15 insertions(+), 18 deletions(-) diff --git a/redis.c b/redis.c index d358ba3a..f970425e 100644 --- a/redis.c +++ b/redis.c @@ -9584,8 +9584,10 @@ static double computeObjectSwappability(robj *o) { /* actual age can be >= minage, but not < minage. As we use wrapping * 21 bit clocks with minutes resolution for the LRU. */ time_t minage = abs(server.lruclock - o->lru); - long asize = 0; + long asize = 0, elesize; + robj *ele; list *l; + listNode *ln; dict *d; struct dictEntry *de; int z; @@ -9600,17 +9602,18 @@ static double computeObjectSwappability(robj *o) { } break; case REDIS_LIST: - l = o->ptr; - listNode *ln = listFirst(l); - - asize = sizeof(list); - if (ln) { - robj *ele = ln->value; - long elesize; - - elesize = (ele->encoding == REDIS_ENCODING_RAW) ? - (sizeof(*o)+sdslen(ele->ptr)) : sizeof(*o); - asize += (sizeof(listNode)+elesize)*listLength(l); + if (o->encoding == REDIS_ENCODING_ZIPLIST) { + asize = sizeof(*o)+ziplistSize(o->ptr); + } else { + l = o->ptr; + ln = listFirst(l); + asize = sizeof(list); + if (ln) { + ele = ln->value; + elesize = (ele->encoding == REDIS_ENCODING_RAW) ? + (sizeof(*o)+sdslen(ele->ptr)) : sizeof(*o); + asize += (sizeof(listNode)+elesize)*listLength(l); + } } break; case REDIS_SET: @@ -9621,9 +9624,6 @@ static double computeObjectSwappability(robj *o) { asize = sizeof(dict)+(sizeof(struct dictEntry*)*dictSlots(d)); if (z) asize += sizeof(zset)-sizeof(dict); if (dictSize(d)) { - long elesize; - robj *ele; - de = dictGetRandomKey(d); ele = dictGetEntryKey(de); elesize = (ele->encoding == REDIS_ENCODING_RAW) ? @@ -9648,9 +9648,6 @@ static double computeObjectSwappability(robj *o) { d = o->ptr; asize = sizeof(dict)+(sizeof(struct dictEntry*)*dictSlots(d)); if (dictSize(d)) { - long elesize; - robj *ele; - de = dictGetRandomKey(d); ele = dictGetEntryKey(de); elesize = (ele->encoding == REDIS_ENCODING_RAW) ? From 306974f5d77c9b0245c6dbb8c44cd4f358eac521 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Wed, 9 Jun 2010 11:36:58 +0200 Subject: [PATCH 02/11] remove pop function and the sds dependency; can be implemented using get+delete --- ziplist.c | 63 +++++++++++++++++++++++++------------------------------ ziplist.h | 1 - 2 files changed, 29 insertions(+), 35 deletions(-) diff --git a/ziplist.c b/ziplist.c index d1d7a151..4b9d0fad 100644 --- a/ziplist.c +++ b/ziplist.c @@ -21,7 +21,6 @@ #include #include #include "zmalloc.h" -#include "sds.h" #include "ziplist.h" /* Important note: the ZIP_END value is used to depict the end of the @@ -382,30 +381,6 @@ unsigned char *ziplistPush(unsigned char *zl, unsigned char *s, unsigned int sle return __ziplistInsert(zl,p,s,slen); } -unsigned char *ziplistPop(unsigned char *zl, sds *target, int where) { - zlentry entry; - unsigned char *p; - long long value; - if (target) *target = NULL; - - /* Get pointer to element to remove */ - p = (where == ZIPLIST_HEAD) ? ZIPLIST_ENTRY_HEAD(zl) : ZIPLIST_ENTRY_TAIL(zl); - if (*p == ZIP_END) return zl; - - entry = zipEntry(p); - if (target) { - if (entry.encoding == ZIP_ENC_RAW) { - *target = sdsnewlen(p+entry.headersize,entry.len); - } else { - value = zipLoadInteger(p+entry.headersize,entry.encoding); - *target = sdscatprintf(sdsempty(), "%lld", value); - } - } - - zl = __ziplistDelete(zl,p,1); - return zl; -} - /* Returns an offset to use for iterating with ziplistNext. When the given * index is negative, the list is traversed back to front. When the list * doesn't contain an element at the provided index, NULL is returned. */ @@ -644,12 +619,36 @@ void stress(int pos, int num, int maxsize, int dnum) { } } +void pop(unsigned char *zl, int where) { + unsigned char *p, *vstr; + unsigned int vlen; + long long vlong; + + p = ziplistIndex(zl,where == ZIPLIST_HEAD ? 0 : -1); + if (ziplistGet(p,&vstr,&vlen,&vlong)) { + if (where == ZIPLIST_HEAD) + printf("Pop head: "); + else + printf("Pop tail: "); + + if (vstr) + fwrite(vstr,vlen,1,stdout); + else + printf("%lld", vlong); + + printf("\n"); + ziplistDeleteRange(zl,-1,1); + } else { + printf("ERROR: Could not pop\n"); + exit(1); + } +} + int main(int argc, char **argv) { unsigned char *zl, *p; unsigned char *entry; unsigned int elen; long long value; - sds s; zl = createIntList(); ziplistRepr(zl); @@ -657,20 +656,16 @@ int main(int argc, char **argv) { zl = createList(); ziplistRepr(zl); - zl = ziplistPop(zl, &s, ZIPLIST_TAIL); - printf("Pop tail: %s (length %ld)\n", s, sdslen(s)); + pop(zl,ZIPLIST_TAIL); ziplistRepr(zl); - zl = ziplistPop(zl, &s, ZIPLIST_HEAD); - printf("Pop head: %s (length %ld)\n", s, sdslen(s)); + pop(zl,ZIPLIST_HEAD); ziplistRepr(zl); - zl = ziplistPop(zl, &s, ZIPLIST_TAIL); - printf("Pop tail: %s (length %ld)\n", s, sdslen(s)); + pop(zl,ZIPLIST_TAIL); ziplistRepr(zl); - zl = ziplistPop(zl, &s, ZIPLIST_TAIL); - printf("Pop tail: %s (length %ld)\n", s, sdslen(s)); + pop(zl,ZIPLIST_TAIL); ziplistRepr(zl); printf("Get element at index 3:\n"); diff --git a/ziplist.h b/ziplist.h index 6d9037dc..31125725 100644 --- a/ziplist.h +++ b/ziplist.h @@ -3,7 +3,6 @@ unsigned char *ziplistNew(void); unsigned char *ziplistPush(unsigned char *zl, unsigned char *s, unsigned int slen, int where); -unsigned char *ziplistPop(unsigned char *zl, sds *target, int where); unsigned char *ziplistIndex(unsigned char *zl, int index); unsigned char *ziplistNext(unsigned char *zl, unsigned char *p); unsigned char *ziplistPrev(unsigned char *zl, unsigned char *p); From dedff272f640b2df73062da06b8f3b82e19de2c2 Mon Sep 17 00:00:00 2001 From: Robey Pointer Date: Fri, 11 Jun 2010 10:08:59 +0200 Subject: [PATCH 03/11] squashed merge from robey/twitter3: LINSERT BEFORE|AFTER, LPUSHX, RPUSHX --- adlist.c | 35 +++++++++++++++-- adlist.h | 1 + redis.c | 84 +++++++++++++++++++++++++++++++++++++++- tests/support/redis.tcl | 2 +- tests/unit/type/list.tcl | 48 +++++++++++++++++++++++ 5 files changed, 165 insertions(+), 5 deletions(-) diff --git a/adlist.c b/adlist.c index fd2d6fd1..015012f5 100644 --- a/adlist.c +++ b/adlist.c @@ -123,6 +123,35 @@ list *listAddNodeTail(list *list, void *value) return list; } +list *listInsertNode(list *list, listNode *old_node, void *value, int after) { + listNode *node; + + if ((node = zmalloc(sizeof(*node))) == NULL) + return NULL; + node->value = value; + if (after) { + node->prev = old_node; + node->next = old_node->next; + if (list->tail == old_node) { + list->tail = node; + } + } else { + node->next = old_node; + node->prev = old_node->prev; + if (list->head == old_node) { + list->head = node; + } + } + if (node->prev != NULL) { + node->prev->next = node; + } + if (node->next != NULL) { + node->next->prev = node; + } + list->len++; + return list; +} + /* Remove the specified node from the specified list. * It's up to the caller to free the private value of the node. * @@ -183,9 +212,9 @@ void listRewindTail(list *list, listIter *li) { * or NULL if there are no more elements, so the classical usage patter * is: * - * iter = listGetItarotr(list,); - * while ((node = listNextIterator(iter)) != NULL) { - * DoSomethingWith(listNodeValue(node)); + * iter = listGetIterator(list,); + * while ((node = listNext(iter)) != NULL) { + * doSomethingWith(listNodeValue(node)); * } * * */ diff --git a/adlist.h b/adlist.h index 41ca13f1..a1209f62 100644 --- a/adlist.h +++ b/adlist.h @@ -74,6 +74,7 @@ list *listCreate(void); void listRelease(list *list); list *listAddNodeHead(list *list, void *value); list *listAddNodeTail(list *list, void *value); +list *listInsertNode(list *list, listNode *old_node, void *value, int after); void listDelNode(list *list, listNode *node); listIter *listGetIterator(list *list, int direction); listNode *listNext(listIter *iter); diff --git a/redis.c b/redis.c index f970425e..5d44e827 100644 --- a/redis.c +++ b/redis.c @@ -261,7 +261,7 @@ typedef struct redisObject { unsigned lru:22; /* lru time (relative to server.lruclock) */ int refcount; void *ptr; - /* VM fields, this are only allocated if VM is active, otherwise the + /* VM fields are only allocated if VM is active, otherwise the * object allocation function will just allocate * sizeof(redisObjct) minus sizeof(redisObjectVM), so using * Redis without VM active will not have any overhead. */ @@ -692,6 +692,9 @@ static void renameCommand(redisClient *c); static void renamenxCommand(redisClient *c); static void lpushCommand(redisClient *c); static void rpushCommand(redisClient *c); +static void lpushxCommand(redisClient *c); +static void rpushxCommand(redisClient *c); +static void linsertCommand(redisClient *c); static void lpopCommand(redisClient *c); static void rpopCommand(redisClient *c); static void llenCommand(redisClient *c); @@ -792,6 +795,9 @@ static struct redisCommand readonlyCommandTable[] = { {"mget",mgetCommand,-2,REDIS_CMD_INLINE,NULL,1,-1,1}, {"rpush",rpushCommand,3,REDIS_CMD_BULK|REDIS_CMD_DENYOOM,NULL,1,1,1}, {"lpush",lpushCommand,3,REDIS_CMD_BULK|REDIS_CMD_DENYOOM,NULL,1,1,1}, + {"rpushx",rpushxCommand,3,REDIS_CMD_BULK|REDIS_CMD_DENYOOM,NULL,1,1,1}, + {"lpushx",lpushxCommand,3,REDIS_CMD_BULK|REDIS_CMD_DENYOOM,NULL,1,1,1}, + {"linsert",linsertCommand,5,REDIS_CMD_BULK|REDIS_CMD_DENYOOM,NULL,1,1,1}, {"rpop",rpopCommand,2,REDIS_CMD_INLINE,NULL,1,1,1}, {"lpop",lpopCommand,2,REDIS_CMD_INLINE,NULL,1,1,1}, {"brpop",brpopCommand,-3,REDIS_CMD_INLINE,NULL,1,1,1}, @@ -5174,6 +5180,82 @@ static void rpushCommand(redisClient *c) { pushGenericCommand(c,REDIS_TAIL); } +static void listTypeInsert(robj *subject, listTypeEntry *old_entry, robj *new_obj, int where) { + listTypeTryConversion(subject,new_obj); + if (subject->encoding == REDIS_ENCODING_ZIPLIST) { + if (where == REDIS_HEAD) { + unsigned char *next = ziplistNext(subject->ptr,old_entry->zi); + if (next == NULL) { + listTypePush(subject,new_obj,REDIS_TAIL); + } else { + subject->ptr = ziplistInsert(subject->ptr,next,new_obj->ptr,sdslen(new_obj->ptr)); + } + } else { + subject->ptr = ziplistInsert(subject->ptr,old_entry->zi,new_obj->ptr,sdslen(new_obj->ptr)); + } + } else if (subject->encoding == REDIS_ENCODING_LIST) { + if (where == REDIS_HEAD) { + listInsertNode(subject->ptr,old_entry->ln,new_obj,1); + } else { + listInsertNode(subject->ptr,old_entry->ln,new_obj,0); + } + incrRefCount(new_obj); + } else { + redisPanic("Unknown list encoding"); + } +} + +static void pushxGenericCommand(redisClient *c, int where, robj *old_obj, robj *new_obj) { + robj *subject; + listTypeIterator *iter; + listTypeEntry entry; + + if ((subject = lookupKeyReadOrReply(c,c->argv[1],shared.czero)) == NULL || + checkType(c,subject,REDIS_LIST)) return; + if (handleClientsWaitingListPush(c,c->argv[1],new_obj)) { + addReply(c,shared.cone); + return; + } + + if (old_obj != NULL) { + if (where == REDIS_HEAD) { + iter = listTypeInitIterator(subject,0,REDIS_TAIL); + } else { + iter = listTypeInitIterator(subject,-1,REDIS_HEAD); + } + while (listTypeNext(iter,&entry)) { + if (listTypeEqual(&entry,old_obj)) { + listTypeInsert(subject,&entry,new_obj,where); + break; + } + } + listTypeReleaseIterator(iter); + } else { + listTypePush(subject,new_obj,where); + } + + server.dirty++; + addReplyUlong(c,listTypeLength(subject)); +} + +static void lpushxCommand(redisClient *c) { + pushxGenericCommand(c,REDIS_HEAD,NULL,c->argv[2]); +} + +static void rpushxCommand(redisClient *c) { + pushxGenericCommand(c,REDIS_TAIL,NULL,c->argv[2]); +} + +static void linsertCommand(redisClient *c) { + if (strcasecmp(c->argv[2]->ptr,"after") == 0) { + pushxGenericCommand(c,REDIS_HEAD,c->argv[3],c->argv[4]); + } else if (strcasecmp(c->argv[2]->ptr,"before") == 0) { + pushxGenericCommand(c,REDIS_TAIL,c->argv[3],c->argv[4]); + } else { + addReply(c,shared.syntaxerr); + } +} + static void llenCommand(redisClient *c) { robj *o = lookupKeyReadOrReply(c,c->argv[1],shared.czero); if (o == NULL || checkType(c,o,REDIS_LIST)) return; diff --git a/tests/support/redis.tcl b/tests/support/redis.tcl index 0f4e401f..8f7d7711 100644 --- a/tests/support/redis.tcl +++ b/tests/support/redis.tcl @@ -40,7 +40,7 @@ array set ::redis::multibulkarg {} # Flag commands requiring last argument as a bulk write operation foreach redis_bulk_cmd { - set setnx rpush lpush lset lrem sadd srem sismember echo getset smove zadd zrem zscore zincrby append zrank zrevrank hget hdel hexists setex + set setnx rpush lpush rpushx lpushx linsert lset lrem sadd srem sismember echo getset smove zadd zrem zscore zincrby append zrank zrevrank hget hdel hexists setex } { set ::redis::bulkarg($redis_bulk_cmd) {} } diff --git a/tests/unit/type/list.tcl b/tests/unit/type/list.tcl index 22f40958..691040ec 100644 --- a/tests/unit/type/list.tcl +++ b/tests/unit/type/list.tcl @@ -52,6 +52,54 @@ start_server { assert_equal $large_value [r lindex mylist2 2] } + test {LPUSHX, RPUSHX, LPUSHXAFTER, RPUSHXAFTER - ziplist} { + r del xlist + assert_equal 0 [r lpushx xlist a] + assert_equal 0 [r rpushx xlist a] + assert_equal 1 [r rpush xlist b] + assert_equal 2 [r rpush xlist c] + assert_equal 3 [r rpushx xlist d] + assert_equal 4 [r lpushx xlist a] + assert_encoding ziplist xlist + assert_equal {a b c d} [r lrange xlist 0 10] + + assert_equal 5 [r linsert xlist before c zz] + assert_equal {a b zz c d} [r lrange xlist 0 10] + assert_equal 6 [r linsert xlist after c yy] + assert_equal {a b zz c yy d} [r lrange xlist 0 10] + assert_equal 7 [r linsert xlist after d dd] + assert_equal 7 [r linsert xlist after bad ddd] + assert_equal {a b zz c yy d dd} [r lrange xlist 0 10] + assert_equal 8 [r linsert xlist before a aa] + assert_equal 8 [r linsert xlist before bad aaa] + assert_equal {aa a b zz c yy d dd} [r lrange xlist 0 10] + } + + test {LPUSHX, RPUSHX, LPUSHXAFTER, RPUSHXAFTER - regular list} { + set large_value "aaaaaaaaaaaaaaaaa" + + r del xlist + assert_equal 0 [r lpushx xlist a] + assert_equal 0 [r rpushx xlist a] + assert_equal 1 [r rpush xlist $large_value] + assert_equal 2 [r rpush xlist c] + assert_equal 3 [r rpushx xlist d] + assert_equal 4 [r lpushx xlist a] + assert_encoding list xlist + assert_equal {a aaaaaaaaaaaaaaaaa c d} [r lrange xlist 0 10] + + assert_equal 5 [r linsert xlist before c zz] + assert_equal {a aaaaaaaaaaaaaaaaa zz c d} [r lrange xlist 0 10] + assert_equal 6 [r linsert xlist after c yy] + assert_equal {a aaaaaaaaaaaaaaaaa zz c yy d} [r lrange xlist 0 10] + assert_equal 7 [r linsert xlist after d dd] + assert_equal 7 [r linsert xlist after bad ddd] + assert_equal {a aaaaaaaaaaaaaaaaa zz c yy d dd} [r lrange xlist 0 10] + assert_equal 8 [r linsert xlist before a aa] + assert_equal 8 [r linsert xlist before bad aaa] + assert_equal {aa a aaaaaaaaaaaaaaaaa zz c yy d dd} [r lrange xlist 0 10] + } + test {DEL a list - ziplist} { assert_equal 1 [r del myziplist2] assert_equal 0 [r exists myziplist2] From 0e1684bcd0c6b3378fb3f957407b08fac3f24bb1 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Fri, 11 Jun 2010 10:52:09 +0200 Subject: [PATCH 04/11] move listTypeInsert to be grouped with other wrapper functions --- redis.c | 50 +++++++++++++++++++++++++------------------------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/redis.c b/redis.c index 5d44e827..6f082972 100644 --- a/redis.c +++ b/redis.c @@ -5088,6 +5088,31 @@ static robj *listTypeGet(listTypeEntry *entry) { return value; } +static void listTypeInsert(robj *subject, listTypeEntry *old_entry, robj *new_obj, int where) { + listTypeTryConversion(subject,new_obj); + if (subject->encoding == REDIS_ENCODING_ZIPLIST) { + if (where == REDIS_HEAD) { + unsigned char *next = ziplistNext(subject->ptr,old_entry->zi); + if (next == NULL) { + listTypePush(subject,new_obj,REDIS_TAIL); + } else { + subject->ptr = ziplistInsert(subject->ptr,next,new_obj->ptr,sdslen(new_obj->ptr)); + } + } else { + subject->ptr = ziplistInsert(subject->ptr,old_entry->zi,new_obj->ptr,sdslen(new_obj->ptr)); + } + } else if (subject->encoding == REDIS_ENCODING_LIST) { + if (where == REDIS_HEAD) { + listInsertNode(subject->ptr,old_entry->ln,new_obj,1); + } else { + listInsertNode(subject->ptr,old_entry->ln,new_obj,0); + } + incrRefCount(new_obj); + } else { + redisPanic("Unknown list encoding"); + } +} + /* Compare the given object with the entry at the current position. */ static int listTypeEqual(listTypeEntry *entry, robj *o) { listTypeIterator *li = entry->li; @@ -5180,31 +5205,6 @@ static void rpushCommand(redisClient *c) { pushGenericCommand(c,REDIS_TAIL); } -static void listTypeInsert(robj *subject, listTypeEntry *old_entry, robj *new_obj, int where) { - listTypeTryConversion(subject,new_obj); - if (subject->encoding == REDIS_ENCODING_ZIPLIST) { - if (where == REDIS_HEAD) { - unsigned char *next = ziplistNext(subject->ptr,old_entry->zi); - if (next == NULL) { - listTypePush(subject,new_obj,REDIS_TAIL); - } else { - subject->ptr = ziplistInsert(subject->ptr,next,new_obj->ptr,sdslen(new_obj->ptr)); - } - } else { - subject->ptr = ziplistInsert(subject->ptr,old_entry->zi,new_obj->ptr,sdslen(new_obj->ptr)); - } - } else if (subject->encoding == REDIS_ENCODING_LIST) { - if (where == REDIS_HEAD) { - listInsertNode(subject->ptr,old_entry->ln,new_obj,1); - } else { - listInsertNode(subject->ptr,old_entry->ln,new_obj,0); - } - incrRefCount(new_obj); - } else { - redisPanic("Unknown list encoding"); - } -} - static void pushxGenericCommand(redisClient *c, int where, robj *old_obj, robj *new_obj) { robj *subject; listTypeIterator *iter; From 279d7e67cf7c4ac0db401291c01588a26c1da9ea Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Fri, 11 Jun 2010 11:53:16 +0200 Subject: [PATCH 05/11] use REDIS_TAIL to insert AFTER an entry and REDIS_HEAD to insert BEFORE an entry --- redis.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/redis.c b/redis.c index 6f082972..22894ec6 100644 --- a/redis.c +++ b/redis.c @@ -5091,7 +5091,7 @@ static robj *listTypeGet(listTypeEntry *entry) { static void listTypeInsert(robj *subject, listTypeEntry *old_entry, robj *new_obj, int where) { listTypeTryConversion(subject,new_obj); if (subject->encoding == REDIS_ENCODING_ZIPLIST) { - if (where == REDIS_HEAD) { + if (where == REDIS_TAIL) { unsigned char *next = ziplistNext(subject->ptr,old_entry->zi); if (next == NULL) { listTypePush(subject,new_obj,REDIS_TAIL); @@ -5102,10 +5102,10 @@ static void listTypeInsert(robj *subject, listTypeEntry *old_entry, robj *new_ob subject->ptr = ziplistInsert(subject->ptr,old_entry->zi,new_obj->ptr,sdslen(new_obj->ptr)); } } else if (subject->encoding == REDIS_ENCODING_LIST) { - if (where == REDIS_HEAD) { - listInsertNode(subject->ptr,old_entry->ln,new_obj,1); + if (where == REDIS_TAIL) { + listInsertNode(subject->ptr,old_entry->ln,new_obj,AL_START_TAIL); } else { - listInsertNode(subject->ptr,old_entry->ln,new_obj,0); + listInsertNode(subject->ptr,old_entry->ln,new_obj,AL_START_HEAD); } incrRefCount(new_obj); } else { @@ -5218,7 +5218,7 @@ static void pushxGenericCommand(redisClient *c, int where, robj *old_obj, robj * } if (old_obj != NULL) { - if (where == REDIS_HEAD) { + if (where == REDIS_TAIL) { iter = listTypeInitIterator(subject,0,REDIS_TAIL); } else { iter = listTypeInitIterator(subject,-1,REDIS_HEAD); @@ -5248,9 +5248,9 @@ static void rpushxCommand(redisClient *c) { static void linsertCommand(redisClient *c) { if (strcasecmp(c->argv[2]->ptr,"after") == 0) { - pushxGenericCommand(c,REDIS_HEAD,c->argv[3],c->argv[4]); - } else if (strcasecmp(c->argv[2]->ptr,"before") == 0) { pushxGenericCommand(c,REDIS_TAIL,c->argv[3],c->argv[4]); + } else if (strcasecmp(c->argv[2]->ptr,"before") == 0) { + pushxGenericCommand(c,REDIS_HEAD,c->argv[3],c->argv[4]); } else { addReply(c,shared.syntaxerr); } From 1240552da92c7a1aa490cea6f6a66d1265c587e2 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Fri, 11 Jun 2010 12:03:15 +0200 Subject: [PATCH 06/11] always iterate from head to tail on LINSERT --- redis.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/redis.c b/redis.c index 22894ec6..f0a0cd3d 100644 --- a/redis.c +++ b/redis.c @@ -5218,11 +5218,7 @@ static void pushxGenericCommand(redisClient *c, int where, robj *old_obj, robj * } if (old_obj != NULL) { - if (where == REDIS_TAIL) { - iter = listTypeInitIterator(subject,0,REDIS_TAIL); - } else { - iter = listTypeInitIterator(subject,-1,REDIS_HEAD); - } + iter = listTypeInitIterator(subject,0,REDIS_TAIL); while (listTypeNext(iter,&entry)) { if (listTypeEqual(&entry,old_obj)) { listTypeInsert(subject,&entry,new_obj,where); From bcfb387694534aa5f2f8ffb3cc12ea6a5f55b092 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Fri, 11 Jun 2010 13:06:03 +0200 Subject: [PATCH 07/11] rename vars, move arguments, add comments --- redis.c | 50 +++++++++++++++++++++++++++++--------------------- 1 file changed, 29 insertions(+), 21 deletions(-) diff --git a/redis.c b/redis.c index f0a0cd3d..c39f5846 100644 --- a/redis.c +++ b/redis.c @@ -5088,26 +5088,29 @@ static robj *listTypeGet(listTypeEntry *entry) { return value; } -static void listTypeInsert(robj *subject, listTypeEntry *old_entry, robj *new_obj, int where) { - listTypeTryConversion(subject,new_obj); - if (subject->encoding == REDIS_ENCODING_ZIPLIST) { +static void listTypeInsert(listTypeEntry *entry, robj *value, int where) { + robj *subject = entry->li->subject; + if (entry->li->encoding == REDIS_ENCODING_ZIPLIST) { if (where == REDIS_TAIL) { - unsigned char *next = ziplistNext(subject->ptr,old_entry->zi); + unsigned char *next = ziplistNext(subject->ptr,entry->zi); + + /* When we insert after the current element, but the current element + * is the tail of the list, we need to do a push. */ if (next == NULL) { - listTypePush(subject,new_obj,REDIS_TAIL); + subject->ptr = ziplistPush(subject->ptr,value->ptr,sdslen(value->ptr),REDIS_TAIL); } else { - subject->ptr = ziplistInsert(subject->ptr,next,new_obj->ptr,sdslen(new_obj->ptr)); + subject->ptr = ziplistInsert(subject->ptr,next,value->ptr,sdslen(value->ptr)); } } else { - subject->ptr = ziplistInsert(subject->ptr,old_entry->zi,new_obj->ptr,sdslen(new_obj->ptr)); + subject->ptr = ziplistInsert(subject->ptr,entry->zi,value->ptr,sdslen(value->ptr)); } - } else if (subject->encoding == REDIS_ENCODING_LIST) { + } else if (entry->li->encoding == REDIS_ENCODING_LIST) { if (where == REDIS_TAIL) { - listInsertNode(subject->ptr,old_entry->ln,new_obj,AL_START_TAIL); + listInsertNode(subject->ptr,entry->ln,value,AL_START_TAIL); } else { - listInsertNode(subject->ptr,old_entry->ln,new_obj,AL_START_HEAD); + listInsertNode(subject->ptr,entry->ln,value,AL_START_HEAD); } - incrRefCount(new_obj); + incrRefCount(value); } else { redisPanic("Unknown list encoding"); } @@ -5205,29 +5208,34 @@ static void rpushCommand(redisClient *c) { pushGenericCommand(c,REDIS_TAIL); } -static void pushxGenericCommand(redisClient *c, int where, robj *old_obj, robj *new_obj) { +static void pushxGenericCommand(redisClient *c, robj *refval, robj *val, int where) { robj *subject; listTypeIterator *iter; listTypeEntry entry; if ((subject = lookupKeyReadOrReply(c,c->argv[1],shared.czero)) == NULL || checkType(c,subject,REDIS_LIST)) return; - if (handleClientsWaitingListPush(c,c->argv[1],new_obj)) { + if (handleClientsWaitingListPush(c,c->argv[1],val)) { addReply(c,shared.cone); return; } - if (old_obj != NULL) { + if (refval != NULL) { + /* Note: we expect refval to be string-encoded because it is *not* the + * last argument of the multi-bulk LINSERT. */ + redisAssert(refval->encoding == REDIS_ENCODING_RAW); + + /* Seek refval from head to tail */ iter = listTypeInitIterator(subject,0,REDIS_TAIL); while (listTypeNext(iter,&entry)) { - if (listTypeEqual(&entry,old_obj)) { - listTypeInsert(subject,&entry,new_obj,where); + if (listTypeEqual(&entry,refval)) { + listTypeInsert(&entry,val,where); break; } } listTypeReleaseIterator(iter); } else { - listTypePush(subject,new_obj,where); + listTypePush(subject,val,where); } server.dirty++; @@ -5235,18 +5243,18 @@ static void pushxGenericCommand(redisClient *c, int where, robj *old_obj, robj * } static void lpushxCommand(redisClient *c) { - pushxGenericCommand(c,REDIS_HEAD,NULL,c->argv[2]); + pushxGenericCommand(c,NULL,c->argv[2],REDIS_HEAD); } static void rpushxCommand(redisClient *c) { - pushxGenericCommand(c,REDIS_TAIL,NULL,c->argv[2]); + pushxGenericCommand(c,NULL,c->argv[2],REDIS_TAIL); } static void linsertCommand(redisClient *c) { if (strcasecmp(c->argv[2]->ptr,"after") == 0) { - pushxGenericCommand(c,REDIS_TAIL,c->argv[3],c->argv[4]); + pushxGenericCommand(c,c->argv[3],c->argv[4],REDIS_TAIL); } else if (strcasecmp(c->argv[2]->ptr,"before") == 0) { - pushxGenericCommand(c,REDIS_HEAD,c->argv[3],c->argv[4]); + pushxGenericCommand(c,c->argv[3],c->argv[4],REDIS_HEAD); } else { addReply(c,shared.syntaxerr); } From 244b873b0c753f126db88ef9fe15aefe4a44f558 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Fri, 11 Jun 2010 13:27:21 +0200 Subject: [PATCH 08/11] make sure the value to insert is string encoded --- redis.c | 2 ++ tests/unit/type/list.tcl | 4 ++++ 2 files changed, 6 insertions(+) diff --git a/redis.c b/redis.c index c39f5846..ca4d9f87 100644 --- a/redis.c +++ b/redis.c @@ -5091,6 +5091,7 @@ static robj *listTypeGet(listTypeEntry *entry) { static void listTypeInsert(listTypeEntry *entry, robj *value, int where) { robj *subject = entry->li->subject; if (entry->li->encoding == REDIS_ENCODING_ZIPLIST) { + value = getDecodedObject(value); if (where == REDIS_TAIL) { unsigned char *next = ziplistNext(subject->ptr,entry->zi); @@ -5104,6 +5105,7 @@ static void listTypeInsert(listTypeEntry *entry, robj *value, int where) { } else { subject->ptr = ziplistInsert(subject->ptr,entry->zi,value->ptr,sdslen(value->ptr)); } + decrRefCount(value); } else if (entry->li->encoding == REDIS_ENCODING_LIST) { if (where == REDIS_TAIL) { listInsertNode(subject->ptr,entry->ln,value,AL_START_TAIL); diff --git a/tests/unit/type/list.tcl b/tests/unit/type/list.tcl index 691040ec..81eabd3a 100644 --- a/tests/unit/type/list.tcl +++ b/tests/unit/type/list.tcl @@ -73,6 +73,8 @@ start_server { assert_equal 8 [r linsert xlist before a aa] assert_equal 8 [r linsert xlist before bad aaa] assert_equal {aa a b zz c yy d dd} [r lrange xlist 0 10] + assert_equal 9 [r linsert xlist before aa 42] + assert_equal 42 [r lrange xlist 0 0] } test {LPUSHX, RPUSHX, LPUSHXAFTER, RPUSHXAFTER - regular list} { @@ -98,6 +100,8 @@ start_server { assert_equal 8 [r linsert xlist before a aa] assert_equal 8 [r linsert xlist before bad aaa] assert_equal {aa a aaaaaaaaaaaaaaaaa zz c yy d dd} [r lrange xlist 0 10] + assert_equal 9 [r linsert xlist before aa 42] + assert_equal 42 [r lrange xlist 0 0] } test {DEL a list - ziplist} { From 70b4b320ae1d04dd087ae9336bdd962a9615fbfe Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Fri, 11 Jun 2010 14:51:59 +0200 Subject: [PATCH 09/11] check if the list encoding needs to be changed on LPUSHX, RPUSHX, LINSERT --- redis.c | 21 +++++- tests/unit/type/list.tcl | 135 ++++++++++++++++++++++++--------------- 2 files changed, 102 insertions(+), 54 deletions(-) diff --git a/redis.c b/redis.c index ca4d9f87..efaeb0f1 100644 --- a/redis.c +++ b/redis.c @@ -4926,7 +4926,7 @@ static void listTypePush(robj *subject, robj *value, int where) { /* Check if we need to convert the ziplist */ listTypeTryConversion(subject,value); if (subject->encoding == REDIS_ENCODING_ZIPLIST && - ziplistLen(subject->ptr) > server.list_max_ziplist_entries) + ziplistLen(subject->ptr) >= server.list_max_ziplist_entries) listTypeConvert(subject,REDIS_ENCODING_LIST); if (subject->encoding == REDIS_ENCODING_ZIPLIST) { @@ -5214,6 +5214,7 @@ static void pushxGenericCommand(redisClient *c, robj *refval, robj *val, int whe robj *subject; listTypeIterator *iter; listTypeEntry entry; + int inserted = 0; if ((subject = lookupKeyReadOrReply(c,c->argv[1],shared.czero)) == NULL || checkType(c,subject,REDIS_LIST)) return; @@ -5227,20 +5228,36 @@ static void pushxGenericCommand(redisClient *c, robj *refval, robj *val, int whe * last argument of the multi-bulk LINSERT. */ redisAssert(refval->encoding == REDIS_ENCODING_RAW); + /* We're not sure if this value can be inserted yet, but we cannot + * convert the list inside the iterator. We don't want to loop over + * the list twice (once to see if the value can be inserted and once + * to do the actual insert), so we assume this value can be inserted + * and convert the ziplist to a regular list if necessary. */ + listTypeTryConversion(subject,val); + /* Seek refval from head to tail */ iter = listTypeInitIterator(subject,0,REDIS_TAIL); while (listTypeNext(iter,&entry)) { if (listTypeEqual(&entry,refval)) { listTypeInsert(&entry,val,where); + inserted = 1; break; } } listTypeReleaseIterator(iter); + + if (inserted) { + /* Check if the length exceeds the ziplist length threshold. */ + if (subject->encoding == REDIS_ENCODING_ZIPLIST && + ziplistLen(subject->ptr) > server.list_max_ziplist_entries) + listTypeConvert(subject,REDIS_ENCODING_LIST); + server.dirty++; + } } else { listTypePush(subject,val,where); + server.dirty++; } - server.dirty++; addReplyUlong(c,listTypeLength(subject)); } diff --git a/tests/unit/type/list.tcl b/tests/unit/type/list.tcl index 81eabd3a..052cde0a 100644 --- a/tests/unit/type/list.tcl +++ b/tests/unit/type/list.tcl @@ -52,58 +52,6 @@ start_server { assert_equal $large_value [r lindex mylist2 2] } - test {LPUSHX, RPUSHX, LPUSHXAFTER, RPUSHXAFTER - ziplist} { - r del xlist - assert_equal 0 [r lpushx xlist a] - assert_equal 0 [r rpushx xlist a] - assert_equal 1 [r rpush xlist b] - assert_equal 2 [r rpush xlist c] - assert_equal 3 [r rpushx xlist d] - assert_equal 4 [r lpushx xlist a] - assert_encoding ziplist xlist - assert_equal {a b c d} [r lrange xlist 0 10] - - assert_equal 5 [r linsert xlist before c zz] - assert_equal {a b zz c d} [r lrange xlist 0 10] - assert_equal 6 [r linsert xlist after c yy] - assert_equal {a b zz c yy d} [r lrange xlist 0 10] - assert_equal 7 [r linsert xlist after d dd] - assert_equal 7 [r linsert xlist after bad ddd] - assert_equal {a b zz c yy d dd} [r lrange xlist 0 10] - assert_equal 8 [r linsert xlist before a aa] - assert_equal 8 [r linsert xlist before bad aaa] - assert_equal {aa a b zz c yy d dd} [r lrange xlist 0 10] - assert_equal 9 [r linsert xlist before aa 42] - assert_equal 42 [r lrange xlist 0 0] - } - - test {LPUSHX, RPUSHX, LPUSHXAFTER, RPUSHXAFTER - regular list} { - set large_value "aaaaaaaaaaaaaaaaa" - - r del xlist - assert_equal 0 [r lpushx xlist a] - assert_equal 0 [r rpushx xlist a] - assert_equal 1 [r rpush xlist $large_value] - assert_equal 2 [r rpush xlist c] - assert_equal 3 [r rpushx xlist d] - assert_equal 4 [r lpushx xlist a] - assert_encoding list xlist - assert_equal {a aaaaaaaaaaaaaaaaa c d} [r lrange xlist 0 10] - - assert_equal 5 [r linsert xlist before c zz] - assert_equal {a aaaaaaaaaaaaaaaaa zz c d} [r lrange xlist 0 10] - assert_equal 6 [r linsert xlist after c yy] - assert_equal {a aaaaaaaaaaaaaaaaa zz c yy d} [r lrange xlist 0 10] - assert_equal 7 [r linsert xlist after d dd] - assert_equal 7 [r linsert xlist after bad ddd] - assert_equal {a aaaaaaaaaaaaaaaaa zz c yy d dd} [r lrange xlist 0 10] - assert_equal 8 [r linsert xlist before a aa] - assert_equal 8 [r linsert xlist before bad aaa] - assert_equal {aa a aaaaaaaaaaaaaaaaa zz c yy d dd} [r lrange xlist 0 10] - assert_equal 9 [r linsert xlist before aa 42] - assert_equal 42 [r lrange xlist 0 0] - } - test {DEL a list - ziplist} { assert_equal 1 [r del myziplist2] assert_equal 0 [r exists myziplist2] @@ -130,6 +78,89 @@ start_server { assert_encoding list $key } + test {LPUSHX, RPUSHX - generic} { + r del xlist + assert_equal 0 [r lpushx xlist a] + assert_equal 0 [r llen xlist] + assert_equal 0 [r rpushx xlist a] + assert_equal 0 [r llen xlist] + } + + foreach type {ziplist list} { + test "LPUSHX, RPUSHX - $type" { + create_$type xlist {b c} + assert_equal 3 [r rpushx xlist d] + assert_equal 4 [r lpushx xlist a] + assert_equal {a b c d} [r lrange xlist 0 -1] + } + + test "LINSERT - $type" { + create_$type xlist {a b c d} + assert_equal 5 [r linsert xlist before c zz] + assert_equal {a b zz c d} [r lrange xlist 0 10] + assert_equal 6 [r linsert xlist after c yy] + assert_equal {a b zz c yy d} [r lrange xlist 0 10] + assert_equal 7 [r linsert xlist after d dd] + assert_equal 7 [r linsert xlist after bad ddd] + assert_equal {a b zz c yy d dd} [r lrange xlist 0 10] + assert_equal 8 [r linsert xlist before a aa] + assert_equal 8 [r linsert xlist before bad aaa] + assert_equal {aa a b zz c yy d dd} [r lrange xlist 0 10] + + # check inserting integer encoded value + assert_equal 9 [r linsert xlist before aa 42] + assert_equal 42 [r lrange xlist 0 0] + } + } + + test {LPUSHX, RPUSHX convert from ziplist to list} { + set large_value "aaaaaaaaaaaaaaaaa" + + # convert when a large value is pushed + create_ziplist xlist a + assert_equal 2 [r rpushx xlist $large_value] + assert_encoding list xlist + create_ziplist xlist a + assert_equal 2 [r lpushx xlist $large_value] + assert_encoding list xlist + + # convert when the length threshold is exceeded + create_ziplist xlist [lrepeat 256 a] + assert_equal 257 [r rpushx xlist b] + assert_encoding list xlist + create_ziplist xlist [lrepeat 256 a] + assert_equal 257 [r lpushx xlist b] + assert_encoding list xlist + } + + test {LINSERT convert from ziplist to list} { + set large_value "aaaaaaaaaaaaaaaaa" + + # convert when a large value is inserted + create_ziplist xlist a + assert_equal 2 [r linsert xlist before a $large_value] + assert_encoding list xlist + create_ziplist xlist a + assert_equal 2 [r linsert xlist after a $large_value] + assert_encoding list xlist + + # convert when the length threshold is exceeded + create_ziplist xlist [lrepeat 256 a] + assert_equal 257 [r linsert xlist before a a] + assert_encoding list xlist + create_ziplist xlist [lrepeat 256 a] + assert_equal 257 [r linsert xlist after a a] + assert_encoding list xlist + + # don't convert when the value could not be inserted + create_ziplist xlist [lrepeat 256 a] + assert_equal 256 [r linsert xlist before foo a] + assert_encoding ziplist xlist + create_ziplist xlist [lrepeat 256 a] + assert_equal 256 [r linsert xlist after foo a] + assert_encoding ziplist xlist + } + foreach {type num} {ziplist 250 list 500} { proc check_numbered_list_consistency {key} { set len [r llen $key] From 23d3a5feef0572762cac979f70accbae7ff1eda0 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Fri, 11 Jun 2010 17:34:23 +0200 Subject: [PATCH 10/11] make LINSERT return -1 when the value could not be inserted --- redis.c | 7 ++++++- tests/unit/type/list.tcl | 8 ++++---- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/redis.c b/redis.c index efaeb0f1..b563c4d5 100644 --- a/redis.c +++ b/redis.c @@ -538,7 +538,7 @@ typedef struct zset { #define REDIS_SHARED_INTEGERS 10000 struct sharedObjectsStruct { - robj *crlf, *ok, *err, *emptybulk, *czero, *cone, *pong, *space, + robj *crlf, *ok, *err, *emptybulk, *czero, *cone, *cnegone, *pong, *space, *colon, *nullbulk, *nullmultibulk, *queued, *emptymultibulk, *wrongtypeerr, *nokeyerr, *syntaxerr, *sameobjecterr, *outofrangeerr, *plus, @@ -1677,6 +1677,7 @@ static void createSharedObjects(void) { shared.emptybulk = createObject(REDIS_STRING,sdsnew("$0\r\n\r\n")); shared.czero = createObject(REDIS_STRING,sdsnew(":0\r\n")); shared.cone = createObject(REDIS_STRING,sdsnew(":1\r\n")); + shared.cnegone = createObject(REDIS_STRING,sdsnew(":-1\r\n")); shared.nullbulk = createObject(REDIS_STRING,sdsnew("$-1\r\n")); shared.nullmultibulk = createObject(REDIS_STRING,sdsnew("*-1\r\n")); shared.emptymultibulk = createObject(REDIS_STRING,sdsnew("*0\r\n")); @@ -5252,6 +5253,10 @@ static void pushxGenericCommand(redisClient *c, robj *refval, robj *val, int whe ziplistLen(subject->ptr) > server.list_max_ziplist_entries) listTypeConvert(subject,REDIS_ENCODING_LIST); server.dirty++; + } else { + /* Notify client of a failed insert */ + addReply(c,shared.cnegone); + return; } } else { listTypePush(subject,val,where); diff --git a/tests/unit/type/list.tcl b/tests/unit/type/list.tcl index 052cde0a..1d69a88f 100644 --- a/tests/unit/type/list.tcl +++ b/tests/unit/type/list.tcl @@ -101,10 +101,10 @@ start_server { assert_equal 6 [r linsert xlist after c yy] assert_equal {a b zz c yy d} [r lrange xlist 0 10] assert_equal 7 [r linsert xlist after d dd] - assert_equal 7 [r linsert xlist after bad ddd] + assert_equal -1 [r linsert xlist after bad ddd] assert_equal {a b zz c yy d dd} [r lrange xlist 0 10] assert_equal 8 [r linsert xlist before a aa] - assert_equal 8 [r linsert xlist before bad aaa] + assert_equal -1 [r linsert xlist before bad aaa] assert_equal {aa a b zz c yy d dd} [r lrange xlist 0 10] # check inserting integer encoded value @@ -154,10 +154,10 @@ start_server { # don't convert when the value could not be inserted create_ziplist xlist [lrepeat 256 a] - assert_equal 256 [r linsert xlist before foo a] + assert_equal -1 [r linsert xlist before foo a] assert_encoding ziplist xlist create_ziplist xlist [lrepeat 256 a] - assert_equal 256 [r linsert xlist after foo a] + assert_equal -1 [r linsert xlist after foo a] assert_encoding ziplist xlist } From 7d288d654793e867020b73d101c17d25bfbc256c Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Fri, 11 Jun 2010 17:35:48 +0200 Subject: [PATCH 11/11] LPUSHX, RPUSHX, LINSERT only work on non-empty lists, so there are no clients waiting for a push --- redis.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/redis.c b/redis.c index b563c4d5..2b0e5d37 100644 --- a/redis.c +++ b/redis.c @@ -5219,10 +5219,6 @@ static void pushxGenericCommand(redisClient *c, robj *refval, robj *val, int whe if ((subject = lookupKeyReadOrReply(c,c->argv[1],shared.czero)) == NULL || checkType(c,subject,REDIS_LIST)) return; - if (handleClientsWaitingListPush(c,c->argv[1],val)) { - addReply(c,shared.cone); - return; - } if (refval != NULL) { /* Note: we expect refval to be string-encoded because it is *not* the