From f15df8ba5db09bdf4be58c53930799d82120cc34 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Thu, 9 Apr 2015 10:37:01 +0300 Subject: [PATCH 1/7] sds size classes - memory optimization --- src/Makefile | 2 +- src/debug.c | 5 +- src/networking.c | 33 +++--- src/object.c | 9 +- src/redis-cli.c | 4 +- src/scripting.c | 12 +- src/sds.c | 287 ++++++++++++++++++++++++++++++++--------------- src/sds.h | 149 ++++++++++++++++++++++-- 8 files changed, 363 insertions(+), 138 deletions(-) diff --git a/src/Makefile b/src/Makefile index 650d438f7..106fef340 100644 --- a/src/Makefile +++ b/src/Makefile @@ -120,7 +120,7 @@ REDIS_SENTINEL_NAME=redis-sentinel REDIS_SERVER_OBJ=adlist.o quicklist.o ae.o anet.o dict.o redis.o sds.o zmalloc.o lzf_c.o lzf_d.o pqsort.o zipmap.o sha1.o ziplist.o release.o networking.o util.o object.o db.o replication.o rdb.o t_string.o t_list.o t_set.o t_zset.o t_hash.o config.o aof.o pubsub.o multi.o debug.o sort.o intset.o syncio.o cluster.o crc16.o endianconv.o slowlog.o scripting.o bio.o rio.o rand.o memtest.o crc64.o bitops.o sentinel.o notify.o setproctitle.o blocked.o hyperloglog.o latency.o sparkline.o redis-check-rdb.o geo.o REDIS_GEOHASH_OBJ=../deps/geohash-int/geohash.o ../deps/geohash-int/geohash_helper.o REDIS_CLI_NAME=redis-cli -REDIS_CLI_OBJ=anet.o sds.o adlist.o redis-cli.o zmalloc.o release.o anet.o ae.o crc64.o +REDIS_CLI_OBJ=anet.o adlist.o redis-cli.o zmalloc.o release.o anet.o ae.o crc64.o REDIS_BENCHMARK_NAME=redis-benchmark REDIS_BENCHMARK_OBJ=ae.o anet.o redis-benchmark.o sds.o adlist.o zmalloc.o redis-benchmark.o REDIS_CHECK_RDB_NAME=redis-check-rdb diff --git a/src/debug.c b/src/debug.c index 2acba1495..e071fa0b6 100644 --- a/src/debug.c +++ b/src/debug.c @@ -423,7 +423,10 @@ void debugCommand(redisClient *c) { sizes = sdscatprintf(sizes,"bits:%d ", (sizeof(void*) == 8)?64:32); sizes = sdscatprintf(sizes,"robj:%d ", (int)sizeof(robj)); sizes = sdscatprintf(sizes,"dictentry:%d ", (int)sizeof(dictEntry)); - sizes = sdscatprintf(sizes,"sdshdr:%d", (int)sizeof(struct sdshdr)); + sizes = sdscatprintf(sizes,"sdshdr8:%d", (int)sizeof(struct sdshdr8)); + sizes = sdscatprintf(sizes,"sdshdr16:%d", (int)sizeof(struct sdshdr16)); + sizes = sdscatprintf(sizes,"sdshdr32:%d", (int)sizeof(struct sdshdr32)); + sizes = sdscatprintf(sizes,"sdshdr64:%d", (int)sizeof(struct sdshdr64)); addReplyBulkSds(c,sizes); } else if (!strcasecmp(c->argv[1]->ptr,"htstats") && c->argc == 3) { long dbid; diff --git a/src/networking.c b/src/networking.c index eb033ae61..12f8d2e9f 100644 --- a/src/networking.c +++ b/src/networking.c @@ -33,21 +33,14 @@ static void setProtocolError(redisClient *c, int pos); -/* To evaluate the output buffer size of a client we need to get size of - * allocated objects, however we can't used zmalloc_size() directly on sds - * strings because of the trick they use to work (the header is before the - * returned pointer), so we use this helper function. */ -size_t zmalloc_size_sds(sds s) { - return zmalloc_size(s-sizeof(struct sdshdr)); -} /* Return the amount of memory used by the sds string at object->ptr * for a string object. */ size_t getStringObjectSdsUsedMemory(robj *o) { redisAssertWithInfo(NULL,o,o->type == REDIS_STRING); switch(o->encoding) { - case REDIS_ENCODING_RAW: return zmalloc_size_sds(o->ptr); - case REDIS_ENCODING_EMBSTR: return sdslen(o->ptr); + case REDIS_ENCODING_RAW: return sdsZmallocSize(o->ptr); + case REDIS_ENCODING_EMBSTR: return zmalloc_size(o)-sizeof(robj); default: return 0; /* Just integer encoding for now. */ } } @@ -235,10 +228,10 @@ void _addReplyObjectToList(redisClient *c, robj *o) { tail->encoding == REDIS_ENCODING_RAW && sdslen(tail->ptr)+sdslen(o->ptr) <= REDIS_REPLY_CHUNK_BYTES) { - c->reply_bytes -= zmalloc_size_sds(tail->ptr); + c->reply_bytes -= sdsZmallocSize(tail->ptr); tail = dupLastObjectIfNeeded(c->reply); tail->ptr = sdscatlen(tail->ptr,o->ptr,sdslen(o->ptr)); - c->reply_bytes += zmalloc_size_sds(tail->ptr); + c->reply_bytes += sdsZmallocSize(tail->ptr); } else { incrRefCount(o); listAddNodeTail(c->reply,o); @@ -260,7 +253,7 @@ void _addReplySdsToList(redisClient *c, sds s) { if (listLength(c->reply) == 0) { listAddNodeTail(c->reply,createObject(REDIS_STRING,s)); - c->reply_bytes += zmalloc_size_sds(s); + c->reply_bytes += sdsZmallocSize(s); } else { tail = listNodeValue(listLast(c->reply)); @@ -268,14 +261,14 @@ void _addReplySdsToList(redisClient *c, sds s) { if (tail->ptr != NULL && tail->encoding == REDIS_ENCODING_RAW && sdslen(tail->ptr)+sdslen(s) <= REDIS_REPLY_CHUNK_BYTES) { - c->reply_bytes -= zmalloc_size_sds(tail->ptr); + c->reply_bytes -= sdsZmallocSize(tail->ptr); tail = dupLastObjectIfNeeded(c->reply); tail->ptr = sdscatlen(tail->ptr,s,sdslen(s)); - c->reply_bytes += zmalloc_size_sds(tail->ptr); + c->reply_bytes += sdsZmallocSize(tail->ptr); sdsfree(s); } else { listAddNodeTail(c->reply,createObject(REDIS_STRING,s)); - c->reply_bytes += zmalloc_size_sds(s); + c->reply_bytes += sdsZmallocSize(s); } } asyncCloseClientOnOutputBufferLimitReached(c); @@ -298,10 +291,10 @@ void _addReplyStringToList(redisClient *c, const char *s, size_t len) { if (tail->ptr != NULL && tail->encoding == REDIS_ENCODING_RAW && sdslen(tail->ptr)+len <= REDIS_REPLY_CHUNK_BYTES) { - c->reply_bytes -= zmalloc_size_sds(tail->ptr); + c->reply_bytes -= sdsZmallocSize(tail->ptr); tail = dupLastObjectIfNeeded(c->reply); tail->ptr = sdscatlen(tail->ptr,s,len); - c->reply_bytes += zmalloc_size_sds(tail->ptr); + c->reply_bytes += sdsZmallocSize(tail->ptr); } else { robj *o = createStringObject(s,len); @@ -440,16 +433,16 @@ void setDeferredMultiBulkLength(redisClient *c, void *node, long length) { len = listNodeValue(ln); len->ptr = sdscatprintf(sdsempty(),"*%ld\r\n",length); len->encoding = REDIS_ENCODING_RAW; /* in case it was an EMBSTR. */ - c->reply_bytes += zmalloc_size_sds(len->ptr); + c->reply_bytes += sdsZmallocSize(len->ptr); if (ln->next != NULL) { next = listNodeValue(ln->next); /* Only glue when the next node is non-NULL (an sds in this case) */ if (next->ptr != NULL) { - c->reply_bytes -= zmalloc_size_sds(len->ptr); + c->reply_bytes -= sdsZmallocSize(len->ptr); c->reply_bytes -= getStringObjectSdsUsedMemory(next); len->ptr = sdscatlen(len->ptr,next->ptr,sdslen(next->ptr)); - c->reply_bytes += zmalloc_size_sds(len->ptr); + c->reply_bytes += sdsZmallocSize(len->ptr); listDelNode(c->reply,ln->next); } } diff --git a/src/object.c b/src/object.c index dcd896917..881b1ac4b 100644 --- a/src/object.c +++ b/src/object.c @@ -58,8 +58,8 @@ robj *createRawStringObject(const char *ptr, size_t len) { * an object where the sds string is actually an unmodifiable string * allocated in the same chunk as the object itself. */ robj *createEmbeddedStringObject(const char *ptr, size_t len) { - robj *o = zmalloc(sizeof(robj)+sizeof(struct sdshdr)+len+1); - struct sdshdr *sh = (void*)(o+1); + robj *o = zmalloc(sizeof(robj)+sizeof(struct sdshdr8)+len+1); + struct sdshdr8 *sh = (void*)(o+1); o->type = REDIS_STRING; o->encoding = REDIS_ENCODING_EMBSTR; @@ -68,7 +68,8 @@ robj *createEmbeddedStringObject(const char *ptr, size_t len) { o->lru = LRU_CLOCK(); sh->len = len; - sh->free = 0; + sh->alloc = len; + sh->flags = SDS_TYPE_8; if (ptr) { memcpy(sh->buf,ptr,len); sh->buf[len] = '\0'; @@ -84,7 +85,7 @@ robj *createEmbeddedStringObject(const char *ptr, size_t len) { * * The current limit of 39 is chosen so that the biggest string object * we allocate as EMBSTR will still fit into the 64 byte arena of jemalloc. */ -#define REDIS_ENCODING_EMBSTR_SIZE_LIMIT 39 +#define REDIS_ENCODING_EMBSTR_SIZE_LIMIT 44 robj *createStringObject(const char *ptr, size_t len) { if (len <= REDIS_ENCODING_EMBSTR_SIZE_LIMIT) return createEmbeddedStringObject(ptr,len); diff --git a/src/redis-cli.c b/src/redis-cli.c index acf7c98b9..64bf210bf 100644 --- a/src/redis-cli.c +++ b/src/redis-cli.c @@ -46,8 +46,8 @@ #include #include -#include "hiredis.h" -#include "sds.h" +#include +#include /* use sds.h from hiredis, so that only one set of sds functions will be present in the binary */ #include "zmalloc.h" #include "linenoise.h" #include "help.h" diff --git a/src/scripting.c b/src/scripting.c index 53c0c9ed2..e8da69f74 100644 --- a/src/scripting.c +++ b/src/scripting.c @@ -265,14 +265,11 @@ int luaRedisGenericCommand(lua_State *lua, int raise_error) { if (j < LUA_CMD_OBJCACHE_SIZE && cached_objects[j] && cached_objects_len[j] >= obj_len) { - char *s = cached_objects[j]->ptr; - struct sdshdr *sh = (void*)(s-(sizeof(struct sdshdr))); - + sds s = cached_objects[j]->ptr; argv[j] = cached_objects[j]; cached_objects[j] = NULL; memcpy(s,obj_s,obj_len+1); - sh->free += sh->len - obj_len; - sh->len = obj_len; + sdssetlen(s, obj_len); } else { argv[j] = createStringObject(obj_s, obj_len); } @@ -422,11 +419,10 @@ cleanup: o->encoding == REDIS_ENCODING_EMBSTR) && sdslen(o->ptr) <= LUA_CMD_OBJCACHE_MAX_LEN) { - struct sdshdr *sh = (void*)(((char*)(o->ptr))-(sizeof(struct sdshdr))); - + sds s = o->ptr; if (cached_objects[j]) decrRefCount(cached_objects[j]); cached_objects[j] = o; - cached_objects_len[j] = sh->free + sh->len; + cached_objects_len[j] = sdsalloc(s); } else { decrRefCount(o); } diff --git a/src/sds.c b/src/sds.c index 2ebe286d1..dbf6c64ad 100644 --- a/src/sds.c +++ b/src/sds.c @@ -36,6 +36,30 @@ #include "sds.h" #include "zmalloc.h" +static inline int sdsHdrSize(char type) { + switch(type&SDS_TYPE_MASK) { + case SDS_TYPE_8: + return sizeof(struct sdshdr8); + case SDS_TYPE_16: + return sizeof(struct sdshdr16); + case SDS_TYPE_32: + return sizeof(struct sdshdr32); + case SDS_TYPE_64: + return sizeof(struct sdshdr64); + } + return 0; +} + +static inline char sdsReqType(size_t string_size) { + if (string_size<0xff) + return SDS_TYPE_8; + if (string_size<0xffff) + return SDS_TYPE_16; + if (string_size<0xffffffff) + return SDS_TYPE_32; + return SDS_TYPE_64; +} + /* Create a new sds string with the content specified by the 'init' pointer * and 'initlen'. * If NULL is used for 'init' the string is initialized with zero bytes. @@ -49,20 +73,65 @@ * end of the string. However the string is binary safe and can contain * \0 characters in the middle, as the length is stored in the sds header. */ sds sdsnewlen(const void *init, size_t initlen) { - struct sdshdr *sh; - - if (init) { - sh = zmalloc(sizeof(struct sdshdr)+initlen+1); - } else { - sh = zcalloc(sizeof(struct sdshdr)+initlen+1); - } + void *sh; + sds s; + char type = sdsReqType(initlen); + int hdrlen = sdsHdrSize(type); + + sh = zmalloc(hdrlen+initlen+1); + if (!init) + memset(sh, 0, hdrlen+initlen+1); if (sh == NULL) return NULL; - sh->len = initlen; - sh->free = 0; + s = (char*)sh+hdrlen; + switch(type) { + case SDS_TYPE_8: { + SDS_HDR_VAR(8,s); + sh->len = initlen; + sh->alloc = initlen; + break; + } + case SDS_TYPE_16: { + SDS_HDR_VAR(16,s); + sh->len = initlen; + sh->alloc = initlen; + break; + } + case SDS_TYPE_32: { + SDS_HDR_VAR(32,s); + sh->len = initlen; + sh->alloc = initlen; + break; + } + case SDS_TYPE_64: { + SDS_HDR_VAR(64,s); + sh->len = initlen; + sh->alloc = initlen; + break; + } + } + s[-1] = type; if (initlen && init) - memcpy(sh->buf, init, initlen); - sh->buf[initlen] = '\0'; - return (char*)sh->buf; + memcpy(s, init, initlen); + s[initlen] = '\0'; + return s; +} + +void sdsIncRefcount(sds s) { + unsigned char flags = s[-1]; + unsigned refs = flags>>SDS_TYPE_BITS; + assert(++refs); + s[-1] = (refs<>SDS_TYPE_BITS; + assert(refs); + if (!(--refs)) + zfree(sh); + else + s[-1] = (refs<free += (sh->len-reallen); - sh->len = reallen; + sdssetlen(s, reallen); } /* Modify an sds string in-place to make it empty (zero length). @@ -114,10 +181,8 @@ void sdsupdatelen(sds s) { * so that next append operations will not require allocations up to the * number of bytes previously available. */ void sdsclear(sds s) { - struct sdshdr *sh = (void*) (s-(sizeof(struct sdshdr))); - sh->free += sh->len; - sh->len = 0; - sh->buf[0] = '\0'; + sdssetlen(s, 0); + s[0] = '\0'; } /* Enlarge the free space at the end of the sds string so that the caller @@ -127,23 +192,41 @@ void sdsclear(sds s) { * Note: this does not change the *length* of the sds string as returned * by sdslen(), but only the free buffer space we have. */ sds sdsMakeRoomFor(sds s, size_t addlen) { - struct sdshdr *sh, *newsh; - size_t free = sdsavail(s); + void *sh, *newsh; + size_t avail = sdsavail(s); size_t len, newlen; + char type, oldtype = s[-1]; + int hdrlen; - if (free >= addlen) return s; + if (avail >= addlen) return s; len = sdslen(s); - sh = (void*) (s-(sizeof(struct sdshdr))); + sh = (char*)s-sdsHdrSize(oldtype); newlen = (len+addlen); if (newlen < SDS_MAX_PREALLOC) newlen *= 2; else newlen += SDS_MAX_PREALLOC; - newsh = zrealloc(sh, sizeof(struct sdshdr)+newlen+1); - if (newsh == NULL) return NULL; - newsh->free = newlen - len; - return newsh->buf; + assert(!(s[-1]>>SDS_TYPE_BITS));/* verify that the ref count is 0 (non ref count managed string) */ + type = sdsReqType(newlen); + hdrlen = sdsHdrSize(type); + if (oldtype==type) { + newsh = zrealloc(sh, hdrlen+newlen+1); + if (newsh == NULL) return NULL; + s = (char*)newsh+hdrlen; + } else { + /* since the header size changes, need to move the string forward, and can't use realloc */ + newsh = zmalloc(hdrlen+newlen+1); + if (newsh == NULL) return NULL; + memcpy((char*)newsh+hdrlen, s, len+1); + zfree(sh); + s = (char*)newsh+hdrlen; + s[-1] = type; + sdssetlen(s, len); + } + sdssetalloc(s, newlen); + s[-1] = type; + return s; } /* Reallocate the sds string so that it has no free space at the end. The @@ -153,12 +236,31 @@ sds sdsMakeRoomFor(sds s, size_t addlen) { * After the call, the passed sds string is no longer valid and all the * references must be substituted with the new pointer returned by the call. */ sds sdsRemoveFreeSpace(sds s) { - struct sdshdr *sh; + void *sh, *newsh; + char type, oldtype = s[-1]; + int hdrlen; + size_t len = sdslen(s); + sh = (char*)s-sdsHdrSize(oldtype); - sh = (void*) (s-(sizeof(struct sdshdr))); - sh = zrealloc(sh, sizeof(struct sdshdr)+sh->len+1); - sh->free = 0; - return sh->buf; + type = sdsReqType(len); + hdrlen = sdsHdrSize(type); + if (oldtype==type) { + newsh = zrealloc(sh, hdrlen+len+1); + if (newsh == NULL) return NULL; + s = (char*)newsh+hdrlen; + } else { + newsh = zmalloc(hdrlen+len+1); + if (newsh == NULL) return NULL; + memcpy((char*)newsh+hdrlen, s, len+1); + zfree(sh); + s = (char*)newsh+hdrlen; + s[-1] = type; + sdssetlen(s, len); + } + sdssetalloc(s, len); + assert(!(s[-1]>>SDS_TYPE_BITS));/* verify that the ref count is 0 (non ref count managed string) */ + s[-1] = type; + return s; } /* Return the total size of the allocation of the specifed sds string, @@ -169,9 +271,15 @@ sds sdsRemoveFreeSpace(sds s) { * 4) The implicit null term. */ size_t sdsAllocSize(sds s) { - struct sdshdr *sh = (void*) (s-(sizeof(struct sdshdr))); + size_t alloc = sdsalloc(s); + return sdsHdrSize(s[-1])+alloc+1; +} - return sizeof(*sh)+sh->len+sh->free+1; +/* Return the size consumed from the allocator, + * including internal fragmentation */ +size_t sdsZmallocSize(sds s) { + struct sdshdr *sh = (void*) (s-sdsHdrSize(s[-1])); + return zmalloc_size(sh); } /* Increment the sds length and decrements the left free space at the @@ -198,15 +306,35 @@ size_t sdsAllocSize(sds s) { * sdsIncrLen(s, nread); */ void sdsIncrLen(sds s, int incr) { - struct sdshdr *sh = (void*) (s-(sizeof(struct sdshdr))); - - if (incr >= 0) - assert(sh->free >= (unsigned int)incr); - else - assert(sh->len >= (unsigned int)(-incr)); - sh->len += incr; - sh->free -= incr; - s[sh->len] = '\0'; + char flags = s[-1]; + size_t len; + switch(flags&SDS_TYPE_MASK) { + case SDS_TYPE_8: { + SDS_HDR_VAR(8,s); + assert((incr >= 0 && sh->alloc-sh->len >= incr) || (incr < 0 && sh->len >= (unsigned int)(-incr))); + len = (sh->len += incr); + break; + } + case SDS_TYPE_16: { + SDS_HDR_VAR(16,s); + assert((incr >= 0 && sh->alloc-sh->len >= incr) || (incr < 0 && sh->len >= (unsigned int)(-incr))); + len = (sh->len += incr); + break; + } + case SDS_TYPE_32: { + SDS_HDR_VAR(32,s); + assert((incr >= 0 && sh->alloc-sh->len >= (unsigned int)incr) || (incr < 0 && sh->len >= (unsigned int)(-incr))); + len = (sh->len += incr); + break; + } + case SDS_TYPE_64: { + SDS_HDR_VAR(64,s); + assert((incr >= 0 && sh->alloc-sh->len >= (uint64_t)incr) || (incr < 0 && sh->len >= (uint64_t)(-incr))); + len = (sh->len += incr); + break; + } + } + s[len] = '\0'; } /* Grow the sds to have the specified length. Bytes that were not part of @@ -215,19 +343,15 @@ void sdsIncrLen(sds s, int incr) { * if the specified length is smaller than the current length, no operation * is performed. */ sds sdsgrowzero(sds s, size_t len) { - struct sdshdr *sh = (void*)(s-(sizeof(struct sdshdr))); - size_t totlen, curlen = sh->len; + size_t curlen = sdslen(s); if (len <= curlen) return s; s = sdsMakeRoomFor(s,len-curlen); if (s == NULL) return NULL; /* Make sure added region doesn't contain garbage */ - sh = (void*)(s-(sizeof(struct sdshdr))); memset(s+curlen,0,(len-curlen+1)); /* also set trailing \0 byte */ - totlen = sh->len+sh->free; - sh->len = len; - sh->free = totlen-sh->len; + sdssetlen(s, len); return s; } @@ -237,15 +361,12 @@ sds sdsgrowzero(sds s, size_t len) { * After the call, the passed sds string is no longer valid and all the * references must be substituted with the new pointer returned by the call. */ sds sdscatlen(sds s, const void *t, size_t len) { - struct sdshdr *sh; size_t curlen = sdslen(s); s = sdsMakeRoomFor(s,len); if (s == NULL) return NULL; - sh = (void*) (s-(sizeof(struct sdshdr))); memcpy(s+curlen, t, len); - sh->len = curlen+len; - sh->free = sh->free-len; + sdssetlen(s, curlen+len); s[curlen+len] = '\0'; return s; } @@ -269,19 +390,13 @@ sds sdscatsds(sds s, const sds t) { /* Destructively modify the sds string 's' to hold the specified binary * safe string pointed by 't' of length 'len' bytes. */ sds sdscpylen(sds s, const char *t, size_t len) { - struct sdshdr *sh = (void*) (s-(sizeof(struct sdshdr))); - size_t totlen = sh->free+sh->len; - - if (totlen < len) { - s = sdsMakeRoomFor(s,len-sh->len); + if (sdsalloc(s) < len) { + s = sdsMakeRoomFor(s,len-sdslen(s)); if (s == NULL) return NULL; - sh = (void*) (s-(sizeof(struct sdshdr))); - totlen = sh->free+sh->len; } memcpy(s, t, len); s[len] = '\0'; - sh->len = len; - sh->free = totlen-len; + sdssetlen(s, len); return s; } @@ -449,7 +564,6 @@ sds sdscatprintf(sds s, const char *fmt, ...) { * %% - Verbatim "%" character. */ sds sdscatfmt(sds s, char const *fmt, ...) { - struct sdshdr *sh = (void*) (s-(sizeof(struct sdshdr))); size_t initlen = sdslen(s); const char *f = fmt; int i; @@ -460,14 +574,13 @@ sds sdscatfmt(sds s, char const *fmt, ...) { i = initlen; /* Position of the next byte to write to dest str. */ while(*f) { char next, *str; - unsigned int l; + size_t l; long long num; unsigned long long unum; /* Make sure there is always space for at least 1 char. */ - if (sh->free == 0) { + if (sdsavail(s)==0) { s = sdsMakeRoomFor(s,1); - sh = (void*) (s-(sizeof(struct sdshdr))); } switch(*f) { @@ -479,13 +592,11 @@ sds sdscatfmt(sds s, char const *fmt, ...) { case 'S': str = va_arg(ap,char*); l = (next == 's') ? strlen(str) : sdslen(str); - if (sh->free < l) { + if (sdsavail(s) < l) { s = sdsMakeRoomFor(s,l); - sh = (void*) (s-(sizeof(struct sdshdr))); } memcpy(s+i,str,l); - sh->len += l; - sh->free -= l; + sdsinclen(s,l); i += l; break; case 'i': @@ -497,13 +608,11 @@ sds sdscatfmt(sds s, char const *fmt, ...) { { char buf[SDS_LLSTR_SIZE]; l = sdsll2str(buf,num); - if (sh->free < l) { + if (sdsavail(s) < l) { s = sdsMakeRoomFor(s,l); - sh = (void*) (s-(sizeof(struct sdshdr))); } memcpy(s+i,buf,l); - sh->len += l; - sh->free -= l; + sdsinclen(s,l); i += l; } break; @@ -516,27 +625,23 @@ sds sdscatfmt(sds s, char const *fmt, ...) { { char buf[SDS_LLSTR_SIZE]; l = sdsull2str(buf,unum); - if (sh->free < l) { + if (sdsavail(s) < l) { s = sdsMakeRoomFor(s,l); - sh = (void*) (s-(sizeof(struct sdshdr))); } memcpy(s+i,buf,l); - sh->len += l; - sh->free -= l; + sdsinclen(s,l); i += l; } break; default: /* Handle %% and generally %. */ s[i++] = next; - sh->len += 1; - sh->free -= 1; + sdsinclen(s,1); break; } break; default: s[i++] = *f; - sh->len += 1; - sh->free -= 1; + sdsinclen(s,1); break; } f++; @@ -563,7 +668,6 @@ sds sdscatfmt(sds s, char const *fmt, ...) { * Output will be just "Hello World". */ sds sdstrim(sds s, const char *cset) { - struct sdshdr *sh = (void*) (s-(sizeof(struct sdshdr))); char *start, *end, *sp, *ep; size_t len; @@ -572,10 +676,9 @@ sds sdstrim(sds s, const char *cset) { while(sp <= end && strchr(cset, *sp)) sp++; while(ep > sp && strchr(cset, *ep)) ep--; len = (sp > ep) ? 0 : ((ep-sp)+1); - if (sh->buf != sp) memmove(sh->buf, sp, len); - sh->buf[len] = '\0'; - sh->free = sh->free+(sh->len-len); - sh->len = len; + if (s != sp) memmove(s, sp, len); + s[len] = '\0'; + sdssetlen(s,len); return s; } @@ -596,7 +699,6 @@ sds sdstrim(sds s, const char *cset) { * sdsrange(s,1,-1); => "ello World" */ void sdsrange(sds s, int start, int end) { - struct sdshdr *sh = (void*) (s-(sizeof(struct sdshdr))); size_t newlen, len = sdslen(s); if (len == 0) return; @@ -619,10 +721,9 @@ void sdsrange(sds s, int start, int end) { } else { start = 0; } - if (start && newlen) memmove(sh->buf, sh->buf+start, newlen); - sh->buf[newlen] = 0; - sh->free = sh->free+(sh->len-newlen); - sh->len = newlen; + if (start && newlen) memmove(s, s+start, newlen); + s[newlen] = 0; + sdssetlen(s,newlen); } /* Apply tolower() to every character of the sds string 's'. */ diff --git a/src/sds.h b/src/sds.h index 93dd4f28e..9201a751c 100644 --- a/src/sds.h +++ b/src/sds.h @@ -35,32 +35,157 @@ #include #include +#include typedef char *sds; -struct sdshdr { - unsigned int len; - unsigned int free; +struct __attribute__ ((__packed__)) sdshdr8 { + uint8_t len; /* used */ + uint8_t alloc; /* excluding the header and null terminator */ + char flags; /* 2 lsb of type, and 6 msb of refcount */ + char buf[]; +}; +struct __attribute__ ((__packed__)) sdshdr16 { + uint16_t len; /* used */ + uint16_t alloc; /* excluding the header and null terminator */ + char flags; /* 2 lsb of type, and 6 msb of refcount */ + char buf[]; +}; +struct __attribute__ ((__packed__)) sdshdr32 { + uint32_t len; /* used */ + uint32_t alloc; /* excluding the header and null terminator */ + char flags; /* 2 lsb of type, and 6 msb of refcount */ + char buf[]; +}; +struct __attribute__ ((__packed__)) sdshdr64 { + uint64_t len; /* used */ + uint64_t alloc; /* excluding the header and null terminator */ + char flags; /* 2 lsb of type, and 6 msb of refcount */ char buf[]; }; +#define SDS_TYPE_8 0 +#define SDS_TYPE_16 1 +#define SDS_TYPE_32 2 +#define SDS_TYPE_64 3 +#define SDS_TYPE_MASK 3 +#define SDS_TYPE_BITS 2 +#define SDS_HDR_VAR(T,s) struct sdshdr##T *sh = (void*)((s)-(sizeof(struct sdshdr##T))); +#define SDS_HDR(T,s) ((struct sdshdr##T *)((s)-(sizeof(struct sdshdr##T)))) + static inline size_t sdslen(const sds s) { - struct sdshdr *sh = (void*)(s-(sizeof(struct sdshdr))); - return sh->len; + char flags = s[-1]; + switch(flags&SDS_TYPE_MASK) { + case SDS_TYPE_8: + return SDS_HDR(8,s)->len; + case SDS_TYPE_16: + return SDS_HDR(16,s)->len; + case SDS_TYPE_32: + return SDS_HDR(32,s)->len; + case SDS_TYPE_64: + return SDS_HDR(64,s)->len; + } + return 0; } static inline size_t sdsavail(const sds s) { - struct sdshdr *sh = (void*)(s-(sizeof(struct sdshdr))); - return sh->free; + char flags = s[-1]; + switch(flags&SDS_TYPE_MASK) { + case SDS_TYPE_8: { + SDS_HDR_VAR(8,s); + return sh->alloc - sh->len; + } + case SDS_TYPE_16: { + SDS_HDR_VAR(16,s); + return sh->alloc - sh->len; + } + case SDS_TYPE_32: { + SDS_HDR_VAR(32,s); + return sh->alloc - sh->len; + } + case SDS_TYPE_64: { + SDS_HDR_VAR(64,s); + return sh->alloc - sh->len; + } + } + return 0; +} + +static inline void sdssetlen(sds s, size_t newlen) { + char flags = s[-1]; + switch(flags&SDS_TYPE_MASK) { + case SDS_TYPE_8: + SDS_HDR(8,s)->len = newlen; + break; + case SDS_TYPE_16: + SDS_HDR(16,s)->len = newlen; + break; + case SDS_TYPE_32: + SDS_HDR(32,s)->len = newlen; + break; + case SDS_TYPE_64: + SDS_HDR(64,s)->len = newlen; + break; + } +} + +static inline void sdsinclen(sds s, size_t inc) { + char flags = s[-1]; + switch(flags&SDS_TYPE_MASK) { + case SDS_TYPE_8: + SDS_HDR(8,s)->len += inc; + break; + case SDS_TYPE_16: + SDS_HDR(16,s)->len += inc; + break; + case SDS_TYPE_32: + SDS_HDR(32,s)->len += inc; + break; + case SDS_TYPE_64: + SDS_HDR(64,s)->len += inc; + break; + } +} + +/* sdsalloc() = sdsavail() + sdslen() */ +static inline size_t sdsalloc(const sds s) { + char flags = s[-1]; + switch(flags&SDS_TYPE_MASK) { + case SDS_TYPE_8: + return SDS_HDR(8,s)->alloc; + case SDS_TYPE_16: + return SDS_HDR(16,s)->alloc; + case SDS_TYPE_32: + return SDS_HDR(32,s)->alloc; + case SDS_TYPE_64: + return SDS_HDR(64,s)->alloc; + } + return 0; +} + +static inline void sdssetalloc(sds s, size_t newlen) { + char flags = s[-1]; + switch(flags&SDS_TYPE_MASK) { + case SDS_TYPE_8: + SDS_HDR(8,s)->alloc = newlen; + break; + case SDS_TYPE_16: + SDS_HDR(16,s)->alloc = newlen; + break; + case SDS_TYPE_32: + SDS_HDR(32,s)->alloc = newlen; + break; + case SDS_TYPE_64: + SDS_HDR(64,s)->alloc = newlen; + break; + } } sds sdsnewlen(const void *init, size_t initlen); sds sdsnew(const char *init); sds sdsempty(void); -size_t sdslen(const sds s); sds sdsdup(const sds s); void sdsfree(sds s); -size_t sdsavail(const sds s); sds sdsgrowzero(sds s, size_t len); sds sdscatlen(sds s, const void *t, size_t len); sds sdscat(sds s, const char *t); @@ -68,6 +193,11 @@ sds sdscatsds(sds s, const sds t); sds sdscpylen(sds s, const char *t, size_t len); sds sdscpy(sds s, const char *t); +/* we can add a reference count on top of any + * existing sds. (max up to 63 references) */ +void sdsIncRefcount(sds s); +void sdsDecRefcount(sds s); + sds sdscatvprintf(sds s, const char *fmt, va_list ap); #ifdef __GNUC__ sds sdscatprintf(sds s, const char *fmt, ...) @@ -97,6 +227,7 @@ sds sdsMakeRoomFor(sds s, size_t addlen); void sdsIncrLen(sds s, int incr); sds sdsRemoveFreeSpace(sds s); size_t sdsAllocSize(sds s); +size_t sdsZmallocSize(sds s); #ifdef REDIS_TEST int sdsTest(int argc, char *argv[]); From a76b380e06d6758dcd84277003ca7af520c2b422 Mon Sep 17 00:00:00 2001 From: antirez Date: Tue, 14 Jul 2015 16:04:00 +0200 Subject: [PATCH 2/7] Fix DEBUG structsize output. --- src/debug.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/debug.c b/src/debug.c index e071fa0b6..3f7a85357 100644 --- a/src/debug.c +++ b/src/debug.c @@ -420,13 +420,13 @@ void debugCommand(redisClient *c) { addReplySds(c,errstr); } else if (!strcasecmp(c->argv[1]->ptr,"structsize") && c->argc == 2) { sds sizes = sdsempty(); - sizes = sdscatprintf(sizes,"bits:%d ", (sizeof(void*) == 8)?64:32); - sizes = sdscatprintf(sizes,"robj:%d ", (int)sizeof(robj)); - sizes = sdscatprintf(sizes,"dictentry:%d ", (int)sizeof(dictEntry)); - sizes = sdscatprintf(sizes,"sdshdr8:%d", (int)sizeof(struct sdshdr8)); - sizes = sdscatprintf(sizes,"sdshdr16:%d", (int)sizeof(struct sdshdr16)); - sizes = sdscatprintf(sizes,"sdshdr32:%d", (int)sizeof(struct sdshdr32)); - sizes = sdscatprintf(sizes,"sdshdr64:%d", (int)sizeof(struct sdshdr64)); + sizes = sdscatprintf(sizes,"bits:%d ",(sizeof(void*) == 8)?64:32); + sizes = sdscatprintf(sizes,"robj:%d ",(int)sizeof(robj)); + sizes = sdscatprintf(sizes,"dictentry:%d ",(int)sizeof(dictEntry)); + sizes = sdscatprintf(sizes,"sdshdr8:%d ",(int)sizeof(struct sdshdr8)); + sizes = sdscatprintf(sizes,"sdshdr16:%d ",(int)sizeof(struct sdshdr16)); + sizes = sdscatprintf(sizes,"sdshdr32:%d ",(int)sizeof(struct sdshdr32)); + sizes = sdscatprintf(sizes,"sdshdr64:%d ",(int)sizeof(struct sdshdr64)); addReplyBulkSds(c,sizes); } else if (!strcasecmp(c->argv[1]->ptr,"htstats") && c->argc == 3) { long dbid; From 056a0ca199edbc9f4644684468b8833884e74cd7 Mon Sep 17 00:00:00 2001 From: antirez Date: Tue, 14 Jul 2015 17:33:30 +0200 Subject: [PATCH 3/7] Fix redis-benchmark sds binding. Same as redis-cli, now redis-benchmark requires to use hiredis sds copy since it is different compared to the memory optimized fork of Redis sds. --- src/Makefile | 2 +- src/redis-benchmark.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Makefile b/src/Makefile index 106fef340..449b4ec26 100644 --- a/src/Makefile +++ b/src/Makefile @@ -122,7 +122,7 @@ REDIS_GEOHASH_OBJ=../deps/geohash-int/geohash.o ../deps/geohash-int/geohash_help REDIS_CLI_NAME=redis-cli REDIS_CLI_OBJ=anet.o adlist.o redis-cli.o zmalloc.o release.o anet.o ae.o crc64.o REDIS_BENCHMARK_NAME=redis-benchmark -REDIS_BENCHMARK_OBJ=ae.o anet.o redis-benchmark.o sds.o adlist.o zmalloc.o redis-benchmark.o +REDIS_BENCHMARK_OBJ=ae.o anet.o redis-benchmark.o adlist.o zmalloc.o redis-benchmark.o REDIS_CHECK_RDB_NAME=redis-check-rdb REDIS_CHECK_AOF_NAME=redis-check-aof REDIS_CHECK_AOF_OBJ=redis-check-aof.o diff --git a/src/redis-benchmark.c b/src/redis-benchmark.c index f735aeb63..e19fdce14 100644 --- a/src/redis-benchmark.c +++ b/src/redis-benchmark.c @@ -40,9 +40,9 @@ #include #include +#include /* Use hiredis sds. */ #include "ae.h" #include "hiredis.h" -#include "sds.h" #include "adlist.h" #include "zmalloc.h" From 0ab27a4594aa73ffdabf2afb935d85ab6c03f0ee Mon Sep 17 00:00:00 2001 From: antirez Date: Wed, 15 Jul 2015 12:24:49 +0200 Subject: [PATCH 4/7] SDS: New sds type 5 implemented. This is an attempt to use the refcount feature of the sds.c fork provided in the Pull Request #2509. A new type, SDS_TYPE_5 is introduced having a one byte header with just the string length, without information about the available additional length at the end of the string (this means that sdsMakeRoomFor() will be required each time we want to append something, since the string will always report to have 0 bytes available). More work needed in order to avoid common SDS functions will pay the cost of this type. For example both sdscatprintf() and sdscatfmt() should try to upgrade to SDS_TYPE_8 ASAP when appending chars. --- src/sds.c | 72 +++++++++++++++++++++++++++---------------------------- src/sds.h | 68 ++++++++++++++++++++++++++++++++++++---------------- 2 files changed, 83 insertions(+), 57 deletions(-) diff --git a/src/sds.c b/src/sds.c index dbf6c64ad..4771b6b3e 100644 --- a/src/sds.c +++ b/src/sds.c @@ -38,6 +38,8 @@ static inline int sdsHdrSize(char type) { switch(type&SDS_TYPE_MASK) { + case SDS_TYPE_5: + return sizeof(struct sdshdr5); case SDS_TYPE_8: return sizeof(struct sdshdr8); case SDS_TYPE_16: @@ -51,11 +53,13 @@ static inline int sdsHdrSize(char type) { } static inline char sdsReqType(size_t string_size) { - if (string_size<0xff) + if (string_size < 32) + return SDS_TYPE_5; + if (string_size < 0xff) return SDS_TYPE_8; - if (string_size<0xffff) + if (string_size < 0xffff) return SDS_TYPE_16; - if (string_size<0xffffffff) + if (string_size < 0xffffffff) return SDS_TYPE_32; return SDS_TYPE_64; } @@ -77,63 +81,54 @@ sds sdsnewlen(const void *init, size_t initlen) { sds s; char type = sdsReqType(initlen); int hdrlen = sdsHdrSize(type); - + unsigned char *fp; /* flags pointer. */ + sh = zmalloc(hdrlen+initlen+1); if (!init) memset(sh, 0, hdrlen+initlen+1); if (sh == NULL) return NULL; s = (char*)sh+hdrlen; + fp = ((unsigned char*)s)-1; switch(type) { + case SDS_TYPE_5: { + *fp = type | (initlen << SDS_TYPE_BITS); + break; + } case SDS_TYPE_8: { SDS_HDR_VAR(8,s); sh->len = initlen; sh->alloc = initlen; + *fp = type; break; } case SDS_TYPE_16: { SDS_HDR_VAR(16,s); sh->len = initlen; sh->alloc = initlen; + *fp = type; break; } case SDS_TYPE_32: { SDS_HDR_VAR(32,s); sh->len = initlen; sh->alloc = initlen; + *fp = type; break; } case SDS_TYPE_64: { SDS_HDR_VAR(64,s); sh->len = initlen; sh->alloc = initlen; + *fp = type; break; } } - s[-1] = type; if (initlen && init) memcpy(s, init, initlen); s[initlen] = '\0'; return s; } -void sdsIncRefcount(sds s) { - unsigned char flags = s[-1]; - unsigned refs = flags>>SDS_TYPE_BITS; - assert(++refs); - s[-1] = (refs<>SDS_TYPE_BITS; - assert(refs); - if (!(--refs)) - zfree(sh); - else - s[-1] = (refs<= addlen) return s; @@ -207,7 +202,6 @@ sds sdsMakeRoomFor(sds s, size_t addlen) { else newlen += SDS_MAX_PREALLOC; - assert(!(s[-1]>>SDS_TYPE_BITS));/* verify that the ref count is 0 (non ref count managed string) */ type = sdsReqType(newlen); hdrlen = sdsHdrSize(type); if (oldtype==type) { @@ -215,7 +209,8 @@ sds sdsMakeRoomFor(sds s, size_t addlen) { if (newsh == NULL) return NULL; s = (char*)newsh+hdrlen; } else { - /* since the header size changes, need to move the string forward, and can't use realloc */ + /* Since the header size changes, need to move the string forward, + * and can't use realloc */ newsh = zmalloc(hdrlen+newlen+1); if (newsh == NULL) return NULL; memcpy((char*)newsh+hdrlen, s, len+1); @@ -225,7 +220,6 @@ sds sdsMakeRoomFor(sds s, size_t addlen) { sdssetlen(s, len); } sdssetalloc(s, newlen); - s[-1] = type; return s; } @@ -237,7 +231,7 @@ sds sdsMakeRoomFor(sds s, size_t addlen) { * references must be substituted with the new pointer returned by the call. */ sds sdsRemoveFreeSpace(sds s) { void *sh, *newsh; - char type, oldtype = s[-1]; + char type, oldtype = s[-1] & SDS_TYPE_MASK; int hdrlen; size_t len = sdslen(s); sh = (char*)s-sdsHdrSize(oldtype); @@ -258,8 +252,6 @@ sds sdsRemoveFreeSpace(sds s) { sdssetlen(s, len); } sdssetalloc(s, len); - assert(!(s[-1]>>SDS_TYPE_BITS));/* verify that the ref count is 0 (non ref count managed string) */ - s[-1] = type; return s; } @@ -275,7 +267,7 @@ size_t sdsAllocSize(sds s) { return sdsHdrSize(s[-1])+alloc+1; } -/* Return the size consumed from the allocator, +/* Return the size consumed from the allocator, * including internal fragmentation */ size_t sdsZmallocSize(sds s) { struct sdshdr *sh = (void*) (s-sdsHdrSize(s[-1])); @@ -306,9 +298,17 @@ size_t sdsZmallocSize(sds s) { * sdsIncrLen(s, nread); */ void sdsIncrLen(sds s, int incr) { - char flags = s[-1]; + unsigned char flags = s[-1]; size_t len; switch(flags&SDS_TYPE_MASK) { + case SDS_TYPE_5: { + unsigned char *fp = ((unsigned char*)s)-1; + unsigned char oldlen = SDS_TYPE_5_LEN(flags); + assert((incr > 0 && oldlen+incr < 32) || (incr < 0 && oldlen >= (unsigned int)(-incr))); + *fp = SDS_TYPE_5 | ((oldlen+1) << SDS_TYPE_BITS); + len = oldlen+1; + break; + } case SDS_TYPE_8: { SDS_HDR_VAR(8,s); assert((incr >= 0 && sh->alloc-sh->len >= incr) || (incr < 0 && sh->len >= (unsigned int)(-incr))); @@ -1069,11 +1069,8 @@ sds sdsjoin(char **argv, int argc, char *sep) { #include "limits.h" #define UNUSED(x) (void)(x) -int sdsTest(int argc, char *argv[]) { - UNUSED(argc); - UNUSED(argv); +int sdsTest(void) { { - struct sdshdr *sh; sds x = sdsnew("foo"), y; test_cond("Create a string and obtain the length", @@ -1109,6 +1106,7 @@ int sdsTest(int argc, char *argv[]) { sdslen(x) == 60 && memcmp(x,"--Hello Hi! World -9223372036854775808," "9223372036854775807--",60) == 0) + printf("[%s]\n",x); sdsfree(x); x = sdsnew("--"); @@ -1195,6 +1193,7 @@ int sdsTest(int argc, char *argv[]) { test_cond("sdscatrepr(...data...)", memcmp(y,"\"\\a\\n\\x00foo\\r\"",15) == 0) +#if 0 { unsigned int oldfree; @@ -1215,6 +1214,7 @@ int sdsTest(int argc, char *argv[]) { sdsfree(x); } +#endif } test_report() return 0; diff --git a/src/sds.h b/src/sds.h index 9201a751c..1fcbe1155 100644 --- a/src/sds.h +++ b/src/sds.h @@ -39,43 +39,53 @@ typedef char *sds; +/* Note: sdshdr5 is never used, we just access the flags byte directly. + * However is here to document the layout of type 5 SDS strings. */ +struct __attribute__ ((__packed__)) sdshdr5 { + unsigned char flags; /* 3 lsb of type, and 5 msb of string length */ + char buf[]; +}; struct __attribute__ ((__packed__)) sdshdr8 { uint8_t len; /* used */ uint8_t alloc; /* excluding the header and null terminator */ - char flags; /* 2 lsb of type, and 6 msb of refcount */ + unsigned char flags; /* 3 lsb of type, 5 unused bits */ char buf[]; }; struct __attribute__ ((__packed__)) sdshdr16 { uint16_t len; /* used */ uint16_t alloc; /* excluding the header and null terminator */ - char flags; /* 2 lsb of type, and 6 msb of refcount */ + unsigned char flags; /* 3 lsb of type, 5 unused bits */ char buf[]; }; struct __attribute__ ((__packed__)) sdshdr32 { uint32_t len; /* used */ uint32_t alloc; /* excluding the header and null terminator */ - char flags; /* 2 lsb of type, and 6 msb of refcount */ + unsigned char flags; /* 3 lsb of type, 5 unused bits */ char buf[]; }; struct __attribute__ ((__packed__)) sdshdr64 { uint64_t len; /* used */ uint64_t alloc; /* excluding the header and null terminator */ - char flags; /* 2 lsb of type, and 6 msb of refcount */ + unsigned char flags; /* 3 lsb of type, 5 unused bits */ char buf[]; }; -#define SDS_TYPE_8 0 -#define SDS_TYPE_16 1 -#define SDS_TYPE_32 2 -#define SDS_TYPE_64 3 -#define SDS_TYPE_MASK 3 -#define SDS_TYPE_BITS 2 +#define SDS_TYPE_5 0 +#define SDS_TYPE_8 1 +#define SDS_TYPE_16 2 +#define SDS_TYPE_32 3 +#define SDS_TYPE_64 4 +#define SDS_TYPE_MASK 7 +#define SDS_TYPE_BITS 3 #define SDS_HDR_VAR(T,s) struct sdshdr##T *sh = (void*)((s)-(sizeof(struct sdshdr##T))); #define SDS_HDR(T,s) ((struct sdshdr##T *)((s)-(sizeof(struct sdshdr##T)))) +#define SDS_TYPE_5_LEN(f) ((f)>>SDS_TYPE_BITS) static inline size_t sdslen(const sds s) { - char flags = s[-1]; + unsigned char flags = s[-1]; switch(flags&SDS_TYPE_MASK) { + case SDS_TYPE_5: + return SDS_TYPE_5_LEN(flags); case SDS_TYPE_8: return SDS_HDR(8,s)->len; case SDS_TYPE_16: @@ -89,8 +99,11 @@ static inline size_t sdslen(const sds s) { } static inline size_t sdsavail(const sds s) { - char flags = s[-1]; + unsigned char flags = s[-1]; switch(flags&SDS_TYPE_MASK) { + case SDS_TYPE_5: { + return 0; + } case SDS_TYPE_8: { SDS_HDR_VAR(8,s); return sh->alloc - sh->len; @@ -112,8 +125,14 @@ static inline size_t sdsavail(const sds s) { } static inline void sdssetlen(sds s, size_t newlen) { - char flags = s[-1]; + unsigned char flags = s[-1]; switch(flags&SDS_TYPE_MASK) { + case SDS_TYPE_5: + { + unsigned char *fp = ((unsigned char*)s)-1; + *fp = SDS_TYPE_5 | (newlen << SDS_TYPE_BITS); + } + break; case SDS_TYPE_8: SDS_HDR(8,s)->len = newlen; break; @@ -130,8 +149,15 @@ static inline void sdssetlen(sds s, size_t newlen) { } static inline void sdsinclen(sds s, size_t inc) { - char flags = s[-1]; + unsigned char flags = s[-1]; switch(flags&SDS_TYPE_MASK) { + case SDS_TYPE_5: + { + unsigned char *fp = ((unsigned char*)s)-1; + unsigned char newlen = SDS_TYPE_5_LEN(flags)+inc; + *fp = SDS_TYPE_5 | (newlen << SDS_TYPE_BITS); + } + break; case SDS_TYPE_8: SDS_HDR(8,s)->len += inc; break; @@ -149,8 +175,10 @@ static inline void sdsinclen(sds s, size_t inc) { /* sdsalloc() = sdsavail() + sdslen() */ static inline size_t sdsalloc(const sds s) { - char flags = s[-1]; + unsigned char flags = s[-1]; switch(flags&SDS_TYPE_MASK) { + case SDS_TYPE_5: + return SDS_TYPE_5_LEN(flags); case SDS_TYPE_8: return SDS_HDR(8,s)->alloc; case SDS_TYPE_16: @@ -164,8 +192,11 @@ static inline size_t sdsalloc(const sds s) { } static inline void sdssetalloc(sds s, size_t newlen) { - char flags = s[-1]; + unsigned char flags = s[-1]; switch(flags&SDS_TYPE_MASK) { + case SDS_TYPE_5: + /* Nothing to do, this type has no total allocation info. */ + break; case SDS_TYPE_8: SDS_HDR(8,s)->alloc = newlen; break; @@ -193,11 +224,6 @@ sds sdscatsds(sds s, const sds t); sds sdscpylen(sds s, const char *t, size_t len); sds sdscpy(sds s, const char *t); -/* we can add a reference count on top of any - * existing sds. (max up to 63 references) */ -void sdsIncRefcount(sds s); -void sdsDecRefcount(sds s); - sds sdscatvprintf(sds s, const char *fmt, va_list ap); #ifdef __GNUC__ sds sdscatprintf(sds s, const char *fmt, ...) From 3da97ea67f3b25097d5a57aeda9ce5d94461035e Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 16 Jul 2015 09:14:39 +0200 Subject: [PATCH 5/7] Add sdshdr5 to DEBUG structsize. --- src/debug.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/debug.c b/src/debug.c index 3f7a85357..0d2f24245 100644 --- a/src/debug.c +++ b/src/debug.c @@ -423,6 +423,7 @@ void debugCommand(redisClient *c) { sizes = sdscatprintf(sizes,"bits:%d ",(sizeof(void*) == 8)?64:32); sizes = sdscatprintf(sizes,"robj:%d ",(int)sizeof(robj)); sizes = sdscatprintf(sizes,"dictentry:%d ",(int)sizeof(dictEntry)); + sizes = sdscatprintf(sizes,"sdshdr5:%d ",(int)sizeof(struct sdshdr5)); sizes = sdscatprintf(sizes,"sdshdr8:%d ",(int)sizeof(struct sdshdr8)); sizes = sdscatprintf(sizes,"sdshdr16:%d ",(int)sizeof(struct sdshdr16)); sizes = sdscatprintf(sizes,"sdshdr32:%d ",(int)sizeof(struct sdshdr32)); From cf68f4ee6a4c466b893fbb269f6aff14c7c75e6a Mon Sep 17 00:00:00 2001 From: antirez Date: Mon, 20 Jul 2015 16:18:06 +0200 Subject: [PATCH 6/7] Fix SDS type 5 sdsIncrLen() bug and added test. Thanks to @oranagra for spotting this error. --- src/sds.c | 42 +++++++++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 15 deletions(-) diff --git a/src/sds.c b/src/sds.c index 4771b6b3e..8283ec438 100644 --- a/src/sds.c +++ b/src/sds.c @@ -305,8 +305,8 @@ void sdsIncrLen(sds s, int incr) { unsigned char *fp = ((unsigned char*)s)-1; unsigned char oldlen = SDS_TYPE_5_LEN(flags); assert((incr > 0 && oldlen+incr < 32) || (incr < 0 && oldlen >= (unsigned int)(-incr))); - *fp = SDS_TYPE_5 | ((oldlen+1) << SDS_TYPE_BITS); - len = oldlen+1; + *fp = SDS_TYPE_5 | ((oldlen+incr) << SDS_TYPE_BITS); + len = oldlen+incr; break; } case SDS_TYPE_8: { @@ -1193,28 +1193,40 @@ int sdsTest(void) { test_cond("sdscatrepr(...data...)", memcmp(y,"\"\\a\\n\\x00foo\\r\"",15) == 0) -#if 0 { unsigned int oldfree; + char *p; + int step = 10, j, i; sdsfree(x); sdsfree(y); x = sdsnew("0"); - sh = (void*) (x-(sizeof(struct sdshdr))); - test_cond("sdsnew() free/len buffers", sh->len == 1 && sh->free == 0); - x = sdsMakeRoomFor(x,1); - sh = (void*) (x-(sizeof(struct sdshdr))); - test_cond("sdsMakeRoomFor()", sh->len == 1 && sh->free > 0); - oldfree = sh->free; - x[1] = '1'; - sdsIncrLen(x,1); - test_cond("sdsIncrLen() -- content", x[0] == '0' && x[1] == '1'); - test_cond("sdsIncrLen() -- len", sh->len == 2); - test_cond("sdsIncrLen() -- free", sh->free == oldfree-1); + test_cond("sdsnew() free/len buffers", sdslen(x) == 1 && sdsavail(x) == 0); + + /* Run the test a few times in order to hit the first two + * SDS header types. */ + for (i = 0; i < 10; i++) { + int oldlen = sdslen(x); + x = sdsMakeRoomFor(x,step); + int type = x[-1]&SDS_TYPE_MASK; + + test_cond("sdsMakeRoomFor() len", sdslen(x) == oldlen); + if (type != SDS_TYPE_5) { + test_cond("sdsMakeRoomFor() free", sdsavail(x) >= step); + oldfree = sdsavail(x); + } + p = x+oldlen; + for (j = 0; j < step; j++) { + p[j] = 'A'+j; + } + sdsIncrLen(x,step); + } + test_cond("sdsMakeRoomFor() content", + memcmp("0ABCDEFGHIJABCDEFGHIJABCDEFGHIJABCDEFGHIJABCDEFGHIJABCDEFGHIJABCDEFGHIJABCDEFGHIJABCDEFGHIJABCDEFGHIJ",x,101) == 0); + test_cond("sdsMakeRoomFor() final length",sdslen(x)==101); sdsfree(x); } -#endif } test_report() return 0; From ea9bd243ecf02760ac7a5e9a25bd2d067b71ee84 Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 23 Jul 2015 09:16:47 +0200 Subject: [PATCH 7/7] SDS: use type 8 if we are likely to append to the string. When empty strings are created, or when sdsMakeRoomFor() is called, we are likely into an appending pattern. Use at least type 8 SDS strings since TYPE 5 does not remember the free allocation size and requires to call sdsMakeRoomFor() at every new piece appended. --- src/sds.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/sds.c b/src/sds.c index 8283ec438..c9a6583ee 100644 --- a/src/sds.c +++ b/src/sds.c @@ -80,6 +80,9 @@ sds sdsnewlen(const void *init, size_t initlen) { void *sh; sds s; char type = sdsReqType(initlen); + /* Empty strings are usually created in order to append. Use type 8 + * since type 5 is not good at this. */ + if (type == SDS_TYPE_5 && initlen == 0) type = SDS_TYPE_8; int hdrlen = sdsHdrSize(type); unsigned char *fp; /* flags pointer. */ @@ -193,7 +196,9 @@ sds sdsMakeRoomFor(sds s, size_t addlen) { char type, oldtype = s[-1] & SDS_TYPE_MASK; int hdrlen; + /* Return ASAP if there is enough space left. */ if (avail >= addlen) return s; + len = sdslen(s); sh = (char*)s-sdsHdrSize(oldtype); newlen = (len+addlen); @@ -203,6 +208,12 @@ sds sdsMakeRoomFor(sds s, size_t addlen) { newlen += SDS_MAX_PREALLOC; type = sdsReqType(newlen); + + /* Don't use type 5: the user is appending to the string and type 5 is + * not able to remember empty space, so sdsMakeRoomFor() must be called + * at every appending operation. */ + if (type == SDS_TYPE_5) type = SDS_TYPE_8; + hdrlen = sdsHdrSize(type); if (oldtype==type) { newsh = zrealloc(sh, hdrlen+newlen+1);