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 <[email protected]>
Signed-off-by: Chen Tianjie <[email protected]>

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 {

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 {

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 */

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
     } {