Cleanups related to expiry/eviction (#13591)

1. `dbRandomKey`: excessive call to `dbFindExpires` (will always return
1 if `allvolatile` + anyway called inside `expireIfNeeded`
2. Add `deleteKeyAndPropagate` that is used by both expiry/eviction
3. Change the order of calls in `expireIfNeeded` to save redundant calls
to `keyIsExpired`
4. `expireIfNeeded`: move `OBJ_STATIC_REFCOUNT` to
`deleteKeyAndPropagate`
5. `performEvictions` now uses `deleteEvictedKeyAndPropagate`
6. active-expire: moved `postExecutionUnitOperations` inside
`activeExpireCycleTryExpire`
7. `activeExpireCycleTryExpire`: less indentation + expire a key if `now
== t`
8. rename `lazy_expire_disabled` to `allow_access_expired`

src/db.c

@@ -375,23 +375,22 @@ robj *dbRandomKey(redisDb *db) {
 
         key = dictGetKey(de);
         keyobj = createStringObject(key,sdslen(key));
-        if (dbFindExpires(db, key)) {
+        if (allvolatile && server.masterhost && --maxtries == 0) {
-            if (allvolatile && server.masterhost && --maxtries == 0) {
+            /* If the DB is composed only of keys with an expire set,
-                /* If the DB is composed only of keys with an expire set,
+             * it could happen that all the keys are already logically
-                 * it could happen that all the keys are already logically
+             * expired in the slave, so the function cannot stop because
-                 * expired in the slave, so the function cannot stop because
+             * expireIfNeeded() is false, nor it can stop because
-                 * expireIfNeeded() is false, nor it can stop because
+             * dictGetFairRandomKey() returns NULL (there are keys to return).
-                 * dictGetFairRandomKey() returns NULL (there are keys to return).
+             * To prevent the infinite loop we do some tries, but if there
-                 * To prevent the infinite loop we do some tries, but if there
+             * are the conditions for an infinite loop, eventually we
-                 * are the conditions for an infinite loop, eventually we
+             * return a key name that may be already expired. */
-                 * return a key name that may be already expired. */
+            return keyobj;
-                return keyobj;
-            }
-            if (expireIfNeeded(db,keyobj,0) != KEY_VALID) {
-                decrRefCount(keyobj);
-                continue; /* search for another key. This expired. */
-            }
         }
+        if (expireIfNeeded(db,keyobj,0) != KEY_VALID) {
+            decrRefCount(keyobj);
+            continue; /* search for another key. This expired. */
+        }
+
         return keyobj;
     }
 }
@@ -1985,17 +1984,72 @@ long long getExpire(redisDb *db, robj *key) {
     return dictGetSignedIntegerVal(de);
 }
 
-/* Delete the specified expired key and propagate expire. */
+
-void deleteExpiredKeyAndPropagate(redisDb *db, robj *keyobj) {
+/* Delete the specified expired or evicted key and propagate to replicas.
-    mstime_t expire_latency;
+ * Currently notify_type can only be NOTIFY_EXPIRED or NOTIFY_EVICTED,
-    latencyStartMonitor(expire_latency);
+ * and it affects other aspects like the latency monitor event name and,
-    dbGenericDelete(db,keyobj,server.lazyfree_lazy_expire,DB_FLAG_KEY_EXPIRED);
+ * which config to look for lazy free, stats var to increment, and so on.
-    latencyEndMonitor(expire_latency);
+ *
-    latencyAddSampleIfNeeded("expire-del",expire_latency);
+ * key_mem_freed is an out parameter which contains the estimated
-    notifyKeyspaceEvent(NOTIFY_EXPIRED,"expired",keyobj,db->id);
+ * amount of memory freed due to the trimming (may be NULL) */
+static void deleteKeyAndPropagate(redisDb *db, robj *keyobj, int notify_type, long long *key_mem_freed) {
+    mstime_t latency;
+    int del_flag = notify_type == NOTIFY_EXPIRED ? DB_FLAG_KEY_EXPIRED : DB_FLAG_KEY_EVICTED;
+    int lazy_flag = notify_type == NOTIFY_EXPIRED ? server.lazyfree_lazy_expire : server.lazyfree_lazy_eviction;
+    char *latency_name = notify_type == NOTIFY_EXPIRED ? "expire-del" : "evict-del";
+    char *notify_name = notify_type == NOTIFY_EXPIRED ? "expired" : "evicted";
+
+    /* The key needs to be converted from static to heap before deleted */
+    int static_key = keyobj->refcount == OBJ_STATIC_REFCOUNT;
+    if (static_key) {
+        keyobj = createStringObject(keyobj->ptr, sdslen(keyobj->ptr));
+    }
+
+    serverLog(LL_DEBUG,"key %s %s: deleting it", (char*)keyobj->ptr, notify_type == NOTIFY_EXPIRED ? "expired" : "evicted");
+
+    /* We compute the amount of memory freed by db*Delete() alone.
+     * It is possible that actually the memory needed to propagate
+     * the DEL in AOF and replication link is greater than the one
+     * we are freeing removing the key, but we can't account for
+     * that otherwise we would never exit the loop.
+     *
+     * Same for CSC invalidation messages generated by signalModifiedKey.
+     *
+     * AOF and Output buffer memory will be freed eventually so
+     * we only care about memory used by the key space.
+     *
+     * The code here used to first propagate and then record delta
+     * using only zmalloc_used_memory but in CRDT we can't do that
+     * so we use freeMemoryGetNotCountedMemory to avoid counting
+     * AOF and slave buffers */
+    if (key_mem_freed) *key_mem_freed = (long long) zmalloc_used_memory() - freeMemoryGetNotCountedMemory();
+    latencyStartMonitor(latency);
+    dbGenericDelete(db, keyobj, lazy_flag, del_flag);
+    latencyEndMonitor(latency);
+    latencyAddSampleIfNeeded(latency_name, latency);
+    if (key_mem_freed) *key_mem_freed -= (long long) zmalloc_used_memory() - freeMemoryGetNotCountedMemory();
+
+    notifyKeyspaceEvent(notify_type, notify_name,keyobj, db->id);
     signalModifiedKey(NULL, db, keyobj);
-    propagateDeletion(db,keyobj,server.lazyfree_lazy_expire);
+    propagateDeletion(db, keyobj, lazy_flag);
-    server.stat_expiredkeys++;
+
+    if (notify_type == NOTIFY_EXPIRED)
+        server.stat_expiredkeys++;
+    else
+        server.stat_evictedkeys++;
+
+    if (static_key)
+        decrRefCount(keyobj);
+}
+
+/* Delete the specified expired key and propagate. */
+void deleteExpiredKeyAndPropagate(redisDb *db, robj *keyobj) {
+    deleteKeyAndPropagate(db, keyobj, NOTIFY_EXPIRED, NULL);
+}
+
+/* Delete the specified evicted key and propagate. */
+void deleteEvictedKeyAndPropagate(redisDb *db, robj *keyobj, long long *key_mem_freed) {
+    deleteKeyAndPropagate(db, keyobj, NOTIFY_EVICTED, key_mem_freed);
 }
 
 /* Propagate an implicit key deletion into replicas and the AOF file.
@@ -2085,9 +2139,9 @@ int keyIsExpired(redisDb *db, robj *key) {
  * The function returns KEY_EXPIRED if the key is expired BUT not deleted,
  * or returns KEY_DELETED if the key is expired and deleted. */
 keyStatus expireIfNeeded(redisDb *db, robj *key, int flags) {
-    if ((!keyIsExpired(db,key)) ||
+    if ((server.allow_access_expired) ||
-        (server.lazy_expire_disabled) ||
+        (flags & EXPIRE_ALLOW_ACCESS_EXPIRED) ||
-        (flags & EXPIRE_ALLOW_ACCESS_EXPIRED))
+        (!keyIsExpired(db,key)))
         return KEY_VALID;
 
     /* If we are running in the context of a replica, instead of
@@ -2118,16 +2172,9 @@ keyStatus expireIfNeeded(redisDb *db, robj *key, int flags) {
      * will have failed over and the new primary will send us the expire. */
     if (isPausedActionsWithUpdate(PAUSE_ACTION_EXPIRE)) return KEY_EXPIRED;
 
-    /* The key needs to be converted from static to heap before deleted */
-    int static_key = key->refcount == OBJ_STATIC_REFCOUNT;
-    if (static_key) {
-        key = createStringObject(key->ptr, sdslen(key->ptr));
-    }
     /* Delete the key */
     deleteExpiredKeyAndPropagate(db,key);
-    if (static_key) {
+
-        decrRefCount(key);
-    }
     return KEY_DELETED;
 }
 

src/evict.c

@@ -525,8 +525,7 @@ int performEvictions(void) {
     int keys_freed = 0;
     size_t mem_reported, mem_tofree;
     long long mem_freed; /* May be negative */
-    mstime_t latency, eviction_latency;
+    mstime_t latency;
-    long long delta;
     int slaves = listLength(server.slaves);
     int result = EVICT_FAIL;
 
@@ -659,34 +658,18 @@ int performEvictions(void) {
 
         /* Finally remove the selected key. */
         if (bestkey) {
+            long long key_mem_freed;
             db = server.db+bestdbid;
-            robj *keyobj = createStringObject(bestkey,sdslen(bestkey));
+
-            /* We compute the amount of memory freed by db*Delete() alone.
-             * It is possible that actually the memory needed to propagate
-             * the DEL in AOF and replication link is greater than the one
-             * we are freeing removing the key, but we can't account for
-             * that otherwise we would never exit the loop.
-             *
-             * Same for CSC invalidation messages generated by signalModifiedKey.
-             *
-             * AOF and Output buffer memory will be freed eventually so
-             * we only care about memory used by the key space. */
             enterExecutionUnit(1, 0);
-            delta = (long long) zmalloc_used_memory();
+            robj *keyobj = createStringObject(bestkey,sdslen(bestkey));
-            latencyStartMonitor(eviction_latency);
+            deleteEvictedKeyAndPropagate(db, keyobj, &key_mem_freed);
-            dbGenericDelete(db,keyobj,server.lazyfree_lazy_eviction,DB_FLAG_KEY_EVICTED);
-            latencyEndMonitor(eviction_latency);
-            latencyAddSampleIfNeeded("eviction-del",eviction_latency);
-            delta -= (long long) zmalloc_used_memory();
-            mem_freed += delta;
-            server.stat_evictedkeys++;
-            signalModifiedKey(NULL,db,keyobj);
-            notifyKeyspaceEvent(NOTIFY_EVICTED, "evicted",
-                keyobj, db->id);
-            propagateDeletion(db,keyobj,server.lazyfree_lazy_eviction);
-            exitExecutionUnit();
-            postExecutionUnitOperations();
             decrRefCount(keyobj);
+            exitExecutionUnit();
+            /* Propagate the DEL command */
+            postExecutionUnitOperations();
+
+            mem_freed += key_mem_freed;
             keys_freed++;
 
             if (keys_freed % 16 == 0) {

src/expire.c

@@ -36,17 +36,18 @@ static double avg_ttl_factor[16] = {0.98, 0.9604, 0.941192, 0.922368, 0.903921,
  * to the function to avoid too many gettimeofday() syscalls. */
 int activeExpireCycleTryExpire(redisDb *db, dictEntry *de, long long now) {
     long long t = dictGetSignedIntegerVal(de);
-    if (now > t) {
+    if (now < t)
-        enterExecutionUnit(1, 0);
-        sds key = dictGetKey(de);
-        robj *keyobj = createStringObject(key,sdslen(key));
-        deleteExpiredKeyAndPropagate(db,keyobj);
-        decrRefCount(keyobj);
-        exitExecutionUnit();
-        return 1;
-    } else {
         return 0;
-    }
+
+    enterExecutionUnit(1, 0);
+    sds key = dictGetKey(de);
+    robj *keyobj = createStringObject(key,sdslen(key));
+    deleteExpiredKeyAndPropagate(db,keyobj);
+    decrRefCount(keyobj);
+    exitExecutionUnit();
+    /* Propagate the DEL command */
+    postExecutionUnitOperations();
+    return 1;
 }
 
 /* Try to expire a few timed out keys. The algorithm used is adaptive and
@@ -113,8 +114,6 @@ void expireScanCallback(void *privdata, const dictEntry *const_de) {
     long long ttl  = dictGetSignedIntegerVal(de) - data->now;
     if (activeExpireCycleTryExpire(data->db, de, data->now)) {
         data->expired++;
-        /* Propagate the DEL command */
-        postExecutionUnitOperations();
     }
     if (ttl > 0) {
         /* We want the average TTL of keys yet not expired. */
@@ -465,20 +464,13 @@ void expireSlaveKeys(void) {
             if ((dbids & 1) != 0) {
                 redisDb *db = server.db+dbid;
                 dictEntry *expire = dbFindExpires(db, keyname);
-
+                int expired = expire && activeExpireCycleTryExpire(server.db+dbid,expire,start);
-                if (expire &&
-                    activeExpireCycleTryExpire(server.db+dbid,expire,start))
-                {
-                    /* Propagate the DEL (writable replicas do not propagate anything to other replicas,
-                     * but they might propagate to AOF) and trigger module hooks. */
-                    postExecutionUnitOperations();
-                }
 
                 /* If the key was not expired in this DB, we need to set the
                  * corresponding bit in the new bitmap we set as value.
                  * At the end of the loop if the bitmap is zero, it means we
                  * no longer need to keep track of this key. */
-                else if (expire) {
+                if (expire && !expired) {
                     noexpire++;
                     new_dbids |= (uint64_t)1 << dbid;
                 }

src/module.c

@@ -8849,9 +8849,9 @@ void moduleNotifyKeyspaceEvent(int type, const char *event, robj *key, int dbid)
              * it will not be notified about it. */
             int prev_active = sub->active;
             sub->active = 1;
-            server.lazy_expire_disabled++;
+            server.allow_access_expired++;
             sub->notify_callback(&ctx, type, event, key);
-            server.lazy_expire_disabled--;
+            server.allow_access_expired--;
             sub->active = prev_active;
             moduleFreeContext(&ctx);
         }
@@ -11890,7 +11890,7 @@ void processModuleLoadingProgressEvent(int is_aof) {
 /* When a key is deleted (in dbAsyncDelete/dbSyncDelete/setKey), it
 *  will be called to tell the module which key is about to be released. */
 void moduleNotifyKeyUnlink(robj *key, robj *val, int dbid, int flags) {
-    server.lazy_expire_disabled++;
+    server.allow_access_expired++;
     int subevent = REDISMODULE_SUBEVENT_KEY_DELETED;
     if (flags & DB_FLAG_KEY_EXPIRED) {
         subevent = REDISMODULE_SUBEVENT_KEY_EXPIRED;
@@ -11913,7 +11913,7 @@ void moduleNotifyKeyUnlink(robj *key, robj *val, int dbid, int flags) {
             mt->unlink(key,mv->value);
         }
     }
-    server.lazy_expire_disabled--;
+    server.allow_access_expired--;
 }
 
 /* Return the free_effort of the module, it will automatically choose to call 

src/server.c

@@ -2074,7 +2074,7 @@ void initServerConfig(void) {
         server.bindaddr[j] = zstrdup(default_bindaddr[j]);
     memset(server.listeners, 0x00, sizeof(server.listeners));
     server.active_expire_enabled = 1;
-    server.lazy_expire_disabled = 0;
+    server.allow_access_expired = 0;
     server.skip_checksum_validation = 0;
     server.loading = 0;
     server.async_loading = 0;

src/server.h

@@ -1744,7 +1744,7 @@ struct redisServer {
     int tcpkeepalive;               /* Set SO_KEEPALIVE if non-zero. */
     int active_expire_enabled;      /* Can be disabled for testing purposes. */
     int active_expire_effort;       /* From 1 (default) to 10, active effort. */
-    int lazy_expire_disabled;       /* If > 0, don't trigger lazy expire */
+    int allow_access_expired;       /* If > 0, allow access to logically expired keys */
     int active_defrag_enabled;
     int sanitize_dump_payload;      /* Enables deep sanitization for ziplist and listpack in RDB and RESTORE. */
     int skip_checksum_validation;   /* Disable checksum validation for RDB and RESTORE payload. */
@@ -3365,6 +3365,7 @@ int setModuleNumericConfig(ModuleConfig *config, long long val, const char **err
 /* db.c -- Keyspace access API */
 int removeExpire(redisDb *db, robj *key);
 void deleteExpiredKeyAndPropagate(redisDb *db, robj *keyobj);
+void deleteEvictedKeyAndPropagate(redisDb *db, robj *keyobj, long long *key_mem_freed);
 void propagateDeletion(redisDb *db, robj *key, int lazy);
 int keyIsExpired(redisDb *db, robj *key);
 long long getExpire(redisDb *db, robj *key);

src/t_hash.c

@@ -747,7 +747,7 @@ GetFieldRes hashTypeGetValue(redisDb *db, robj *o, sds field, unsigned char **vs
     }
 
     if ((server.loading) ||
-        (server.lazy_expire_disabled) ||
+        (server.allow_access_expired) ||
         (hfeFlags & HFE_LAZY_AVOID_FIELD_DEL) ||
         (isPausedActionsWithUpdate(PAUSE_ACTION_EXPIRE)))
         return GETF_EXPIRED;
@@ -1929,7 +1929,7 @@ static int hashTypeExpireIfNeeded(redisDb *db, robj *o) {
 
     /* Follow expireIfNeeded() conditions of when not lazy-expire */
     if ( (server.loading) ||
-         (server.lazy_expire_disabled) ||
+         (server.allow_access_expired) ||
          (server.masterhost) ||  /* master-client or user-client, don't delete */
          (isPausedActionsWithUpdate(PAUSE_ACTION_EXPIRE)))
         return 0;