Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sds size classes - memory optimization #2509

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ REDIS_SERVER_NAME=redis-server
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
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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

redis-cli uses hiredis (which comes with its own implementation of sds), no need for sds.o in redis-cli.

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
Expand Down
5 changes: 4 additions & 1 deletion src/debug.c
Original file line number Diff line number Diff line change
Expand Up @@ -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,"jemalloc") && c->argc == 3) {
#if defined(USE_JEMALLOC)
Expand Down
33 changes: 13 additions & 20 deletions src/networking.c
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
}
}
Expand Down Expand Up @@ -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);
Expand All @@ -260,22 +253,22 @@ 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));

/* Append to this object when possible. */
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);
Expand All @@ -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);

Expand Down Expand Up @@ -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);
}
}
Expand Down
9 changes: 5 additions & 4 deletions src/object.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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';
Expand All @@ -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);
Expand Down
4 changes: 2 additions & 2 deletions src/redis-cli.c
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@
#include <limits.h>
#include <math.h>

#include "hiredis.h"
#include "sds.h"
#include <hiredis.h>
#include <sds.h> /* use sds.h from hiredis, so that only one set of sds functions will be present in the binary */
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with the previous code (include with double quotes), the result was that redis-cli uses the inline functions of sds from src/sds.h but uses the non-inline functions of sds from the sds.c implementation inside hiredis.

now that these implementations are not the same (different sds headers), this will no longer work!
this change causes redis-cli to use the sds inline functions from hiredis.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not making redis-cli just use the (new) Redis versions of sds.[ch]? Because it exchanges SDS strings with hiredis? Thanks.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can also update the sds.c and sds.h inside hiredis. but i think we should keep the above change.
since hiredis has its own sds implementation, redis-cli should use it, rather than use the one that belongs to the redis-server.

otherwise, if/when there's a difference between the two implementations, redis-cli uses linline functions from one implementation and non-inline functions from the other.

please note that i also made a change in the Makefile to reflect that... although the sds.o there was actually unused (seems like the linker simply ignored it)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep your reasoning makes sense to me, I'm wondering if we should switch all the implementations inside Redis to a single one. But I don't remember if the current hiredis implementation was a bit different. For sure if it was different, the difference was only marginal. Maybe as part of this pull request, I'll try to unify at least sds of Redis, Disque, and antirez/sds :-) Cheers.

#include "zmalloc.h"
#include "linenoise.h"
#include "help.h"
Expand Down
12 changes: 4 additions & 8 deletions src/scripting.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -422,11 +419,10 @@ int luaRedisGenericCommand(lua_State *lua, int raise_error) {
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);
}
Expand Down
Loading