From d16b4ec1b9a2309f17601abb1e238f06dfb736a9 Mon Sep 17 00:00:00 2001 From: Chen Tianjie Date: Sat, 1 Jun 2024 16:09:20 +0800 Subject: [PATCH] Unshare object to avoid LRU/LFU being messed up (#250) When LRU/LFU enabled, Valkey does not allow using shared objects, as value objects may be shared among many different keys and they can't share LRU/LFU information. However `maxmemory-policy` is modifiable at runtime. If LRU/LFU is not enabled at start, but then enabled when some shared objects are already used, there could be some confusion in LRU/LFU information. For `set` command it is OK since it is going to create a new object when LRU/LFU enabled, but `get` command will not unshare the object and just update LRU/LFU information. So we may duplicate the object in this case. It is a one-time task for each key using shared objects, unless this is the case for so many keys, there should be no serious performance degradation. Still, LRU will be updated anyway, no matter LRU/LFU is enabled or not, because `OBJECT IDLETIME` needs it, unless `maxmemory-policy` is set to LFU. So idle time of a key may still be messed up. --------- Signed-off-by: chentianjie.ctj Signed-off-by: Chen Tianjie --- src/db.c | 4 ++++ src/object.c | 6 ++++-- src/server.h | 3 +++ tests/unit/maxmemory.tcl | 15 +++++++++++++++ 4 files changed, 26 insertions(+), 2 deletions(-) diff --git a/src/db.c b/src/db.c index a78c8bad2..2e6d85cf4 100644 --- a/src/db.c +++ b/src/db.c @@ -122,6 +122,10 @@ robj *lookupKey(serverDb *db, robj *key, int flags) { server.current_client->cmd->proc != touchCommand) flags |= LOOKUP_NOTOUCH; if (!hasActiveChildProcess() && !(flags & LOOKUP_NOTOUCH)) { + if (!canUseSharedObject() && val->refcount == OBJ_SHARED_REFCOUNT) { + val = dupStringObject(val); + kvstoreDictSetVal(db->keys, getKeySlot(key->ptr), de, val); + } if (server.maxmemory_policy & MAXMEMORY_FLAG_LFU) { updateLFU(val); } else { diff --git a/src/object.c b/src/object.c index 1a335edd6..ea3538b9e 100644 --- a/src/object.c +++ b/src/object.c @@ -647,8 +647,10 @@ robj *tryObjectEncodingEx(robj *o, int try_trim) { * Note that we avoid using shared integers when maxmemory is used * because every object needs to have a private LRU field for the LRU * algorithm to work well. */ - if ((server.maxmemory == 0 || !(server.maxmemory_policy & MAXMEMORY_FLAG_NO_SHARED_INTEGERS)) && value >= 0 && - value < OBJ_SHARED_INTEGERS) { + if (canUseSharedObject() && + value >= 0 && + value < OBJ_SHARED_INTEGERS) + { decrRefCount(o); return shared.integers[value]; } else { diff --git a/src/server.h b/src/server.h index bac4c33c9..8f273db1e 100644 --- a/src/server.h +++ b/src/server.h @@ -2855,6 +2855,9 @@ int collateStringObjects(const robj *a, const robj *b); int equalStringObjects(robj *a, robj *b); unsigned long long estimateObjectIdleTime(robj *o); void trimStringObjectIfNeeded(robj *o, int trim_small_values); +static inline int canUseSharedObject(void) { + return server.maxmemory == 0 || !(server.maxmemory_policy & MAXMEMORY_FLAG_NO_SHARED_INTEGERS); +} #define sdsEncodedObject(objptr) (objptr->encoding == OBJ_ENCODING_RAW || objptr->encoding == OBJ_ENCODING_EMBSTR) /* Synchronous I/O with timeout */ diff --git a/tests/unit/maxmemory.tcl b/tests/unit/maxmemory.tcl index 92e68ac1e..ee1232796 100644 --- a/tests/unit/maxmemory.tcl +++ b/tests/unit/maxmemory.tcl @@ -168,6 +168,21 @@ start_server {tags {"maxmemory external:skip"}} { r config set maxmemory 0 } + test "Shared integers are unshared with maxmemory and LRU policy" { + r set a 1 + r set b 1 + assert_refcount_morethan a 1 + assert_refcount_morethan b 1 + r config set maxmemory 1073741824 + r config set maxmemory-policy allkeys-lru + r get a + assert_refcount 1 a + r config set maxmemory-policy volatile-lru + r get b + assert_refcount 1 b + r config set maxmemory 0 + } + foreach policy { allkeys-random allkeys-lru allkeys-lfu volatile-lru volatile-lfu volatile-random volatile-ttl } {