From badb709a8ff694f2ed5a495cf0064846be81e209 Mon Sep 17 00:00:00 2001 From: John Sully Date: Fri, 27 Dec 2019 17:45:56 -0500 Subject: [PATCH 1/7] Ignore dependency files Former-commit-id: 6e06e0dfc7cd572d93cce99c4b8b8b0b59e95e60 --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index c5cba54dd..f6e6abb12 100644 --- a/.gitignore +++ b/.gitignore @@ -1,6 +1,7 @@ .*.swp core *.o +*.d *.log dump.rdb redis-benchmark From da17594170d444d944698e5be71c616296900a41 Mon Sep 17 00:00:00 2001 From: John Sully Date: Fri, 27 Dec 2019 18:17:22 -0500 Subject: [PATCH 2/7] Fix some static analysis warnings Former-commit-id: 42a8f22c21706f9ddcaa63ceafc5ad817c1fe876 --- src/ae.cpp | 15 +++++++++------ src/rdb.cpp | 2 +- src/replication.cpp | 2 +- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/ae.cpp b/src/ae.cpp index 87212f704..90c148510 100644 --- a/src/ae.cpp +++ b/src/ae.cpp @@ -84,7 +84,7 @@ fastlock g_lock("AE (global)"); #endif thread_local aeEventLoop *g_eventLoopThisThread = NULL; -#define AE_ASSERT(x) if (!(x)) do { fprintf(stderr, "AE_ASSERT FAILURE %s: %d\n", __FILE__, __LINE__); *((volatile int*)0) = 1; } while(0) +#define AE_ASSERT(x) if (!(x)) do { fprintf(stderr, "AE_ASSERT FAILURE %s: %d\n", __FILE__, __LINE__); *((volatile int*)1) = 1; } while(0) /* Include the best multiplexing layer supported by this system. * The following should be ordered by performances, descending. */ @@ -237,11 +237,11 @@ int aeCreateRemoteFileEvent(aeEventLoop *eventLoop, int fd, int mask, cmd.clientData = clientData; cmd.pctl = nullptr; if (fSynchronous) + { cmd.pctl = new (MALLOC_LOCAL) aeCommandControl(); - - std::unique_lock ulock(cmd.pctl->mutexcv, std::defer_lock); - if (fSynchronous) cmd.pctl->mutexcv.lock(); + } + auto size = safe_write(eventLoop->fdCmdWrite, &cmd, sizeof(cmd)); if (size != sizeof(cmd)) { @@ -252,6 +252,7 @@ int aeCreateRemoteFileEvent(aeEventLoop *eventLoop, int fd, int mask, if (fSynchronous) { + std::unique_lock ulock(cmd.pctl->mutexcv, std::defer_lock); cmd.pctl->cv.wait(ulock); ret = cmd.pctl->rval; delete cmd.pctl; @@ -289,15 +290,17 @@ int aePostFunction(aeEventLoop *eventLoop, std::function fn, bool fSynch cmd.pfn = new (MALLOC_LOCAL) std::function(fn); cmd.pctl = nullptr; if (fSynchronous) + { cmd.pctl = new (MALLOC_LOCAL) aeCommandControl(); - std::unique_lock ulock(cmd.pctl->mutexcv, std::defer_lock); - if (fSynchronous) cmd.pctl->mutexcv.lock(); + } + auto size = write(eventLoop->fdCmdWrite, &cmd, sizeof(cmd)); AE_ASSERT(size == sizeof(cmd)); int ret = AE_OK; if (fSynchronous) { + std::unique_lock ulock(cmd.pctl->mutexcv, std::defer_lock); cmd.pctl->cv.wait(ulock); ret = cmd.pctl->rval; delete cmd.pctl; diff --git a/src/rdb.cpp b/src/rdb.cpp index 1c5b25d16..b76afdfd7 100644 --- a/src/rdb.cpp +++ b/src/rdb.cpp @@ -1919,7 +1919,7 @@ int rdbLoadRio(rio *rdb, rdbSaveInfo *rsi, int loading_aof) { redisDb *db = g_pserver->db+0; char buf[1024]; /* Key-specific attributes, set by opcodes before the key type. */ - long long lru_idle = -1, lfu_freq = -1, expiretime = -1, now = mstime(); + long long lru_idle = -1, lfu_freq = -1, expiretime = -1, now; long long lru_clock = 0; uint64_t mvcc_tstamp = OBJ_MVCC_INVALID; robj *subexpireKey = nullptr; diff --git a/src/replication.cpp b/src/replication.cpp index b7eb13aeb..ec2bbea63 100644 --- a/src/replication.cpp +++ b/src/replication.cpp @@ -3061,7 +3061,7 @@ void replicationCron(void) { } /* Timed out master when we are an already connected replica? */ - if (mi->masterhost && mi->repl_state == REPL_STATE_CONNECTED && + if (mi->masterhost && mi->master && mi->repl_state == REPL_STATE_CONNECTED && (time(NULL)-mi->master->lastinteraction) > g_pserver->repl_timeout) { serverLog(LL_WARNING,"MASTER timeout: no data nor PING received..."); From 1790b02984e1c2bb527ef0e8e8496da5c85ae7c3 Mon Sep 17 00:00:00 2001 From: John Sully Date: Wed, 1 Jan 2020 11:52:00 -0500 Subject: [PATCH 3/7] Fix issue #130 due to fastlock timeout reduction Former-commit-id: dbef17c2e16f115733242721e9b5a43f01e7a554 --- src/fastlock.cpp | 2 +- src/fastlock_x64.asm | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/fastlock.cpp b/src/fastlock.cpp index d566bb267..19375cd0e 100644 --- a/src/fastlock.cpp +++ b/src/fastlock.cpp @@ -293,7 +293,7 @@ extern "C" void fastlock_lock(struct fastlock *lock) #elif defined(__arm__) __asm__ __volatile__ ("yield"); #endif - if ((++cloops % 0x10000) == 0) + if ((++cloops % 0x100000) == 0) { fastlock_sleep(lock, tid, ticketT.u, mask); } diff --git a/src/fastlock_x64.asm b/src/fastlock_x64.asm index f7ab6316e..7c9990a6d 100644 --- a/src/fastlock_x64.asm +++ b/src/fastlock_x64.asm @@ -45,7 +45,7 @@ fastlock_lock: cmp dx, ax # is our ticket up? je .LLocked # leave the loop pause - add ecx, 0x10000 # Have we been waiting a long time? (oflow if we have) + add ecx, 0x1000 # Have we been waiting a long time? (oflow if we have) # 1000h is set so we overflow on the 1024*1024'th iteration (like the C code) jnc .LLoop # If so, give up our timeslice to someone who's doing real work # Like the compiler, you're probably thinking: "Hey! I should take these pushs out of the loop" From 20e529c441c3948ff6fc720cf9d4cf763f2e2647 Mon Sep 17 00:00:00 2001 From: John Sully Date: Wed, 1 Jan 2020 19:13:48 -0500 Subject: [PATCH 4/7] C++ wrapper classes for SDS Former-commit-id: 45817db8c3a86815945359113dcbccfde4257ce5 --- src/sds.c | 43 +++++++++++++++-- src/sds.h | 138 +++++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 176 insertions(+), 5 deletions(-) diff --git a/src/sds.c b/src/sds.c index 637db9663..e1678a95b 100644 --- a/src/sds.c +++ b/src/sds.c @@ -53,11 +53,18 @@ static inline int sdsHdrSize(char type) { return sizeof(struct sdshdr32); case SDS_TYPE_64: return sizeof(struct sdshdr64); + case SDS_TYPE_REFCOUNTED: + return sizeof(struct sdshdrrefcount); } return 0; } -static inline char sdsReqType(size_t string_size) { +static inline char sdsReqType(ssize_t string_size) { + if (string_size < 0){ + string_size = -string_size; + if (string_size < 1<<16) + return SDS_TYPE_REFCOUNTED; + } if (string_size < 1<<5) return SDS_TYPE_5; if (string_size < 1<<8) @@ -86,10 +93,12 @@ static inline char sdsReqType(size_t string_size) { * You can print the string with printf() as there is an implicit \0 at the * 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) { +sds sdsnewlen(const void *init, ssize_t initlen) { void *sh; sds s; char type = sdsReqType(initlen); + if (initlen < 0) + initlen = -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; @@ -137,6 +146,13 @@ sds sdsnewlen(const void *init, size_t initlen) { *fp = type; break; } + case SDS_TYPE_REFCOUNTED: { + SDS_HDR_VAR_REFCOUNTED(s); + sh->len = initlen; + sh->refcount = 1; + *fp = type; + break; + } } if (initlen && init) memcpy(s, init, initlen); @@ -161,9 +177,25 @@ sds sdsdup(const char *s) { return sdsnewlen(s, sdslen(s)); } +sds sdsdupshared(const char *s) { + unsigned char flags = s[-1]; + if ((flags & SDS_TYPE_MASK) != SDS_TYPE_REFCOUNTED) + return sdsnewlen(s, -sdslen(s)); + SDS_HDR_VAR_REFCOUNTED(s); + __atomic_fetch_add(&sh->refcount, 1, __ATOMIC_RELAXED); + return (sds)s; +} + /* Free an sds string. No operation is performed if 's' is NULL. */ void sdsfree(const char *s) { if (s == NULL) return; + unsigned char flags = s[-1]; + if ((flags & SDS_TYPE_MASK) == SDS_TYPE_REFCOUNTED) + { + SDS_HDR_VAR_REFCOUNTED(s); + if (__atomic_fetch_sub(&sh->refcount, 1, __ATOMIC_RELAXED) > 1) + return; + } s_free((char*)s-sdsHdrSize(s[-1])); } @@ -368,6 +400,11 @@ void sdsIncrLen(sds s, ssize_t incr) { len = (sh->len += incr); break; } + case SDS_TYPE_REFCOUNTED: { + SDS_HDR_VAR_REFCOUNTED(s); + len = (sh->len += incr); + break; + } default: len = 0; /* Just to avoid compilation warnings. */ } s[len] = '\0'; @@ -787,7 +824,7 @@ void sdstoupper(sds s) { * If two strings share exactly the same prefix, but one of the two has * additional characters, the longer string is considered to be greater than * the smaller one. */ -int sdscmp(const sds s1, const sds s2) { +int sdscmp(const char *s1, const char *s2) { size_t l1, l2, minlen; int cmp; diff --git a/src/sds.h b/src/sds.h index 7f6f141e0..23a11afa4 100644 --- a/src/sds.h +++ b/src/sds.h @@ -91,15 +91,27 @@ struct __attribute__ ((__packed__)) sdshdr64 { #endif }; +struct __attribute__ ((__packed__)) sdshdrrefcount { + uint64_t len; /* used */ + uint16_t refcount; + unsigned char flags; /* 3 lsb of type, 5 unused bits */ +#ifndef __cplusplus + char buf[]; +#endif +}; + #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_REFCOUNTED 5 #define SDS_TYPE_MASK 7 #define SDS_TYPE_BITS 3 #define SDS_HDR_VAR(T,s) struct sdshdr##T *sh = (struct sdshdr##T *)(((void*)((s)-(sizeof(struct sdshdr##T))))); +#define SDS_HDR_VAR_REFCOUNTED(s) struct sdshdrrefcount *sh = (struct sdshdrrefcount *)(((void*)((s)-(sizeof(struct sdshdrrefcount))))); #define SDS_HDR(T,s) ((struct sdshdr##T *)((s)-(sizeof(struct sdshdr##T)))) +#define SDS_HDR_REFCOUNTED(s) ((struct sdshdrrefcount *)((s)-(sizeof(struct sdshdrrefcount)))) #define SDS_TYPE_5_LEN(f) ((f)>>SDS_TYPE_BITS) static inline size_t sdslen(const char *s) { @@ -121,6 +133,8 @@ static inline size_t sdslen(const char *s) { return SDS_HDR(32,s)->len; case SDS_TYPE_64: return SDS_HDR(64,s)->len; + case SDS_TYPE_REFCOUNTED: + return SDS_HDR_REFCOUNTED(s)->len; } } return 0; @@ -148,6 +162,9 @@ static inline size_t sdsavail(const char * s) { SDS_HDR_VAR(64,s); return sh->alloc - sh->len; } + case SDS_TYPE_REFCOUNTED: { + return 0; // immutable + } } return 0; } @@ -173,6 +190,9 @@ static inline void sdssetlen(sds s, size_t newlen) { case SDS_TYPE_64: SDS_HDR(64,s)->len = newlen; break; + case SDS_TYPE_REFCOUNTED: + SDS_HDR_REFCOUNTED(s)->len = newlen; + break; } } @@ -198,6 +218,9 @@ static inline void sdsinclen(sds s, size_t inc) { case SDS_TYPE_64: SDS_HDR(64,s)->len += inc; break; + case SDS_TYPE_REFCOUNTED: + SDS_HDR_REFCOUNTED(s)->len += inc; + break; } } @@ -215,6 +238,8 @@ static inline size_t sdsalloc(const sds s) { return SDS_HDR(32,s)->alloc; case SDS_TYPE_64: return SDS_HDR(64,s)->alloc; + case SDS_TYPE_REFCOUNTED: + return SDS_HDR_REFCOUNTED(s)->len; } return 0; } @@ -237,13 +262,22 @@ static inline void sdssetalloc(sds s, size_t newlen) { case SDS_TYPE_64: SDS_HDR(64,s)->alloc = newlen; break; + case SDS_TYPE_REFCOUNTED: + break; } } -sds sdsnewlen(const void *init, size_t initlen); +static inline int sdsisshared(const char *s) +{ + unsigned char flags = s[-1]; + return ((flags & SDS_TYPE_MASK) == SDS_TYPE_REFCOUNTED); +} + +sds sdsnewlen(const void *init, ssize_t initlen); sds sdsnew(const char *init); sds sdsempty(void); sds sdsdup(const char *s); +sds sdsdupshared(const char *s); void sdsfree(const char *s); sds sdsgrowzero(sds s, size_t len); sds sdscatlen(sds s, const void *t, size_t len); @@ -265,7 +299,7 @@ sds sdstrim(sds s, const char *cset); void sdsrange(sds s, ssize_t start, ssize_t end); void sdsupdatelen(sds s); void sdsclear(sds s); -int sdscmp(const sds s1, const sds s2); +int sdscmp(const char *s1, const char *s2); sds *sdssplitlen(const char *s, ssize_t len, const char *sep, int seplen, int *count); void sdsfreesplitres(sds *tokens, int count); void sdstolower(sds s); @@ -298,6 +332,106 @@ int sdsTest(int argc, char *argv[]); #ifdef __cplusplus } + +class sdsview +{ +protected: + sds m_str = nullptr; + + sdsview() = default; // Not allowed to create a sdsview directly with a nullptr +public: + sdsview(sds str) + : m_str(str) + {} + + sdsview(const char *str) + : m_str((sds)str) + {} + + bool operator<(const sdsview &other) const + { + return sdscmp(m_str, other.m_str) < 0; + } + + bool operator==(const sdsview &other) const + { + return sdscmp(m_str, other.m_str) == 0; + } + + bool operator==(const char *other) const + { + return sdscmp(m_str, other) == 0; + } + + char operator[](size_t idx) const + { + return m_str[idx]; + } + + size_t size() const + { + return sdslen(m_str); + } + + const char *get() const { return m_str; } + + explicit operator const char*() const { return m_str; } +}; + +class sdsstring : public sdsview +{ +public: + sdsstring() = default; + explicit sdsstring(sds str) + : sdsview(str) + {} + + sdsstring(const sdsstring &other) + : sdsview(sdsdup(other.m_str)) + {} + + sdsstring(sdsstring &&other) + : sdsview(other.m_str) + { + other.m_str = nullptr; + } + + ~sdsstring() + { + sdsfree(m_str); + } +}; + +class sdsimmutablestring : public sdsstring +{ +public: + sdsimmutablestring() = default; + explicit sdsimmutablestring(sds str) + : sdsstring(str) + {} + + explicit sdsimmutablestring(const char *str) + : sdsstring((sds)str) + {} + + sdsimmutablestring(const sdsimmutablestring &other) + : sdsstring(sdsdupshared(other.m_str)) + {} + + sdsimmutablestring(sdsimmutablestring &&other) + : sdsstring(other.m_str) + { + other.m_str = nullptr; + } + + auto &operator=(const sdsimmutablestring &other) + { + sdsfree(m_str); + m_str = sdsdupshared(other.m_str); + return *this; + } +}; + #endif #endif From da917c4de5f4c98e76f648a4898286197107d8a6 Mon Sep 17 00:00:00 2001 From: John Sully Date: Wed, 1 Jan 2020 20:41:17 -0500 Subject: [PATCH 5/7] Fix issue where expire is lost when performing a defrag Former-commit-id: aea333bb78fafabbddb340dfd4c232c2e207cfba --- src/defrag.cpp | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/src/defrag.cpp b/src/defrag.cpp index c49cd2665..1b5596609 100644 --- a/src/defrag.cpp +++ b/src/defrag.cpp @@ -48,7 +48,7 @@ extern "C" int je_get_defrag_hint(void* ptr, int *bin_util, int *run_util); /* forward declarations*/ void defragDictBucketCallback(void *privdata, dictEntry **bucketref); dictEntry* replaceSateliteDictKeyPtrAndOrDefragDictEntry(dict *d, sds oldkey, sds newkey, uint64_t hash, long *defragged); -void replaceSateliteOSetKeyPtr(expireset &set, sds oldkey, sds newkey); +bool replaceSateliteOSetKeyPtr(expireset &set, sds oldkey, sds newkey); /* Defrag helper for generic allocations. * @@ -407,7 +407,7 @@ dictEntry* replaceSateliteDictKeyPtrAndOrDefragDictEntry(dict *d, sds oldkey, sd return NULL; } -void replaceSateliteOSetKeyPtr(expireset &set, sds oldkey, sds newkey) { +bool replaceSateliteOSetKeyPtr(expireset &set, sds oldkey, sds newkey) { auto itr = set.find(oldkey); if (itr != set.end()) { @@ -415,7 +415,10 @@ void replaceSateliteOSetKeyPtr(expireset &set, sds oldkey, sds newkey) { eNew.setKeyUnsafe(newkey); set.erase(itr); set.insert(eNew); + serverAssert(set.find(newkey) != set.end()); + return true; } + return false; } long activeDefragQuickListNodes(quicklist *ql) { @@ -777,16 +780,22 @@ long defragKey(redisDb *db, dictEntry *de) { long defragged = 0; sds newsds; + ob = (robj*)dictGetVal(de); + /* Try to defrag the key name. */ newsds = activeDefragSds(keysds); if (newsds) + { defragged++, de->key = newsds; - if (!db->setexpire->empty()) { - replaceSateliteOSetKeyPtr(*db->setexpire, keysds, newsds); + if (!db->setexpire->empty()) { + bool fReplaced = replaceSateliteOSetKeyPtr(*db->setexpire, keysds, newsds); + serverAssert(fReplaced == ob->FExpires()); + } else { + serverAssert(!ob->FExpires()); + } } /* Try to defrag robj and / or string value. */ - ob = (robj*)dictGetVal(de); if ((newob = activeDefragStringOb(ob, &defragged))) { de->v.val = newob; ob = newob; @@ -839,6 +848,7 @@ long defragKey(redisDb *db, dictEntry *de) { } else { serverPanic("Unknown object type"); } + return defragged; } From 6370309b58e929774daefb17ab6a93055dcc7434 Mon Sep 17 00:00:00 2001 From: John Sully Date: Thu, 2 Jan 2020 15:36:02 -0500 Subject: [PATCH 6/7] Drop severity of master disconnect log when multimaster is enabled Former-commit-id: edb993d52b25c30392c6eb1e60896498f991a223 --- src/replication.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/replication.cpp b/src/replication.cpp index ec2bbea63..b8236420d 100644 --- a/src/replication.cpp +++ b/src/replication.cpp @@ -2298,7 +2298,8 @@ int connectWithMaster(redisMaster *mi) { fd = anetTcpNonBlockBestEffortBindConnect(NULL, mi->masterhost,mi->masterport,NET_FIRST_BIND_ADDR); if (fd == -1) { - serverLog(LL_WARNING,"Unable to connect to MASTER: %s", + int sev = g_pserver->enable_multimaster ? LL_NOTICE : LL_WARNING; // with multimaster its not unheard of to intentiallionall have downed masters + serverLog(sev,"Unable to connect to MASTER: %s", strerror(errno)); return C_ERR; } From fc4d9e6d1fbacdc9064a53392c719ebaea4e41bc Mon Sep 17 00:00:00 2001 From: John Sully Date: Fri, 3 Jan 2020 16:50:13 -0500 Subject: [PATCH 7/7] subkey expire testes Former-commit-id: 0cf3af6857c192bd03656c28b5a0a2bb11416b8c --- tests/unit/expire.tcl | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/tests/unit/expire.tcl b/tests/unit/expire.tcl index de24eabed..477df0242 100644 --- a/tests/unit/expire.tcl +++ b/tests/unit/expire.tcl @@ -219,4 +219,37 @@ start_server {tags {"expire"}} { set ttl [r ttl foo] assert {$ttl <= 98 && $ttl > 90} } + + test { EXPIREMEMBER works (set) } { + r flushall + r sadd testkey foo bar baz + r expiremember testkey foo 1 + after 1500 + assert_equal {2} [r scard testkey] + } + + test { EXPIREMEMBER works (hash) } { + r flushall + r hset testkey foo bar + r expiremember testkey foo 1 + after 1500 + r exists testkey + } {0} + + test { EXPIREMEMBER works (zset) } { + r flushall + r zadd testkey 1 foo + r zadd testkey 2 bar + assert_equal {2} [r zcard testkey] + r expiremember testkey foo 1 + after 1500 + assert_equal {1} [r zcard testkey] + } + + test { TTL for subkey expires works } { + r flushall + r sadd testkey foo bar baz + r expiremember testkey foo 10000 + assert [expr [r ttl testkey foo] > 0] + } }