From c4705381422ead4ad99f4b7a3bc11f059c460401 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Fri, 13 Aug 2010 19:28:49 +0200 Subject: [PATCH 01/20] Make ziplist schema more efficient for strings with length > 15 --- src/ziplist.c | 227 ++++++++++++++++++++++++++++++++------------------ 1 file changed, 144 insertions(+), 83 deletions(-) diff --git a/src/ziplist.c b/src/ziplist.c index 7a3a8b01..a6383517 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) << 6) | 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; } @@ -198,11 +257,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 +274,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 +293,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 +314,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; } @@ -327,7 +386,7 @@ static unsigned char *__ziplistInsert(unsigned char *zl, unsigned char *p, unsig 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; @@ -344,11 +403,13 @@ static unsigned char *__ziplistInsert(unsigned char *zl, unsigned char *p, unsig /* 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); @@ -380,10 +441,10 @@ static unsigned char *__ziplistInsert(unsigned char *zl, unsigned char *p, unsig /* 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; @@ -463,7 +524,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 +571,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; @@ -562,7 +623,7 @@ void ziplistRepr(unsigned char *zl) { entry = zipEntry(p); printf("{offset %ld, header %u, payload %u} ",p-zl,entry.headersize,entry.len); p += entry.headersize; - if (entry.encoding == ZIP_ENC_RAW) { + if (ZIP_IS_STR(entry.encoding)) { fwrite(p,entry.len,1,stdout); } else { printf("%lld", (long long) zipLoadInteger(p,entry.encoding)); From 169d2ef1e0259945e667a33db7944947a6b047a0 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Mon, 6 Sep 2010 23:12:41 +0200 Subject: [PATCH 02/20] Fix updating the prevlen field of consecutive entries In the condition where the prevlen field of the next entry on insert and delete operations needs more bytes to be properly encoded, the next entry also needs to be updated with a new prevlen. This patch makes sure that this effect cascades throughout the ziplist. --- src/ziplist.c | 221 +++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 208 insertions(+), 13 deletions(-) diff --git a/src/ziplist.c b/src/ziplist.c index a6383517..f1069e41 100644 --- a/src/ziplist.c +++ b/src/ziplist.c @@ -226,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) { @@ -344,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++; @@ -365,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); @@ -375,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; } @@ -385,19 +482,18 @@ 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 = 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); } } @@ -429,15 +525,32 @@ 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); @@ -510,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; } } @@ -615,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 (ZIP_IS_STR(entry.encoding)) { - fwrite(p,entry.len,1,stdout); + 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"); } @@ -1019,7 +1164,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"); From 84403fe7c1ab582c1fff4ddb5d933ba1a5f61759 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Tue, 7 Sep 2010 00:08:42 +0200 Subject: [PATCH 03/20] Allow a random seed argument for the ziplist test binary --- src/ziplist.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/ziplist.c b/src/ziplist.c index f1069e41..5254423d 100644 --- a/src/ziplist.c +++ b/src/ziplist.c @@ -870,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); From 34a719d25034d6f1140a10eb0429bdee0efa5cd9 Mon Sep 17 00:00:00 2001 From: antirez Date: Fri, 17 Sep 2010 15:25:32 +0200 Subject: [PATCH 04/20] try to parse the request in a smarter way to gain speed... work in progress --- src/networking.c | 13 +++++++++++-- src/redis.h | 1 + 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/networking.c b/src/networking.c index 867032aa..aaf7518f 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) { @@ -672,13 +674,14 @@ again: if (c->flags & REDIS_BLOCKED || c->flags & REDIS_IO_WAIT) return; 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 +768,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.h b/src/redis.h index 38727ae2..e6166f8b 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 */ From a4f3f93b90c7cc688ffff665914bdadc224847fc Mon Sep 17 00:00:00 2001 From: antirez Date: Fri, 17 Sep 2010 16:05:01 +0200 Subject: [PATCH 05/20] new parsing code bugfixing --- src/networking.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/networking.c b/src/networking.c index aaf7518f..632fd047 100644 --- a/src/networking.c +++ b/src/networking.c @@ -664,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 @@ -672,6 +674,9 @@ 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 */ size_t querylen; From 5ca2f0c49894878be47161f667ae0daf70bb6fd3 Mon Sep 17 00:00:00 2001 From: antirez Date: Wed, 22 Sep 2010 16:09:30 +0200 Subject: [PATCH 06/20] preventive conflict resolution to merge pietern/zset-mem --- src/t_zset.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/t_zset.c b/src/t_zset.c index d944e923..e3eb8325 100644 --- a/src/t_zset.c +++ b/src/t_zset.c @@ -355,7 +355,8 @@ void zaddGenericCommand(redisClient *c, robj *key, robj *ele, double scoreval, i *score = scoreval; } if (isnan(*score)) { - addReplyError(c,"resulting score is not a number (NaN)"); + addReplySds(c, + sdsnew("-ERR resulting score is not a number (NaN)\r\n")); zfree(score); /* Note that we don't need to check if the zset may be empty and * should be removed here, as we can only obtain Nan as score if From beb7756dcbb44099352abcb3368fcd3d23b55782 Mon Sep 17 00:00:00 2001 From: antirez Date: Wed, 22 Sep 2010 16:10:13 +0200 Subject: [PATCH 07/20] error generation format reverted to the new style after merge --- src/t_zset.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/t_zset.c b/src/t_zset.c index eeb8dab3..93ade5aa 100644 --- a/src/t_zset.c +++ b/src/t_zset.c @@ -327,8 +327,7 @@ void zaddGenericCommand(redisClient *c, robj *key, robj *ele, double score, int score += *(double*)dictGetEntryVal(de); if (isnan(score)) { - addReplySds(c, - sdsnew("-ERR resulting score is not a number (NaN)\r\n")); + addReplyError(c,"resulting score is not a number (NaN)"); /* Note that we don't need to check if the zset may be empty and * should be removed here, as we can only obtain Nan as score if * there was already an element in the sorted set. */ From 50a9fad5d50488592447dc599a9ef6a184088ee3 Mon Sep 17 00:00:00 2001 From: antirez Date: Wed, 22 Sep 2010 17:49:04 +0200 Subject: [PATCH 08/20] two leaks fixed --- src/t_zset.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/src/t_zset.c b/src/t_zset.c index 93ade5aa..114c95d6 100644 --- a/src/t_zset.c +++ b/src/t_zset.c @@ -625,25 +625,23 @@ void zunionInterGenericCommand(redisClient *c, robj *dstkey, int op) { * from small to large, all src[i > 0].dict are non-empty too */ di = dictGetIterator(src[0].dict); while((de = dictNext(di)) != NULL) { - double *score = zmalloc(sizeof(double)), value; - *score = src[0].weight * zunionInterDictValue(de); + double score, value; + score = src[0].weight * zunionInterDictValue(de); for (j = 1; j < setnum; j++) { dictEntry *other = dictFind(src[j].dict,dictGetEntryKey(de)); if (other) { value = src[j].weight * zunionInterDictValue(other); - zunionInterAggregate(score, value, aggregate); + zunionInterAggregate(&score, value, aggregate); } else { break; } } - /* skip entry when not present in every source dict */ - if (j != setnum) { - zfree(score); - } else { + /* accept entry only when present in every source dict */ + if (j == setnum) { robj *o = dictGetEntryKey(de); - znode = zslInsert(dstzset->zsl,*score,o); + znode = zslInsert(dstzset->zsl,score,o); incrRefCount(o); /* added to skiplist */ dictAdd(dstzset->dict,o,&znode->score); incrRefCount(o); /* added to dictionary */ @@ -657,11 +655,12 @@ void zunionInterGenericCommand(redisClient *c, robj *dstkey, int op) { di = dictGetIterator(src[i].dict); while((de = dictNext(di)) != NULL) { - /* skip key when already processed */ - if (dictFind(dstzset->dict,dictGetEntryKey(de)) != NULL) continue; + double score, value; - double *score = zmalloc(sizeof(double)), value; - *score = src[i].weight * zunionInterDictValue(de); + /* skip key when already processed */ + if (dictFind(dstzset->dict,dictGetEntryKey(de)) != NULL) + continue; + score = src[i].weight * zunionInterDictValue(de); /* because the zsets are sorted by size, its only possible * for sets at larger indices to hold this entry */ @@ -669,12 +668,12 @@ void zunionInterGenericCommand(redisClient *c, robj *dstkey, int op) { dictEntry *other = dictFind(src[j].dict,dictGetEntryKey(de)); if (other) { value = src[j].weight * zunionInterDictValue(other); - zunionInterAggregate(score, value, aggregate); + zunionInterAggregate(&score, value, aggregate); } } robj *o = dictGetEntryKey(de); - znode = zslInsert(dstzset->zsl,*score,o); + znode = zslInsert(dstzset->zsl,score,o); incrRefCount(o); /* added to skiplist */ dictAdd(dstzset->dict,o,&znode->score); incrRefCount(o); /* added to dictionary */ From 56e52b69feebb11931cbe8162ce1749909b7ff30 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Wed, 22 Sep 2010 18:07:52 +0200 Subject: [PATCH 09/20] Update rdb.c to properly work with new memory strategy for sorted sets --- src/rdb.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/rdb.c b/src/rdb.c index c15fc6f2..a401a5b9 100644 --- a/src/rdb.c +++ b/src/rdb.c @@ -730,13 +730,14 @@ robj *rdbLoadObject(int type, FILE *fp) { /* Load every single element of the list/set */ while(zsetlen--) { robj *ele; - double *score = zmalloc(sizeof(double)); + double score; + zskiplistNode *znode; if ((ele = rdbLoadEncodedStringObject(fp)) == NULL) return NULL; ele = tryObjectEncoding(ele); - if (rdbLoadDoubleValue(fp,score) == -1) return NULL; - dictAdd(zs->dict,ele,score); - zslInsert(zs->zsl,*score,ele); + if (rdbLoadDoubleValue(fp,&score) == -1) return NULL; + znode = zslInsert(zs->zsl,score,ele); + dictAdd(zs->dict,ele,&znode->score); incrRefCount(ele); /* added to skiplist */ } } else if (type == REDIS_HASH) { From 136cf53f22539b33396247d356e7e9d077068ccf Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 23 Sep 2010 16:05:17 +0200 Subject: [PATCH 10/20] minimal C test framework + a first example sds.c tests --- src/sds.c | 30 ++++++++++++++++++++++++---- src/testhelp.h | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 4 deletions(-) create mode 100644 src/testhelp.h diff --git a/src/sds.c b/src/sds.c index 2f3ffedc..cfe94af8 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,22 @@ err: if (current) sdsfree(current); return NULL; } + +#ifdef SDS_TEST_MAIN +#include +#include "testhelp.h" + +int main(void) { + { + sds x = sdsnew("foo"); + + /* SDS creation and length */ + test_cond("Can create a string and obtain the length", + sdslen(x) == 3 && memcmp(x,"foo",3) == 0) + + /* Nul term checking */ + test_cond("The string contains the nul term", x[3] == '\0') + } + test_report() +} +#endif 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 From 963238f713f02e538cf0f5851f3337173116ea39 Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 23 Sep 2010 16:39:02 +0200 Subject: [PATCH 11/20] more tests for sds.c --- src/sds.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 77 insertions(+), 6 deletions(-) diff --git a/src/sds.c b/src/sds.c index cfe94af8..2d063c4a 100644 --- a/src/sds.c +++ b/src/sds.c @@ -488,14 +488,85 @@ err: int main(void) { { - sds x = sdsnew("foo"); + sds x = sdsnew("foo"), y; - /* SDS creation and length */ - test_cond("Can create a string and obtain the length", - sdslen(x) == 3 && memcmp(x,"foo",3) == 0) + test_cond("Create a string and obtain the length", + sdslen(x) == 3 && memcmp(x,"foo\0",4) == 0) - /* Nul term checking */ - test_cond("The string contains the nul term", x[3] == '\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() } From 30d31cc8bb416f67183a218f1511ef517eb9ae3b Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 23 Sep 2010 18:24:47 +0200 Subject: [PATCH 12/20] Contributing file added --- CONTRIBUTING | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 CONTRIBUTING 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! From b0d605c1d6bbf5746cc957946138108b928c88a1 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Thu, 23 Sep 2010 22:04:19 +0200 Subject: [PATCH 13/20] Add regression test and fix for >255 byte string entries --- src/ziplist.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/src/ziplist.c b/src/ziplist.c index 5254423d..4f44bd58 100644 --- a/src/ziplist.c +++ b/src/ziplist.c @@ -144,7 +144,7 @@ static unsigned int zipDecodeLength(unsigned char *p, unsigned int *lensize) { if (lensize) *lensize = 1; break; case ZIP_STR_14B: - len = ((p[0] & 0x3f) << 6) | p[1]; + len = ((p[0] & 0x3f) << 8) | p[1]; if (lensize) *lensize = 2; break; case ZIP_STR_32B: @@ -1125,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(); From 1a06bf93c4de5016c746eab0d9d0255a458693b7 Mon Sep 17 00:00:00 2001 From: antirez Date: Fri, 24 Sep 2010 10:30:15 +0200 Subject: [PATCH 14/20] ziplist implementation fuzzy tests --- tests/unit/type/list.tcl | 53 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/tests/unit/type/list.tcl b/tests/unit/type/list.tcl index bf188fd7..c4a3b217 100644 --- a/tests/unit/type/list.tcl +++ b/tests/unit/type/list.tcl @@ -603,5 +603,58 @@ start_server { assert_equal 1 [r lrem myotherlist 1 2] assert_equal 3 [r llen myotherlist] } + + 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] + } + } + } + } + } } } From ef27ba988b10a632057b04ee3abb287cbe87322d Mon Sep 17 00:00:00 2001 From: antirez Date: Fri, 24 Sep 2010 10:37:00 +0200 Subject: [PATCH 15/20] explicit regression test for a ziplist bug added --- tests/unit/type/list.tcl | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/unit/type/list.tcl b/tests/unit/type/list.tcl index c4a3b217..e609865c 100644 --- a/tests/unit/type/list.tcl +++ b/tests/unit/type/list.tcl @@ -604,6 +604,15 @@ start_server { assert_equal 3 [r llen myotherlist] } + 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} { From 4610b0332c199563e58de005eb1b10ed54e1ec9c Mon Sep 17 00:00:00 2001 From: antirez Date: Fri, 24 Sep 2010 11:15:06 +0200 Subject: [PATCH 16/20] intset stress testing added, ziplist stress testing relocated in a more appropriate place --- tests/unit/type/list.tcl | 109 +++++++++++++++++++++------------------ tests/unit/type/set.tcl | 36 +++++++++++++ 2 files changed, 95 insertions(+), 50 deletions(-) diff --git a/tests/unit/type/list.tcl b/tests/unit/type/list.tcl index e609865c..4c131fc3 100644 --- a/tests/unit/type/list.tcl +++ b/tests/unit/type/list.tcl @@ -604,63 +604,72 @@ start_server { assert_equal 3 [r llen myotherlist] } - 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 +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] } - 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] - } + 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 - } + 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] - } + } + 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 + } + } + } } From fdc0bde9359d12dd53b686a6db9c66cf0001670f Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 7 Oct 2010 12:49:14 +0200 Subject: [PATCH 17/20] minor typo fixed, reported by Thomas Bassetto --- src/redis-cli.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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; From 144a5e72f2adc9d151438569f680533e41778504 Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 7 Oct 2010 16:21:35 +0200 Subject: [PATCH 18/20] fixed an alignment problem with time_t is 32 bit, long is 64 bit, and arch is sparc or any other where unaligned accesses will result to sigbus --- src/db.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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; From 1dd10ca233a533928ec2ff83f675499e4e7c1ff1 Mon Sep 17 00:00:00 2001 From: antirez Date: Mon, 11 Oct 2010 13:05:09 +0200 Subject: [PATCH 19/20] maxmemory fixed, we now try to release memory just before we check for the memory limit. Before fixing there was code between the attempt to free memory and the check for memory limits, and this code could result into allocations going again after the memory limit. --- src/redis.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/redis.c b/src/redis.c index 7b1b3f4f..27a855d9 100644 --- a/src/redis.c +++ b/src/redis.c @@ -889,9 +889,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 @@ -1029,7 +1026,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) { From b4f2e412d087bae0a523fe6ea40fcad30fe74b5b Mon Sep 17 00:00:00 2001 From: antirez Date: Mon, 11 Oct 2010 16:46:21 +0200 Subject: [PATCH 20/20] free memory if the maxmemory parameter is reduced via CONFIG SET --- src/config.c | 1 + 1 file changed, 1 insertion(+) 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;