diff --git a/CONTRIBUTING b/CONTRIBUTING new file mode 100644 index 00000000..644b5991 --- /dev/null +++ b/CONTRIBUTING @@ -0,0 +1,13 @@ +1. Enter irc.freenode.org #redis and start talking with 'antirez' and/or 'pietern' to check if there is interest for such a feature and to understand the probability of it being merged. We'll try hard to keep Redis simple... so you'll likely encounter an high resistence. + +2. Drop a message to the Redis Google Group with a proposal of semantics/API. + +3. If steps 1 and 2 are ok, use the following procedure to submit a patch: + + a. Fork Redis on github + b. Create a topic branch (git checkout -b my_branch) + c. Push to your branch (git push origin my_branch) + d. Create an issue in the Redis google code site with a link to your patch + e. Done :) + +Thanks! diff --git a/src/config.c b/src/config.c index 8a5ad6c2..1bd678c7 100644 --- a/src/config.c +++ b/src/config.c @@ -241,6 +241,7 @@ void configSetCommand(redisClient *c) { if (getLongLongFromObject(o,&ll) == REDIS_ERR || ll < 0) goto badfmt; server.maxmemory = ll; + if (server.maxmemory) freeMemoryIfNeeded(); } else if (!strcasecmp(c->argv[2]->ptr,"timeout")) { if (getLongLongFromObject(o,&ll) == REDIS_ERR || ll < 0 || ll > LONG_MAX) goto badfmt; diff --git a/src/db.c b/src/db.c index 470310a3..44507847 100644 --- a/src/db.c +++ b/src/db.c @@ -478,7 +478,7 @@ int expireIfNeeded(redisDb *db, robj *key) { void expireGenericCommand(redisClient *c, robj *key, robj *param, long offset) { dictEntry *de; - time_t seconds; + long seconds; if (getLongFromObjectOrReply(c, param, &seconds, NULL) != REDIS_OK) return; diff --git a/src/networking.c b/src/networking.c index 867032aa..632fd047 100644 --- a/src/networking.c +++ b/src/networking.c @@ -28,6 +28,7 @@ redisClient *createClient(int fd) { selectDb(c,0); c->fd = fd; c->querybuf = sdsempty(); + c->newline = NULL; c->argc = 0; c->argv = NULL; c->bulklen = -1; @@ -631,6 +632,7 @@ void resetClient(redisClient *c) { freeClientArgv(c); c->bulklen = -1; c->multibulk = 0; + c->newline = NULL; } void closeTimedoutClients(void) { @@ -662,6 +664,8 @@ void closeTimedoutClients(void) { } void processInputBuffer(redisClient *c) { + int seeknewline = 0; + again: /* Before to process the input buffer, make sure the client is not * waitig for a blocking operation such as BLPOP. Note that the first @@ -670,15 +674,19 @@ again: * in the input buffer the client may be blocked, and the "goto again" * will try to reiterate. The following line will make it return asap. */ if (c->flags & REDIS_BLOCKED || c->flags & REDIS_IO_WAIT) return; + + if (seeknewline && c->bulklen == -1) c->newline = strchr(c->querybuf,'\n'); + seeknewline = 1; if (c->bulklen == -1) { /* Read the first line of the query */ - char *p = strchr(c->querybuf,'\n'); size_t querylen; - if (p) { + if (c->newline) { + char *p = c->newline; sds query, *argv; int argc, j; + c->newline = NULL; query = c->querybuf; c->querybuf = sdsempty(); querylen = 1+(p-(query)); @@ -765,8 +773,14 @@ void readQueryFromClient(aeEventLoop *el, int fd, void *privdata, int mask) { return; } if (nread) { + size_t oldlen = sdslen(c->querybuf); c->querybuf = sdscatlen(c->querybuf, buf, nread); c->lastinteraction = time(NULL); + /* Scan this new piece of the query for the newline. We do this + * here in order to make sure we perform this scan just one time + * per piece of buffer, leading to an O(N) scan instead of O(N*N) */ + if (c->bulklen == -1 && c->newline == NULL) + c->newline = strchr(c->querybuf+oldlen,'\n'); } else { return; } diff --git a/src/redis-cli.c b/src/redis-cli.c index 0e6edbe7..5071604b 100644 --- a/src/redis-cli.c +++ b/src/redis-cli.c @@ -364,7 +364,7 @@ static int parseOptions(int argc, char **argv) { "automatically used as last argument.\n" ); } else if (!strcmp(argv[i],"-v")) { - printf("redis-cli shipped with Redis verison %s\n", REDIS_VERSION); + printf("redis-cli shipped with Redis version %s\n", REDIS_VERSION); exit(0); } else { break; diff --git a/src/redis.c b/src/redis.c index fca5f1b3..8f208926 100644 --- a/src/redis.c +++ b/src/redis.c @@ -890,9 +890,6 @@ void call(redisClient *c, struct redisCommand *cmd) { int processCommand(redisClient *c) { struct redisCommand *cmd; - /* Free some memory if needed (maxmemory setting) */ - if (server.maxmemory) freeMemoryIfNeeded(); - /* Handle the multi bulk command type. This is an alternative protocol * supported by Redis in order to receive commands that are composed of * multiple binary-safe "bulk" arguments. The latency of processing is @@ -1030,7 +1027,12 @@ int processCommand(redisClient *c) { return 1; } - /* Handle the maxmemory directive */ + /* Handle the maxmemory directive. + * + * First we try to free some memory if possible (if there are volatile + * keys in the dataset). If there are not the only thing we can do + * is returning an error. */ + if (server.maxmemory) freeMemoryIfNeeded(); if (server.maxmemory && (cmd->flags & REDIS_CMD_DENYOOM) && zmalloc_used_memory() > server.maxmemory) { diff --git a/src/redis.h b/src/redis.h index d53ccf5b..24dfb9b5 100644 --- a/src/redis.h +++ b/src/redis.h @@ -286,6 +286,7 @@ typedef struct redisClient { int dictid; sds querybuf; robj **argv, **mbargv; + char *newline; /* pointing to the detected newline in querybuf */ int argc, mbargc; long bulklen; /* bulk read len. -1 if not in bulk read mode */ int multibulk; /* multi bulk command format active */ diff --git a/src/sds.c b/src/sds.c index 2f3ffedc..2d063c4a 100644 --- a/src/sds.c +++ b/src/sds.c @@ -223,13 +223,16 @@ sds sdsrange(sds s, int start, int end) { } newlen = (start > end) ? 0 : (end-start)+1; if (newlen != 0) { - if (start >= (signed)len) start = len-1; - if (end >= (signed)len) end = len-1; - newlen = (start > end) ? 0 : (end-start)+1; + if (start >= (signed)len) { + newlen = 0; + } else if (end >= (signed)len) { + end = len-1; + newlen = (start > end) ? 0 : (end-start)+1; + } } else { start = 0; } - if (start != 0) memmove(sh->buf, sh->buf+start, newlen); + if (start && newlen) memmove(sh->buf, sh->buf+start, newlen); sh->buf[newlen] = 0; sh->free = sh->free+(sh->len-newlen); sh->len = newlen; @@ -478,3 +481,93 @@ err: if (current) sdsfree(current); return NULL; } + +#ifdef SDS_TEST_MAIN +#include +#include "testhelp.h" + +int main(void) { + { + sds x = sdsnew("foo"), y; + + test_cond("Create a string and obtain the length", + sdslen(x) == 3 && memcmp(x,"foo\0",4) == 0) + + sdsfree(x); + x = sdsnewlen("foo",2); + test_cond("Create a string with specified length", + sdslen(x) == 2 && memcmp(x,"fo\0",3) == 0) + + x = sdscat(x,"bar"); + test_cond("Strings concatenation", + sdslen(x) == 5 && memcmp(x,"fobar\0",6) == 0); + + x = sdscpy(x,"a"); + test_cond("sdscpy() against an originally longer string", + sdslen(x) == 1 && memcmp(x,"a\0",2) == 0) + + x = sdscpy(x,"xyzxxxxxxxxxxyyyyyyyyyykkkkkkkkkk"); + test_cond("sdscpy() against an originally shorter string", + sdslen(x) == 33 && + memcmp(x,"xyzxxxxxxxxxxyyyyyyyyyykkkkkkkkkk\0",33) == 0) + + sdsfree(x); + x = sdscatprintf(sdsempty(),"%d",123); + test_cond("sdscatprintf() seems working in the base case", + sdslen(x) == 3 && memcmp(x,"123\0",4) ==0) + + sdsfree(x); + x = sdstrim(sdsnew("xxciaoyyy"),"xy"); + test_cond("sdstrim() correctly trims characters", + sdslen(x) == 4 && memcmp(x,"ciao\0",5) == 0) + + y = sdsrange(sdsdup(x),1,1); + test_cond("sdsrange(...,1,1)", + sdslen(y) == 1 && memcmp(y,"i\0",2) == 0) + + sdsfree(y); + y = sdsrange(sdsdup(x),1,-1); + test_cond("sdsrange(...,1,-1)", + sdslen(y) == 3 && memcmp(y,"iao\0",4) == 0) + + sdsfree(y); + y = sdsrange(sdsdup(x),-2,-1); + test_cond("sdsrange(...,-2,-1)", + sdslen(y) == 2 && memcmp(y,"ao\0",3) == 0) + + sdsfree(y); + y = sdsrange(sdsdup(x),2,1); + test_cond("sdsrange(...,2,1)", + sdslen(y) == 0 && memcmp(y,"\0",1) == 0) + + sdsfree(y); + y = sdsrange(sdsdup(x),1,100); + test_cond("sdsrange(...,1,100)", + sdslen(y) == 3 && memcmp(y,"iao\0",4) == 0) + + sdsfree(y); + y = sdsrange(sdsdup(x),100,100); + test_cond("sdsrange(...,100,100)", + sdslen(y) == 0 && memcmp(y,"\0",1) == 0) + + sdsfree(y); + sdsfree(x); + x = sdsnew("foo"); + y = sdsnew("foa"); + test_cond("sdscmp(foo,foa)", sdscmp(x,y) > 0) + + sdsfree(y); + sdsfree(x); + x = sdsnew("bar"); + y = sdsnew("bar"); + test_cond("sdscmp(bar,bar)", sdscmp(x,y) == 0) + + sdsfree(y); + sdsfree(x); + x = sdsnew("aar"); + y = sdsnew("bar"); + test_cond("sdscmp(bar,bar)", sdscmp(x,y) < 0) + } + test_report() +} +#endif diff --git a/src/t_zset.c b/src/t_zset.c index 5df8f288..1a67febc 100644 --- a/src/t_zset.c +++ b/src/t_zset.c @@ -664,8 +664,8 @@ void zunionInterGenericCommand(redisClient *c, robj *dstkey, int op) { di = dictGetIterator(src[0].dict); while((de = dictNext(di)) != NULL) { double score, value; - score = src[0].weight * zunionInterDictValue(de); + score = src[0].weight * zunionInterDictValue(de); for (j = 1; j < setnum; j++) { dictEntry *other = dictFind(src[j].dict,dictGetEntryKey(de)); if (other) { diff --git a/src/testhelp.h b/src/testhelp.h new file mode 100644 index 00000000..d699f2ae --- /dev/null +++ b/src/testhelp.h @@ -0,0 +1,54 @@ +/* This is a really minimal testing framework for C. + * + * Example: + * + * test_cond("Check if 1 == 1", 1==1) + * test_cond("Check if 5 > 10", 5 > 10) + * test_report() + * + * Copyright (c) 2010, Salvatore Sanfilippo + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * * Redistributions of source code must retain the above copyright notice, + * this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * * Neither the name of Redis nor the names of its contributors may be used + * to endorse or promote products derived from this software without + * specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE + * POSSIBILITY OF SUCH DAMAGE. + */ + +#ifndef __TESTHELP_H +#define __TESTHELP_H + +int __failed_tests = 0; +int __test_num = 0; +#define test_cond(descr,_c) do { \ + __test_num++; printf("%d - %s: ", __test_num, descr); \ + if(_c) printf("PASSED\n"); else {printf("FAILED\n"); __failed_tests++;} \ +} while(0); +#define test_report() do { \ + printf("%d tests, %d passed, %d failed\n", __test_num, \ + __test_num-__failed_tests, __failed_tests); \ + if (__failed_tests) { \ + printf("=== WARNING === We have failed tests here...\n"); \ + } \ +} while(0); + +#endif diff --git a/src/ziplist.c b/src/ziplist.c index 7a3a8b01..4f44bd58 100644 --- a/src/ziplist.c +++ b/src/ziplist.c @@ -1,17 +1,63 @@ -/* Memory layout of a ziplist, containing "foo", "bar", "quux": - * "foo""bar""quux" +/* The ziplist is a specially encoded dually linked list that is designed + * to be very memory efficient. It stores both strings and integer values, + * where integers are encoded as actual integers instead of a series of + * characters. It allows push and pop operations on either side of the list + * in O(1) time. However, because every operation requires a reallocation of + * the memory used by the ziplist, the actual complexity is related to the + * amount of memory used by the ziplist. * - * is an unsigned integer to hold the number of bytes that - * the ziplist occupies. This is stored to not have to traverse the ziplist - * to know the new length when pushing. + * ---------------------------------------------------------------------------- * - * is the number of items in the ziplist. When this value is - * greater than 254, we need to traverse the entire list to know - * how many items it holds. + * ZIPLIST OVERALL LAYOUT: + * The general layout of the ziplist is as follows: + * * - * is the number of bytes occupied by a single entry. When this - * number is greater than 253, the length will occupy 5 bytes, where - * the extra bytes contain an unsigned integer to hold the length. + * is an unsigned integer to hold the number of bytes that the + * ziplist occupies. This value needs to be stored to be able to resize the + * entire structure without the need to traverse it first. + * + * is the offset to the last entry in the list. This allows a pop + * operation on the far side of the list without the need for full traversal. + * + * is the number of entries.When this value is larger than 2**16-2, + * we need to traverse the entire list to know how many items it holds. + * + * is a single byte special value, equal to 255, which indicates the + * end of the list. + * + * ZIPLIST ENTRIES: + * Every entry in the ziplist is prefixed by a header that contains two pieces + * of information. First, the length of the previous entry is stored to be + * able to traverse the list from back to front. Second, the encoding with an + * optional string length of the entry itself is stored. + * + * The length of the previous entry is encoded in the following way: + * If this length is smaller than 254 bytes, it will only consume a single + * byte that takes the length as value. When the length is greater than or + * equal to 254, it will consume 5 bytes. The first byte is set to 254 to + * indicate a larger value is following. The remaining 4 bytes take the + * length of the previous entry as value. + * + * The other header field of the entry itself depends on the contents of the + * entry. When the entry is a string, the first 2 bits of this header will hold + * the type of encoding used to store the length of the string, followed by the + * actual length of the string. When the entry is an integer the first 2 bits + * are both set to 1. The following 2 bits are used to specify what kind of + * integer will be stored after this header. An overview of the different + * types and encodings is as follows: + * + * |00pppppp| - 1 byte + * String value with length less than or equal to 63 bytes (6 bits). + * |01pppppp|qqqqqqqq| - 2 bytes + * String value with length less than or equal to 16383 bytes (14 bits). + * |10______|qqqqqqqq|rrrrrrrr|ssssssss|tttttttt| - 5 bytes + * String value with length greater than or equal to 16384 bytes. + * |1100____| - 1 byte + * Integer encoded as int16_t (2 bytes). + * |1101____| - 1 byte + * Integer encoded as int32_t (4 bytes). + * |1110____| - 1 byte + * Integer encoded as int64_t (8 bytes). */ #include @@ -25,25 +71,20 @@ int ll2string(char *s, size_t len, long long value); -/* Important note: the ZIP_END value is used to depict the end of the - * ziplist structure. When a pointer contains an entry, the first couple - * of bytes contain the encoded length of the previous entry. This length - * is encoded as ZIP_ENC_RAW length, so the first two bits will contain 00 - * and the byte will therefore never have a value of 255. */ #define ZIP_END 255 #define ZIP_BIGLEN 254 -/* Entry encoding */ -#define ZIP_ENC_RAW 0 -#define ZIP_ENC_INT16 1 -#define ZIP_ENC_INT32 2 -#define ZIP_ENC_INT64 3 -#define ZIP_ENCODING(p) ((p)[0] >> 6) +/* Different encoding/length possibilities */ +#define ZIP_STR_06B (0 << 6) +#define ZIP_STR_14B (1 << 6) +#define ZIP_STR_32B (2 << 6) +#define ZIP_INT_16B (0xc0 | 0<<4) +#define ZIP_INT_32B (0xc0 | 1<<4) +#define ZIP_INT_64B (0xc0 | 2<<4) -/* Length encoding for raw entries */ -#define ZIP_LEN_INLINE 0 -#define ZIP_LEN_UINT16 1 -#define ZIP_LEN_UINT32 2 +/* Macro's to determine type */ +#define ZIP_IS_STR(enc) (((enc) & 0xc0) < 0xc0) +#define ZIP_IS_INT(enc) (!ZIP_IS_STR(enc) && ((enc) & 0x30) < 0x30) /* Utility macros */ #define ZIPLIST_BYTES(zl) (*((uint32_t*)(zl))) @@ -67,14 +108,25 @@ typedef struct zlentry { unsigned char *p; } zlentry; +/* Return the encoding pointer to by 'p'. */ +static unsigned int zipEntryEncoding(unsigned char *p) { + /* String encoding: 2 MSBs */ + unsigned char b = p[0] & 0xc0; + if (b < 0xc0) { + return b; + } else { + /* Integer encoding: 4 MSBs */ + return p[0] & 0xf0; + } + assert(NULL); +} + /* Return bytes needed to store integer encoded by 'encoding' */ -static unsigned int zipEncodingSize(unsigned char encoding) { - if (encoding == ZIP_ENC_INT16) { - return sizeof(int16_t); - } else if (encoding == ZIP_ENC_INT32) { - return sizeof(int32_t); - } else if (encoding == ZIP_ENC_INT64) { - return sizeof(int64_t); +static unsigned int zipIntSize(unsigned char encoding) { + switch(encoding) { + case ZIP_INT_16B: return sizeof(int16_t); + case ZIP_INT_32B: return sizeof(int32_t); + case ZIP_INT_64B: return sizeof(int64_t); } assert(NULL); } @@ -82,23 +134,28 @@ static unsigned int zipEncodingSize(unsigned char encoding) { /* Decode the encoded length pointed by 'p'. If a pointer to 'lensize' is * provided, it is set to the number of bytes required to encode the length. */ static unsigned int zipDecodeLength(unsigned char *p, unsigned int *lensize) { - unsigned char encoding = ZIP_ENCODING(p), lenenc; + unsigned char encoding = zipEntryEncoding(p); unsigned int len; - if (encoding == ZIP_ENC_RAW) { - lenenc = (p[0] >> 4) & 0x3; - if (lenenc == ZIP_LEN_INLINE) { - len = p[0] & 0xf; + if (ZIP_IS_STR(encoding)) { + switch(encoding) { + case ZIP_STR_06B: + len = p[0] & 0x3f; if (lensize) *lensize = 1; - } else if (lenenc == ZIP_LEN_UINT16) { - len = p[1] | (p[2] << 8); - if (lensize) *lensize = 3; - } else { - len = p[1] | (p[2] << 8) | (p[3] << 16) | (p[4] << 24); + break; + case ZIP_STR_14B: + len = ((p[0] & 0x3f) << 8) | p[1]; + if (lensize) *lensize = 2; + break; + case ZIP_STR_32B: + len = (p[1] << 24) | (p[2] << 16) | (p[3] << 8) | p[4]; if (lensize) *lensize = 5; + break; + default: + assert(NULL); } } else { - len = zipEncodingSize(encoding); + len = zipIntSize(encoding); if (lensize) *lensize = 1; } return len; @@ -106,34 +163,36 @@ static unsigned int zipDecodeLength(unsigned char *p, unsigned int *lensize) { /* Encode the length 'l' writing it in 'p'. If p is NULL it just returns * the amount of bytes required to encode such a length. */ -static unsigned int zipEncodeLength(unsigned char *p, char encoding, unsigned int rawlen) { - unsigned char len = 1, lenenc, buf[5]; - if (encoding == ZIP_ENC_RAW) { - if (rawlen <= 0xf) { +static unsigned int zipEncodeLength(unsigned char *p, unsigned char encoding, unsigned int rawlen) { + unsigned char len = 1, buf[5]; + + if (ZIP_IS_STR(encoding)) { + /* Although encoding is given it may not be set for strings, + * so we determine it here using the raw length. */ + if (rawlen <= 0x3f) { if (!p) return len; - lenenc = ZIP_LEN_INLINE; - buf[0] = rawlen; - } else if (rawlen <= 0xffff) { - len += 2; + buf[0] = ZIP_STR_06B | rawlen; + } else if (rawlen <= 0x3fff) { + len += 1; if (!p) return len; - lenenc = ZIP_LEN_UINT16; - buf[1] = (rawlen ) & 0xff; - buf[2] = (rawlen >> 8) & 0xff; + buf[0] = ZIP_STR_14B | ((rawlen >> 8) & 0x3f); + buf[1] = rawlen & 0xff; } else { len += 4; if (!p) return len; - lenenc = ZIP_LEN_UINT32; - buf[1] = (rawlen ) & 0xff; - buf[2] = (rawlen >> 8) & 0xff; - buf[3] = (rawlen >> 16) & 0xff; - buf[4] = (rawlen >> 24) & 0xff; + buf[0] = ZIP_STR_32B; + buf[1] = (rawlen >> 24) & 0xff; + buf[2] = (rawlen >> 16) & 0xff; + buf[3] = (rawlen >> 8) & 0xff; + buf[4] = rawlen & 0xff; } - buf[0] = (lenenc << 4) | (buf[0] & 0xf); + } else { + /* Implies integer encoding, so length is always 1. */ + if (!p) return len; + buf[0] = encoding; } - if (!p) return len; - /* Apparently we need to store the length in 'p' */ - buf[0] = (encoding << 6) | (buf[0] & 0x3f); + /* Store this length at p */ memcpy(p,buf,len); return len; } @@ -167,6 +226,14 @@ static unsigned int zipPrevEncodeLength(unsigned char *p, unsigned int len) { } } +/* Encode the length of the previous entry and write it to "p". This only + * uses the larger encoding (required in __ziplistCascadeUpdate). */ +static void zipPrevEncodeLengthForceLarge(unsigned char *p, unsigned int len) { + if (p == NULL) return; + p[0] = ZIP_BIGLEN; + memcpy(p+1,&len,sizeof(len)); +} + /* Return the difference in number of bytes needed to store the new length * "len" on the entry pointed to by "p". */ static int zipPrevLenByteDiff(unsigned char *p, unsigned int len) { @@ -198,11 +265,11 @@ static int zipTryEncoding(unsigned char *entry, unsigned int entrylen, long long /* Great, the string can be encoded. Check what's the smallest * of our encoding types that can hold this value. */ if (value >= INT16_MIN && value <= INT16_MAX) { - *encoding = ZIP_ENC_INT16; + *encoding = ZIP_INT_16B; } else if (value >= INT32_MIN && value <= INT32_MAX) { - *encoding = ZIP_ENC_INT32; + *encoding = ZIP_INT_32B; } else { - *encoding = ZIP_ENC_INT64; + *encoding = ZIP_INT_64B; } *v = value; return 1; @@ -215,13 +282,13 @@ static void zipSaveInteger(unsigned char *p, int64_t value, unsigned char encodi int16_t i16; int32_t i32; int64_t i64; - if (encoding == ZIP_ENC_INT16) { + if (encoding == ZIP_INT_16B) { i16 = value; memcpy(p,&i16,sizeof(i16)); - } else if (encoding == ZIP_ENC_INT32) { + } else if (encoding == ZIP_INT_32B) { i32 = value; memcpy(p,&i32,sizeof(i32)); - } else if (encoding == ZIP_ENC_INT64) { + } else if (encoding == ZIP_INT_64B) { i64 = value; memcpy(p,&i64,sizeof(i64)); } else { @@ -234,13 +301,13 @@ static int64_t zipLoadInteger(unsigned char *p, unsigned char encoding) { int16_t i16; int32_t i32; int64_t i64, ret; - if (encoding == ZIP_ENC_INT16) { + if (encoding == ZIP_INT_16B) { memcpy(&i16,p,sizeof(i16)); ret = i16; - } else if (encoding == ZIP_ENC_INT32) { + } else if (encoding == ZIP_INT_32B) { memcpy(&i32,p,sizeof(i32)); ret = i32; - } else if (encoding == ZIP_ENC_INT64) { + } else if (encoding == ZIP_INT_64B) { memcpy(&i64,p,sizeof(i64)); ret = i64; } else { @@ -255,7 +322,7 @@ static zlentry zipEntry(unsigned char *p) { e.prevrawlen = zipPrevDecodeLength(p,&e.prevrawlensize); e.len = zipDecodeLength(p+e.prevrawlensize,&e.lensize); e.headersize = e.prevrawlensize+e.lensize; - e.encoding = ZIP_ENCODING(p+e.prevrawlensize); + e.encoding = zipEntryEncoding(p+e.prevrawlensize); e.p = p; return e; } @@ -285,11 +352,86 @@ static unsigned char *ziplistResize(unsigned char *zl, unsigned int len) { return zl; } +/* When an entry is inserted, we need to set the prevlen field of the next + * entry to equal the length of the inserted entry. It can occur that this + * length cannot be encoded in 1 byte and the next entry needs to be grow + * a bit larger to hold the 5-byte encoded prevlen. This can be done for free, + * because this only happens when an entry is already being inserted (which + * causes a realloc and memmove). However, encoding the prevlen may require + * that this entry is grown as well. This effect may cascade throughout + * the ziplist when there are consecutive entries with a size close to + * ZIP_BIGLEN, so we need to check that the prevlen can be encoded in every + * consecutive entry. + * + * Note that this effect can also happen in reverse, where the bytes required + * to encode the prevlen field can shrink. This effect is deliberately ignored, + * because it can cause a "flapping" effect where a chain prevlen fields is + * first grown and then shrunk again after consecutive inserts. Rather, the + * field is allowed to stay larger than necessary, because a large prevlen + * field implies the ziplist is holding large entries anyway. + * + * The pointer "p" points to the first entry that does NOT need to be + * updated, i.e. consecutive fields MAY need an update. */ +static unsigned char *__ziplistCascadeUpdate(unsigned char *zl, unsigned char *p) { + unsigned int curlen = ZIPLIST_BYTES(zl), rawlen, rawlensize; + unsigned int offset, noffset, extra; + unsigned char *np; + zlentry cur, next; + + while (p[0] != ZIP_END) { + cur = zipEntry(p); + rawlen = cur.headersize + cur.len; + rawlensize = zipPrevEncodeLength(NULL,rawlen); + + /* Abort if there is no next entry. */ + if (p[rawlen] == ZIP_END) break; + next = zipEntry(p+rawlen); + + /* Abort when "prevlen" has not changed. */ + if (next.prevrawlen == rawlen) break; + + if (next.prevrawlensize < rawlensize) { + /* The "prevlen" field of "next" needs more bytes to hold + * the raw length of "cur". */ + offset = p-zl; + extra = rawlensize-next.prevrawlensize; + zl = ziplistResize(zl,curlen+extra); + ZIPLIST_TAIL_OFFSET(zl) += extra; + p = zl+offset; + + /* Move the tail to the back. */ + np = p+rawlen; + noffset = np-zl; + memmove(np+rawlensize, + np+next.prevrawlensize, + curlen-noffset-next.prevrawlensize-1); + zipPrevEncodeLength(np,rawlen); + + /* Advance the cursor */ + p += rawlen; + } else { + if (next.prevrawlensize > rawlensize) { + /* This would result in shrinking, which we want to avoid. + * So, set "rawlen" in the available bytes. */ + zipPrevEncodeLengthForceLarge(p+rawlen,rawlen); + } else { + zipPrevEncodeLength(p+rawlen,rawlen); + } + + /* Stop here, as the raw length of "next" has not changed. */ + break; + } + } + return zl; +} + /* Delete "num" entries, starting at "p". Returns pointer to the ziplist. */ static unsigned char *__ziplistDelete(unsigned char *zl, unsigned char *p, unsigned int num) { unsigned int i, totlen, deleted = 0; - int nextdiff = 0; - zlentry first = zipEntry(p); + int offset, nextdiff = 0; + zlentry first, tail; + + first = zipEntry(p); for (i = 0; p[0] != ZIP_END && i < num; i++) { p += zipRawEntryLength(p); deleted++; @@ -306,7 +448,14 @@ static unsigned char *__ziplistDelete(unsigned char *zl, unsigned char *p, unsig zipPrevEncodeLength(p-nextdiff,first.prevrawlen); /* Update offset for tail */ - ZIPLIST_TAIL_OFFSET(zl) -= totlen+nextdiff; + ZIPLIST_TAIL_OFFSET(zl) -= totlen; + + /* When the tail contains more than one entry, we need to take + * "nextdiff" in account as well. Otherwise, a change in the + * size of prevlen doesn't have an effect on the *tail* offset. */ + tail = zipEntry(p); + if (p[tail.headersize+tail.len] != ZIP_END) + ZIPLIST_TAIL_OFFSET(zl) += nextdiff; /* Move tail to the front of the ziplist */ memmove(first.p,p-nextdiff,ZIPLIST_BYTES(zl)-(p-zl)-1+nextdiff); @@ -316,8 +465,15 @@ static unsigned char *__ziplistDelete(unsigned char *zl, unsigned char *p, unsig } /* Resize and update length */ + offset = first.p-zl; zl = ziplistResize(zl, ZIPLIST_BYTES(zl)-totlen+nextdiff); ZIPLIST_INCR_LENGTH(zl,-deleted); + p = zl+offset; + + /* When nextdiff != 0, the raw length of the next entry has changed, so + * we need to cascade the update throughout the ziplist */ + if (nextdiff != 0) + zl = __ziplistCascadeUpdate(zl,p); } return zl; } @@ -326,29 +482,30 @@ static unsigned char *__ziplistDelete(unsigned char *zl, unsigned char *p, unsig static unsigned char *__ziplistInsert(unsigned char *zl, unsigned char *p, unsigned char *s, unsigned int slen) { unsigned int curlen = ZIPLIST_BYTES(zl), reqlen, prevlen = 0; unsigned int offset, nextdiff = 0; - unsigned char *tail; - unsigned char encoding = ZIP_ENC_RAW; + unsigned char encoding = 0; long long value; - zlentry entry; + zlentry entry, tail; /* Find out prevlen for the entry that is inserted. */ if (p[0] != ZIP_END) { entry = zipEntry(p); prevlen = entry.prevrawlen; } else { - tail = ZIPLIST_ENTRY_TAIL(zl); - if (tail[0] != ZIP_END) { - prevlen = zipRawEntryLength(tail); + unsigned char *ptail = ZIPLIST_ENTRY_TAIL(zl); + if (ptail[0] != ZIP_END) { + prevlen = zipRawEntryLength(ptail); } } /* See if the entry can be encoded */ if (zipTryEncoding(s,slen,&value,&encoding)) { - reqlen = zipEncodingSize(encoding); + /* 'encoding' is set to the appropriate integer encoding */ + reqlen = zipIntSize(encoding); } else { + /* 'encoding' is untouched, however zipEncodeLength will use the + * string length to figure out how to encode it. */ reqlen = slen; } - /* We need space for both the length of the previous entry and * the length of the payload. */ reqlen += zipPrevEncodeLength(NULL,prevlen); @@ -368,22 +525,39 @@ static unsigned char *__ziplistInsert(unsigned char *zl, unsigned char *p, unsig if (p[0] != ZIP_END) { /* Subtract one because of the ZIP_END bytes */ memmove(p+reqlen,p-nextdiff,curlen-offset-1+nextdiff); + /* Encode this entry's raw length in the next entry. */ zipPrevEncodeLength(p+reqlen,reqlen); + /* Update offset for tail */ - ZIPLIST_TAIL_OFFSET(zl) += reqlen+nextdiff; + ZIPLIST_TAIL_OFFSET(zl) += reqlen; + + /* When the tail contains more than one entry, we need to take + * "nextdiff" in account as well. Otherwise, a change in the + * size of prevlen doesn't have an effect on the *tail* offset. */ + tail = zipEntry(p+reqlen); + if (p[reqlen+tail.headersize+tail.len] != ZIP_END) + ZIPLIST_TAIL_OFFSET(zl) += nextdiff; } else { /* This element will be the new tail. */ ZIPLIST_TAIL_OFFSET(zl) = p-zl; } + /* When nextdiff != 0, the raw length of the next entry has changed, so + * we need to cascade the update throughout the ziplist */ + if (nextdiff != 0) { + offset = p-zl; + zl = __ziplistCascadeUpdate(zl,p+reqlen); + p = zl+offset; + } + /* Write the entry */ p += zipPrevEncodeLength(p,prevlen); p += zipEncodeLength(p,encoding,slen); - if (encoding != ZIP_ENC_RAW) { - zipSaveInteger(p,value,encoding); - } else { + if (ZIP_IS_STR(encoding)) { memcpy(p,s,slen); + } else { + zipSaveInteger(p,value,encoding); } ZIPLIST_INCR_LENGTH(zl,1); return zl; @@ -449,6 +623,7 @@ unsigned char *ziplistPrev(unsigned char *zl, unsigned char *p) { return NULL; } else { entry = zipEntry(p); + assert(entry.prevrawlen > 0); return p-entry.prevrawlen; } } @@ -463,7 +638,7 @@ unsigned int ziplistGet(unsigned char *p, unsigned char **sstr, unsigned int *sl if (sstr) *sstr = NULL; entry = zipEntry(p); - if (entry.encoding == ZIP_ENC_RAW) { + if (ZIP_IS_STR(entry.encoding)) { if (sstr) { *slen = entry.len; *sstr = p+entry.headersize; @@ -510,7 +685,7 @@ unsigned int ziplistCompare(unsigned char *p, unsigned char *sstr, unsigned int if (p[0] == ZIP_END) return 0; entry = zipEntry(p); - if (entry.encoding == ZIP_ENC_RAW) { + if (ZIP_IS_STR(entry.encoding)) { /* Raw compare */ if (entry.len == slen) { return memcmp(p+entry.headersize,sstr,slen) == 0; @@ -554,21 +729,52 @@ unsigned int ziplistSize(unsigned char *zl) { void ziplistRepr(unsigned char *zl) { unsigned char *p; + int index = 0; zlentry entry; - printf("{total bytes %d} {length %u}\n",ZIPLIST_BYTES(zl), ZIPLIST_LENGTH(zl)); + printf( + "{total bytes %d} " + "{length %u}\n" + "{tail offset %u}\n", + ZIPLIST_BYTES(zl), + ZIPLIST_LENGTH(zl), + ZIPLIST_TAIL_OFFSET(zl)); p = ZIPLIST_ENTRY_HEAD(zl); while(*p != ZIP_END) { entry = zipEntry(p); - printf("{offset %ld, header %u, payload %u} ",p-zl,entry.headersize,entry.len); + printf( + "{" + "addr 0x%08lx, " + "index %2d, " + "offset %5ld, " + "rl: %5u, " + "hs %2u, " + "pl: %5u, " + "pls: %2u, " + "payload %5u" + "} ", + (long unsigned int)p, + index, + p-zl, + entry.headersize+entry.len, + entry.headersize, + entry.prevrawlen, + entry.prevrawlensize, + entry.len); p += entry.headersize; - if (entry.encoding == ZIP_ENC_RAW) { - fwrite(p,entry.len,1,stdout); + if (ZIP_IS_STR(entry.encoding)) { + if (entry.len > 40) { + fwrite(p,40,1,stdout); + printf("..."); + } else { + fwrite(p,entry.len,1,stdout); + } } else { printf("%lld", (long long) zipLoadInteger(p,entry.encoding)); } printf("\n"); p += entry.len; + index++; } printf("{end}\n\n"); } @@ -664,6 +870,10 @@ int main(int argc, char **argv) { unsigned int elen; long long value; + /* If an argument is given, use it as the random seed. */ + if (argc == 2) + srand(atoi(argv[1])); + zl = createIntList(); ziplistRepr(zl); @@ -915,6 +1125,25 @@ int main(int argc, char **argv) { ziplistRepr(zl); } + printf("Regression test for >255 byte strings:\n"); + { + char v1[257],v2[257]; + memset(v1,'x',256); + memset(v2,'y',256); + zl = ziplistNew(); + zl = ziplistPush(zl,(unsigned char*)v1,strlen(v1),ZIPLIST_TAIL); + zl = ziplistPush(zl,(unsigned char*)v2,strlen(v2),ZIPLIST_TAIL); + + /* Pop values again and compare their value. */ + p = ziplistIndex(zl,0); + assert(ziplistGet(p,&entry,&elen,&value)); + assert(strncmp(v1,entry,elen) == 0); + p = ziplistIndex(zl,1); + assert(ziplistGet(p,&entry,&elen,&value)); + assert(strncmp(v2,entry,elen) == 0); + printf("SUCCESS\n\n"); + } + printf("Create long list and check indices:\n"); { zl = ziplistNew(); @@ -958,7 +1187,57 @@ int main(int argc, char **argv) { printf("ERROR: \"1025\"\n"); return 1; } - printf("SUCCESS\n"); + printf("SUCCESS\n\n"); + } + + printf("Stress with random payloads of different encoding:\n"); + { + int i, idx, where, len; + long long v; + unsigned char *p; + char buf[0x4041]; /* max length of generated string */ + zl = ziplistNew(); + for (i = 0; i < 100000; i++) { + where = (rand() & 1) ? ZIPLIST_HEAD : ZIPLIST_TAIL; + if (rand() & 1) { + /* equally likely create a 16, 32 or 64 bit int */ + v = (rand() & INT16_MAX) + ((1ll << 32) >> ((rand() % 3)*16)); + v *= 2*(rand() & 1)-1; /* randomly flip sign */ + sprintf(buf, "%lld", v); + zl = ziplistPush(zl, (unsigned char*)buf, strlen(buf), where); + } else { + /* equally likely generate 6, 14 or >14 bit length */ + v = rand() & 0x3f; + v += 0x4000 >> ((rand() % 3)*8); + memset(buf, 'x', v); + zl = ziplistPush(zl, (unsigned char*)buf, v, where); + } + + /* delete a random element */ + if ((len = ziplistLen(zl)) >= 10) { + idx = rand() % len; + // printf("Delete index %d\n", idx); + // ziplistRepr(zl); + ziplistDeleteRange(zl, idx, 1); + // ziplistRepr(zl); + len--; + } + + /* iterate from front to back */ + idx = 0; + p = ziplistIndex(zl, 0); + while((p = ziplistNext(zl,p))) + idx++; + assert(len == idx+1); + + /* iterate from back to front */ + idx = 0; + p = ziplistIndex(zl, -1); + while((p = ziplistPrev(zl,p))) + idx++; + assert(len == idx+1); + } + printf("SUCCESS\n\n"); } printf("Stress with variable ziplist size:\n"); diff --git a/tests/unit/type/list.tcl b/tests/unit/type/list.tcl index bf188fd7..4c131fc3 100644 --- a/tests/unit/type/list.tcl +++ b/tests/unit/type/list.tcl @@ -603,5 +603,76 @@ start_server { assert_equal 1 [r lrem myotherlist 1 2] assert_equal 3 [r llen myotherlist] } + + } +} + +start_server { + tags {list ziplist} + overrides { + "list-max-ziplist-value" 200000 + "list-max-ziplist-entries" 256 + } +} { + test {Explicit regression for a list bug} { + set mylist {49376042582 {BkG2o\pIC]4YYJa9cJ4GWZalG[4tin;1D2whSkCOW`mX;SFXGyS8sedcff3fQI^tgPCC@^Nu1J6o]meM@Lko]t_jRyotK?tH[\EvWqS]b`o2OCtjg:?nUTwdjpcUm]y:pg5q24q7LlCOwQE^}} + r del l + r rpush l [lindex $mylist 0] + r rpush l [lindex $mylist 1] + assert_equal [r lindex l 0] [lindex $mylist 0] + assert_equal [r lindex l 1] [lindex $mylist 1] + } + + tags {slow} { + test {ziplist implementation: value encoding and backlink} { + for {set j 0} {$j < 100} {incr j} { + r del l + set l {} + for {set i 0} {$i < 200} {incr i} { + randpath { + set data [string repeat x [randomInt 100000]] + } { + set data [randomInt 65536] + } { + set data [randomInt 4294967296] + } { + set data [randomInt 18446744073709551616] + } + lappend l $data + r rpush l $data + } + assert_equal [llength $l] [r llen l] + # Traverse backward + for {set i 199} {$i >= 0} {incr i -1} { + if {[lindex $l $i] ne [r lindex l $i]} { + assert_equal [lindex $l $i] [r lindex l $i] + } + } + } + } + + test {ziplist implementation: encoding stress testing} { + for {set j 0} {$j < 200} {incr j} { + r del l + set l {} + set len [randomInt 400] + for {set i 0} {$i < $len} {incr i} { + set rv [randomValue] + randpath { + lappend l $rv + r rpush l $rv + } { + set l [concat [list $rv] $l] + r lpush l $rv + } + } + assert_equal [llength $l] [r llen l] + for {set i 0} {$i < 200} {incr i} { + if {[lindex $l $i] ne [r lindex l $i]} { + assert_equal [lindex $l $i] [r lindex l $i] + } + } + } + } } } diff --git a/tests/unit/type/set.tcl b/tests/unit/type/set.tcl index 0f9f6abe..5608a648 100644 --- a/tests/unit/type/set.tcl +++ b/tests/unit/type/set.tcl @@ -295,4 +295,40 @@ start_server { r set x 10 assert_error "ERR*wrong kind*" {r smove myset2 x foo} } + + tags {slow} { + test {intsets implementation stress testing} { + for {set j 0} {$j < 20} {incr j} { + unset -nocomplain s + array set s {} + r del s + set len [randomInt 1024] + for {set i 0} {$i < $len} {incr i} { + randpath { + set data [randomInt 65536] + } { + set data [randomInt 4294967296] + } { + set data [randomInt 18446744073709551616] + } + set s($data) {} + r sadd s $data + } + assert_equal [lsort [r smembers s]] [lsort [array names s]] + set len [array size s] + for {set i 0} {$i < $len} {incr i} { + set e [r spop s] + if {![info exists s($e)]} { + puts "Can't find '$e' on local array" + puts "Local array: [lsort [r smembers s]]" + puts "Remote array: [lsort [array names s]]" + error "exception" + } + array unset s $e + } + assert_equal [r scard s] 0 + assert_equal [array size s] 0 + } + } + } }