From 43078ff844b14bb864e118d35795ddc3b3d18f00 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Sun, 28 Mar 2010 22:59:15 +0200 Subject: [PATCH 1/8] implemented strategy that doesn't use free blocks in zipmaps --- zipmap.c | 114 +++++++++++++++++++++++++------------------------------ 1 file changed, 52 insertions(+), 62 deletions(-) diff --git a/zipmap.c b/zipmap.c index f45ef0dd6..f168d0a75 100644 --- a/zipmap.c +++ b/zipmap.c @@ -149,43 +149,33 @@ static unsigned int zipmapEncodeLength(unsigned char *p, unsigned int len) { * (freelen is an integer pointer used both to signal the required length * and to get the reply from the function). If there is not a suitable * free space block to hold the requested bytes, *freelen is set to 0. */ -static unsigned char *zipmapLookupRaw(unsigned char *zm, unsigned char *key, unsigned int klen, unsigned int *totlen, unsigned int *freeoff, unsigned int *freelen) { - unsigned char *p = zm+1; +static unsigned char *zipmapLookupRaw(unsigned char *zm, unsigned char *key, unsigned int klen, unsigned int *totlen) { + unsigned char *p = zm+1, *k = NULL; unsigned int l; - unsigned int reqfreelen = 0; /* initialized just to prevent warning */ - if (freelen) { - reqfreelen = *freelen; - *freelen = 0; - assert(reqfreelen != 0); - } while(*p != ZIPMAP_END) { - if (*p == ZIPMAP_EMPTY) { - l = zipmapDecodeLength(p+1); - /* if the user want a free space report, and this space is - * enough, and we did't already found a suitable space... */ - if (freelen && l >= reqfreelen && *freelen == 0) { - *freelen = l; - *freeoff = p-zm; - } - p += l; - zm[0] |= ZIPMAP_STATUS_FRAGMENTED; - } else { - unsigned char free; + unsigned char free; - /* Match or skip the key */ - l = zipmapDecodeLength(p); - if (l == klen && !memcmp(p+1,key,l)) return p; - p += zipmapEncodeLength(NULL,l) + l; - /* Skip the value as well */ - l = zipmapDecodeLength(p); - p += zipmapEncodeLength(NULL,l); - free = p[0]; - p += l+1+free; /* +1 to skip the free byte */ + /* Match or skip the key */ + l = zipmapDecodeLength(p); + if (k == NULL && l == klen && !memcmp(p+1,key,l)) { + /* Only return when the user doesn't care + * for the total length of the zipmap. */ + if (totlen != NULL) { + k = p; + } else { + return p; + } } + p += zipmapEncodeLength(NULL,l) + l; + /* Skip the value as well */ + l = zipmapDecodeLength(p); + p += zipmapEncodeLength(NULL,l); + free = p[0]; + p += l+1+free; /* +1 to skip the free byte */ } if (totlen != NULL) *totlen = (unsigned int)(p-zm)+1; - return NULL; + return k; } static unsigned long zipmapRequiredLength(unsigned int klen, unsigned int vlen) { @@ -224,28 +214,29 @@ static unsigned int zipmapRawEntryLength(unsigned char *p) { return l + zipmapRawValueLength(p+l); } +static inline unsigned char *zipmapResize(unsigned char *zm, unsigned int len) { + zm = zrealloc(zm, len); + zm[len-1] = ZIPMAP_END; + return zm; +} + /* Set key to value, creating the key if it does not already exist. * If 'update' is not NULL, *update is set to 1 if the key was * already preset, otherwise to 0. */ unsigned char *zipmapSet(unsigned char *zm, unsigned char *key, unsigned int klen, unsigned char *val, unsigned int vlen, int *update) { - unsigned int oldlen = 0, freeoff = 0, freelen; - unsigned int reqlen = zipmapRequiredLength(klen,vlen); + unsigned int zmlen; + unsigned int freelen, reqlen = zipmapRequiredLength(klen,vlen); unsigned int empty, vempty; unsigned char *p; freelen = reqlen; if (update) *update = 0; - p = zipmapLookupRaw(zm,key,klen,&oldlen,&freeoff,&freelen); - if (p == NULL && freelen == 0) { - /* Key not found, and not space for the new key. Enlarge */ - zm = zrealloc(zm,oldlen+reqlen); - p = zm+oldlen-1; - zm[oldlen+reqlen-1] = ZIPMAP_END; - freelen = reqlen; - } else if (p == NULL) { - /* Key not found, but there is enough free space. */ - p = zm+freeoff; - /* note: freelen is already set in this case */ + p = zipmapLookupRaw(zm,key,klen,&zmlen); + if (p == NULL) { + /* Key not found: enlarge */ + zm = zipmapResize(zm, zmlen+reqlen); + p = zm+zmlen-1; + zmlen = zmlen+reqlen; } else { unsigned char *b = p; @@ -256,11 +247,14 @@ unsigned char *zipmapSet(unsigned char *zm, unsigned char *key, unsigned int kle b += freelen; freelen += zipmapRawValueLength(b); if (freelen < reqlen) { - /* Mark this entry as free and recurse */ - p[0] = ZIPMAP_EMPTY; - zipmapEncodeLength(p+1,freelen); - zm[0] |= ZIPMAP_STATUS_FRAGMENTED; - return zipmapSet(zm,key,klen,val,vlen,NULL); + /* Move remaining entries to the current position, so this + * pair can be appended. Note: the +1 in memmove is caused + * by the end-of-zipmap byte. */ + memmove(p, p+freelen, zmlen-((p-zm)+freelen+1)); + zm = zipmapResize(zm, zmlen-freelen+reqlen); + p = zm+zmlen-1-freelen; + zmlen = zmlen-1-freelen+reqlen; + freelen = reqlen; } } @@ -270,14 +264,11 @@ unsigned char *zipmapSet(unsigned char *zm, unsigned char *key, unsigned int kle /* If there is too much free space mark it as a free block instead * of adding it as trailing empty space for the value, as we want * zipmaps to be very space efficient. */ - if (empty > ZIPMAP_VALUE_MAX_FREE) { - unsigned char *e; - - e = p+reqlen; - e[0] = ZIPMAP_EMPTY; - zipmapEncodeLength(e+1,empty); + if (empty >= ZIPMAP_VALUE_MAX_FREE) { + memmove(p+reqlen, p+freelen, zmlen-((p-zm)+freelen+1)); + zmlen -= empty; + zm = zipmapResize(zm, zmlen); vempty = 0; - zm[0] |= ZIPMAP_STATUS_FRAGMENTED; } else { vempty = empty; } @@ -297,13 +288,12 @@ unsigned char *zipmapSet(unsigned char *zm, unsigned char *key, unsigned int kle /* Remove the specified key. If 'deleted' is not NULL the pointed integer is * set to 0 if the key was not found, to 1 if it was found and deleted. */ unsigned char *zipmapDel(unsigned char *zm, unsigned char *key, unsigned int klen, int *deleted) { - unsigned char *p = zipmapLookupRaw(zm,key,klen,NULL,NULL,NULL); + unsigned int zmlen; + unsigned char *p = zipmapLookupRaw(zm,key,klen,&zmlen); if (p) { unsigned int freelen = zipmapRawEntryLength(p); - - p[0] = ZIPMAP_EMPTY; - zipmapEncodeLength(p+1,freelen); - zm[0] |= ZIPMAP_STATUS_FRAGMENTED; + memmove(p, p+freelen, zmlen-((p-zm)+freelen+1)); + zm = zipmapResize(zm, zmlen-freelen); if (deleted) *deleted = 1; } else { if (deleted) *deleted = 0; @@ -351,7 +341,7 @@ unsigned char *zipmapNext(unsigned char *zm, unsigned char **key, unsigned int * int zipmapGet(unsigned char *zm, unsigned char *key, unsigned int klen, unsigned char **value, unsigned int *vlen) { unsigned char *p; - if ((p = zipmapLookupRaw(zm,key,klen,NULL,NULL,NULL)) == NULL) return 0; + if ((p = zipmapLookupRaw(zm,key,klen,NULL)) == NULL) return 0; p += zipmapRawKeyLength(p); *vlen = zipmapDecodeLength(p); *value = p + ZIPMAP_LEN_BYTES(*vlen) + 1; @@ -360,7 +350,7 @@ int zipmapGet(unsigned char *zm, unsigned char *key, unsigned int klen, unsigned /* Return 1 if the key exists, otherwise 0 is returned. */ int zipmapExists(unsigned char *zm, unsigned char *key, unsigned int klen) { - return zipmapLookupRaw(zm,key,klen,NULL,NULL,NULL) != NULL; + return zipmapLookupRaw(zm,key,klen,NULL) != NULL; } /* Return the number of entries inside a zipmap */ From 9e071b4bf489d02a4b6e307516f0376ddcf02d4d Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Sun, 28 Mar 2010 23:07:32 +0200 Subject: [PATCH 2/8] use first byte of zipmap to store length --- zipmap.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/zipmap.c b/zipmap.c index f168d0a75..8c7a79a8d 100644 --- a/zipmap.c +++ b/zipmap.c @@ -106,7 +106,7 @@ unsigned char *zipmapNew(void) { unsigned char *zm = zmalloc(2); - zm[0] = 0; /* Status */ + zm[0] = 0; /* Length */ zm[1] = ZIPMAP_END; return zm; } @@ -237,6 +237,9 @@ unsigned char *zipmapSet(unsigned char *zm, unsigned char *key, unsigned int kle zm = zipmapResize(zm, zmlen+reqlen); p = zm+zmlen-1; zmlen = zmlen+reqlen; + + /* Increase zipmap length (this is an insert) */ + if (zm[0] < ZIPMAP_BIGLEN) zm[0]++; } else { unsigned char *b = p; @@ -288,12 +291,16 @@ unsigned char *zipmapSet(unsigned char *zm, unsigned char *key, unsigned int kle /* Remove the specified key. If 'deleted' is not NULL the pointed integer is * set to 0 if the key was not found, to 1 if it was found and deleted. */ unsigned char *zipmapDel(unsigned char *zm, unsigned char *key, unsigned int klen, int *deleted) { - unsigned int zmlen; + unsigned int zmlen, freelen; unsigned char *p = zipmapLookupRaw(zm,key,klen,&zmlen); if (p) { - unsigned int freelen = zipmapRawEntryLength(p); + freelen = zipmapRawEntryLength(p); memmove(p, p+freelen, zmlen-((p-zm)+freelen+1)); zm = zipmapResize(zm, zmlen-freelen); + + /* Decrease zipmap length */ + if (zm[0] < ZIPMAP_BIGLEN) zm[0]--; + if (deleted) *deleted = 1; } else { if (deleted) *deleted = 0; @@ -355,10 +362,16 @@ int zipmapExists(unsigned char *zm, unsigned char *key, unsigned int klen) { /* Return the number of entries inside a zipmap */ unsigned int zipmapLen(unsigned char *zm) { - unsigned char *p = zipmapRewind(zm); unsigned int len = 0; + if (zm[0] < ZIPMAP_BIGLEN) { + len = zm[0]; + } else { + unsigned char *p = zipmapRewind(zm); + while((p = zipmapNext(p,NULL,NULL,NULL,NULL)) != NULL) len++; - while((p = zipmapNext(p,NULL,NULL,NULL,NULL)) != NULL) len++; + /* Re-store length if small enough */ + if (len < ZIPMAP_BIGLEN) zm[0] = len; + } return len; } From 381920793346413bc34dc87a31bc4ddf501fcdf8 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Sun, 28 Mar 2010 23:10:01 +0200 Subject: [PATCH 3/8] removed references in code to ZIPMAP_EMPTY --- zipmap.c | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/zipmap.c b/zipmap.c index 8c7a79a8d..7d28c202b 100644 --- a/zipmap.c +++ b/zipmap.c @@ -87,12 +87,9 @@ #include #include "zmalloc.h" -#define ZIPMAP_BIGLEN 253 -#define ZIPMAP_EMPTY 254 +#define ZIPMAP_BIGLEN 254 #define ZIPMAP_END 255 -#define ZIPMAP_STATUS_FRAGMENTED 1 - /* The following defines the max value for the field described in the * comments above, that is, the max number of trailing bytes in a value. */ #define ZIPMAP_VALUE_MAX_FREE 5 @@ -142,13 +139,7 @@ static unsigned int zipmapEncodeLength(unsigned char *p, unsigned int len) { * * If NULL is returned, and totlen is not NULL, it is set to the entire * size of the zimap, so that the calling function will be able to - * reallocate the original zipmap to make room for more entries. - * - * If NULL is returned, and freeoff and freelen are not NULL, they are set - * to the offset of the first empty space that can hold '*freelen' bytes - * (freelen is an integer pointer used both to signal the required length - * and to get the reply from the function). If there is not a suitable - * free space block to hold the requested bytes, *freelen is set to 0. */ + * reallocate the original zipmap to make room for more entries. */ static unsigned char *zipmapLookupRaw(unsigned char *zm, unsigned char *key, unsigned int klen, unsigned int *totlen) { unsigned char *p = zm+1, *k = NULL; unsigned int l; @@ -325,8 +316,6 @@ unsigned char *zipmapRewind(unsigned char *zm) { * } */ unsigned char *zipmapNext(unsigned char *zm, unsigned char **key, unsigned int *klen, unsigned char **value, unsigned int *vlen) { - while(zm[0] == ZIPMAP_EMPTY) - zm += zipmapDecodeLength(zm+1); if (zm[0] == ZIPMAP_END) return NULL; if (key) { *key = zm; @@ -383,10 +372,6 @@ void zipmapRepr(unsigned char *p) { if (p[0] == ZIPMAP_END) { printf("{end}"); break; - } else if (p[0] == ZIPMAP_EMPTY) { - l = zipmapDecodeLength(p+1); - printf("{%u empty block}", l); - p += l; } else { unsigned char e; From 06278a675853bc518a2613625d1b430af0523254 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Thu, 1 Apr 2010 12:58:08 +0200 Subject: [PATCH 4/8] use function to determine length of a single entry --- zipmap.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/zipmap.c b/zipmap.c index 7d28c202b..8a07b388e 100644 --- a/zipmap.c +++ b/zipmap.c @@ -201,7 +201,6 @@ static unsigned int zipmapRawValueLength(unsigned char *p) { * free space if any). */ static unsigned int zipmapRawEntryLength(unsigned char *p) { unsigned int l = zipmapRawKeyLength(p); - return l + zipmapRawValueLength(p+l); } @@ -232,14 +231,10 @@ unsigned char *zipmapSet(unsigned char *zm, unsigned char *key, unsigned int kle /* Increase zipmap length (this is an insert) */ if (zm[0] < ZIPMAP_BIGLEN) zm[0]++; } else { - unsigned char *b = p; - /* Key found. Is there enough space for the new value? */ /* Compute the total length: */ if (update) *update = 1; - freelen = zipmapRawKeyLength(b); - b += freelen; - freelen += zipmapRawValueLength(b); + freelen = zipmapRawEntryLength(p); if (freelen < reqlen) { /* Move remaining entries to the current position, so this * pair can be appended. Note: the +1 in memmove is caused From 8c6700720d7794bf9d526b6e2adbf73cc12207f0 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Thu, 1 Apr 2010 13:15:32 +0200 Subject: [PATCH 5/8] allow 4 free trailing bytes for each value --- zipmap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zipmap.c b/zipmap.c index 8a07b388e..5215954ad 100644 --- a/zipmap.c +++ b/zipmap.c @@ -92,7 +92,7 @@ /* The following defines the max value for the field described in the * comments above, that is, the max number of trailing bytes in a value. */ -#define ZIPMAP_VALUE_MAX_FREE 5 +#define ZIPMAP_VALUE_MAX_FREE 4 /* The following macro returns the number of bytes needed to encode the length * for the integer value _l, that is, 1 byte for lengths < ZIPMAP_BIGLEN and From bfded2aa511f70bb07941698f883b38de68c7773 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Thu, 1 Apr 2010 13:24:18 +0200 Subject: [PATCH 6/8] updated zipmap documentation to match the implementation --- zipmap.c | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/zipmap.c b/zipmap.c index 5215954ad..7f6268b48 100644 --- a/zipmap.c +++ b/zipmap.c @@ -42,10 +42,11 @@ /* Memory layout of a zipmap, for the map "foo" => "bar", "hello" => "world": * - * "foo""bar""hello""world" + * "foo""bar""hello""world" * - * is 1 byte status. Currently only 1 bit is used: if the least - * significant bit is set, it means the zipmap needs to be defragmented. + * is 1 byte length that holds the current size of the zipmap. + * When the zipmap length is greater than or equal to 254, this value + * is not used and the zipmap needs to be traversed to find out the length. * * is the length of the following string (key or value). * lengths are encoded in a single value or in a 5 bytes value. @@ -62,22 +63,15 @@ * or even in order to add a key/value pair if it fits. * * is always an unsigned 8 bit number, because if after an - * update operation there are more than a few free bytes, they'll be converted - * into empty space prefixed by the special value 254. + * update operation there are more than a few free bytes, the zipmap will be + * reallocated to make sure it is as small as possible. * * The most compact representation of the above two elements hash is actually: * - * "\x00\x03foo\x03\x00bar\x05hello\x05\x00world\xff" + * "\x02\x03foo\x03\x00bar\x05hello\x05\x00world\xff" * - * Empty space is marked using a 254 bytes + a (coded as already - * specified). The length includes the 254 bytes in the count and the - * space taken by the field. So for instance removing the "foo" key - * from the zipmap above will lead to the following representation: - * - * "\x00\xfd\x10........\x05hello\x05\x00world\xff" - * - * Note that because empty space, keys, values, are all prefixed length - * "objects", the lookup will take O(N) where N is the numeber of elements + * Note that because keys and values are prefixed length "objects", + * the lookup will take O(N) where N is the number of elements * in the zipmap and *not* the number of bytes needed to represent the zipmap. * This lowers the constant times considerably. */ From da2cfe8a3baeaad619c11290acf377f4ea7356c1 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Thu, 1 Apr 2010 14:02:22 +0200 Subject: [PATCH 7/8] update the zipmap entry in-place instead of appending it --- zipmap.c | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/zipmap.c b/zipmap.c index 7f6268b48..6c13f8062 100644 --- a/zipmap.c +++ b/zipmap.c @@ -208,7 +208,7 @@ static inline unsigned char *zipmapResize(unsigned char *zm, unsigned int len) { * If 'update' is not NULL, *update is set to 1 if the key was * already preset, otherwise to 0. */ unsigned char *zipmapSet(unsigned char *zm, unsigned char *key, unsigned int klen, unsigned char *val, unsigned int vlen, int *update) { - unsigned int zmlen; + unsigned int zmlen, offset; unsigned int freelen, reqlen = zipmapRequiredLength(klen,vlen); unsigned int empty, vempty; unsigned char *p; @@ -230,27 +230,34 @@ unsigned char *zipmapSet(unsigned char *zm, unsigned char *key, unsigned int kle if (update) *update = 1; freelen = zipmapRawEntryLength(p); if (freelen < reqlen) { - /* Move remaining entries to the current position, so this - * pair can be appended. Note: the +1 in memmove is caused - * by the end-of-zipmap byte. */ - memmove(p, p+freelen, zmlen-((p-zm)+freelen+1)); + /* Store the offset of this key within the current zipmap, so + * it can be resized. Then, move the tail backwards so this + * pair fits at the current position. */ + offset = p-zm; zm = zipmapResize(zm, zmlen-freelen+reqlen); - p = zm+zmlen-1-freelen; - zmlen = zmlen-1-freelen+reqlen; + p = zm+offset; + + /* The +1 in the number of bytes to be moved is caused by the + * end-of-zipmap byte. Note: the *original* zmlen is used. */ + memmove(p+reqlen, p+freelen, zmlen-(offset+freelen+1)); + zmlen = zmlen-freelen+reqlen; freelen = reqlen; } } - /* Ok we have a suitable block where to write the new key/value - * entry. */ + /* We now have a suitable block where the key/value entry can + * be written. If there is too much free space, move the tail + * of the zipmap a few bytes to the front and shrink the zipmap, + * as we want zipmaps to be very space efficient. */ empty = freelen-reqlen; - /* If there is too much free space mark it as a free block instead - * of adding it as trailing empty space for the value, as we want - * zipmaps to be very space efficient. */ if (empty >= ZIPMAP_VALUE_MAX_FREE) { - memmove(p+reqlen, p+freelen, zmlen-((p-zm)+freelen+1)); + /* First, move the tail bytes to the front, then resize + * the zipmap to be bytes smaller. */ + offset = p-zm; + memmove(p+reqlen, p+freelen, zmlen-(offset+freelen+1)); zmlen -= empty; zm = zipmapResize(zm, zmlen); + p = zm+offset; vempty = 0; } else { vempty = empty; From e9484a8502ce478585f884466efd061db782899f Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Thu, 1 Apr 2010 14:31:13 +0200 Subject: [PATCH 8/8] reduce code complexity because zipmapLen now is O(1) --- redis.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/redis.c b/redis.c index f2f54316c..e69d3a3ab 100644 --- a/redis.c +++ b/redis.c @@ -5937,10 +5937,8 @@ static void hsetCommand(redisClient *c) { decrRefCount(valobj); o->ptr = zm; - /* And here there is the second check for hash conversion... - * we want to do it only if the operation was not just an update as - * zipmapLen() is O(N). */ - if (!update && zipmapLen(zm) > server.hash_max_zipmap_entries) + /* And here there is the second check for hash conversion. */ + if (zipmapLen(zm) > server.hash_max_zipmap_entries) convertToRealHash(o); } else { tryObjectEncoding(c->argv[2]); @@ -5958,7 +5956,6 @@ static void hsetCommand(redisClient *c) { } static void hincrbyCommand(redisClient *c) { - int update = 0; long long value = 0, incr = 0; robj *o = lookupKeyWrite(c->db,c->argv[1]); @@ -5995,13 +5992,12 @@ static void hincrbyCommand(redisClient *c) { value += incr; sds svalue = sdscatprintf(sdsempty(),"%lld",value); zm = zipmapSet(zm,c->argv[2]->ptr,sdslen(c->argv[2]->ptr), - (unsigned char*)svalue,sdslen(svalue),&update); + (unsigned char*)svalue,sdslen(svalue),NULL); sdsfree(svalue); o->ptr = zm; - /* Check if the zipmap needs to be converted - * if this was not an update. */ - if (!update && zipmapLen(zm) > server.hash_max_zipmap_entries) + /* Check if the zipmap needs to be converted. */ + if (zipmapLen(zm) > server.hash_max_zipmap_entries) convertToRealHash(o); } else { robj *hval;