address Jim early PR comments

Signed-off-by: Ran Shidlansik <ranshid@amazon.com>

Author: ranshid

src/db.c

@@ -44,10 +44,6 @@
  * C-level DB API
  *----------------------------------------------------------------------------*/
 
-/* Flags for expireIfNeeded */
-#define EXPIRE_FORCE_DELETE_EXPIRED 1
-#define EXPIRE_AVOID_DELETE_EXPIRED 2
-
 static keyStatus expireIfNeededWithDictIndex(serverDb *db, robj *key, robj *val, int flags, int dict_index);
 static keyStatus expireIfNeeded(serverDb *db, robj *key, robj *val, int flags);
 static int keyIsExpiredWithDictIndex(serverDb *db, robj *key, int dict_index);
@@ -1921,69 +1917,6 @@ void propagateDeletion(serverDb *db, robj *key, int lazy) {
     server.replication_allowed = prev_replication_allowed;
 }
 
-/* Returns 1 if the expire value is expired, 0 otherwise. */
-int timestampIsExpired(mstime_t when) {
-    if (when < 0) return 0; /* no expire */
-    mstime_t now = commandTimeSnapshot();
-
-    /* The key expired if the current (virtual or real) time is greater
-     * than the expire time of the key. */
-    return now > when;
-}
-
-/* This function verify if the current conditions allow expiration of keys and fields.
- * For some cases expiration is not allowed, but we would still like to ignore the key
- * so to treat it as "expired" without actively deleting it. */
-expirationPolicy getExpirationPolicyWithFlags(int flags) {
-    if (server.loading) return POLICY_IGNORE_EXPIRE;
-
-    /* If we are running in the context of a replica, instead of
-     * evicting the expired key from the database, we return ASAP:
-     * the replica key expiration is controlled by the primary that will
-     * send us synthesized DEL operations for expired keys. The
-     * exception is when write operations are performed on writable
-     * replicas.
-     *
-     * Still we try to reflect the correct state to the caller,
-     * that is, POLICY_KEEP_EXPIRED so that the key will be ignored, but not deleted.
-     *
-     * When replicating commands from the primary, keys are never considered
-     * expired, so we return POLICY_IGNORE_EXPIRE */
-    if (server.primary_host != NULL) {
-        if (server.current_client && (server.current_client->flag.primary)) return POLICY_IGNORE_EXPIRE;
-        if (!(flags & EXPIRE_FORCE_DELETE_EXPIRED)) return POLICY_KEEP_EXPIRED;
-    } else if (server.import_mode) {
-        /* If we are running in the import mode on a primary, instead of
-         * evicting the expired key from the database, we return ASAP:
-         * the key expiration is controlled by the import source that will
-         * send us synthesized DEL operations for expired keys. The
-         * exception is when write operations are performed on this server
-         * because it's a primary.
-         *
-         * Notice: other clients, apart from the import source, should not access
-         * the data imported by import source.
-         *
-         * Still we try to reflect the correct state to the caller,
-         * that is, POLICY_KEEP_EXPIRED so that the key will be ignored, but not deleted.
-         *
-         * When receiving commands from the import source, keys are never considered
-         * expired, so we return POLICY_IGNORE_EXPIRE */
-        if (server.current_client && (server.current_client->flag.import_source)) return POLICY_IGNORE_EXPIRE;
-        if (!(flags & EXPIRE_FORCE_DELETE_EXPIRED)) return POLICY_KEEP_EXPIRED;
-    }
-
-    /* In some cases we're explicitly instructed to return an indication of a
-     * missing key without actually deleting it, even on primaries. */
-    if (flags & EXPIRE_AVOID_DELETE_EXPIRED) return POLICY_KEEP_EXPIRED;
-
-    /* If 'expire' action is paused, for whatever reason, then don't expire any key.
-     * Typically, at the end of the pause we will properly expire the key OR we
-     * will have failed over and the new primary will send us the expire. */
-    if (isPausedActionsWithUpdate(PAUSE_ACTION_EXPIRE)) return POLICY_KEEP_EXPIRED;
-
-    return POLICY_DELETE_EXPIRED;
-}
-
 /* Use this instead of keyIsExpired if you already have the value object. */
 static int objectIsExpired(robj *val) {
     /* Don't expire anything while loading. It will be done later. */

src/entry.c

@@ -50,7 +50,7 @@
 
 /* Returns true in case the entry's value is not embedded in the entry.
  * Returns false otherwise. */
-bool entryHasValuePtr(const entry *entry) {
+static inline bool entryHasValuePtr(const entry *entry) {
     return sdsGetAuxBit(entry, FIELD_SDS_AUX_BIT_ENTRY_HAS_VALUE_PTR);
 }
 
@@ -81,8 +81,6 @@ sds entryGetValue(const entry *entry) {
     } else {
         /* Skip field content, field null terminator and value sds8 hdr. */
         size_t offset = sdslen(entry) + 1 + sdsHdrSize(SDS_TYPE_8);
-        serverAssert((char *)entry + offset);
-
         return (char *)entry + offset;
     }
 }
@@ -117,8 +115,9 @@ long long entryGetExpiry(const entry *entry) {
     long long expiry = EXPIRY_NONE;
     if (entryHasExpiry(entry)) {
         char *buf = sdsAllocPtr(entry);
+        debugServerAssert((((uintptr_t)buf & 0x7) == 0));
         if (entryHasValuePtr(entry)) buf -= sizeof(sds *);
-        buf -= sizeof(expiry);
+        buf -= sizeof(long long);
         expiry = *(long long *)buf;
     }
     return expiry;
@@ -138,26 +137,21 @@ entry *entrySetExpiry(entry *e, long long expiry) {
 }
 
 /* Return true in case the entry has assigned expiration or false otherwise. */
-int entryIsExpired(entry *entry) {
-    /* Don't expire anything while loading. It will be done later. */
-    if (server.loading) return 0;
-    if (!timestampIsExpired(entryGetExpiry(entry))) return 0;
-    if (server.primary_host == NULL && server.import_mode) {
-        if (server.current_client && server.current_client->flag.import_source) return 0;
-    }
-    return 1;
+bool entryIsExpired(entry *entry) {
+    if (!timestampIsExpired(entryGetExpiry(entry))) return false;
+    return true;
 }
 /**************************************** Entry Expiry API - End *****************************************/
 
 void entryFree(entry *entry) {
     if (entryHasValuePtr(entry)) {
-        sdsfree(*entryGetValueRef(entry));
+        sdsfree(entryGetValue(entry));
     }
     zfree(entryAllocPtr(entry));
 }
 
 /* Takes ownership of value. does not take ownership of field */
-entry *entryCreate(sds field, sds value, long long expiry) {
+entry *entryCreate(const_sds field, sds value, long long expiry) {
     sds embedded_field_sds;
     size_t expiry_size = (expiry == EXPIRY_NONE) ? 0 : sizeof(long long);
     size_t field_len = sdslen(field);
@@ -192,7 +186,7 @@ entry *entryCreate(sds field, sds value, long long expiry) {
              *     +------+-------+---------------+
              */
             embed_value = false;
-            alloc_size += sizeof(sds *);
+            alloc_size += sizeof(sds);
             if (field_sds_type == SDS_TYPE_5) {
                 field_sds_type = SDS_TYPE_8;
                 alloc_size -= field_size;
@@ -214,8 +208,8 @@ entry *entryCreate(sds field, sds value, long long expiry) {
     if (value) {
         if (!embed_value) {
             *(sds *)buf = value;
-            buf += sizeof(sds *);
-            buf_size -= sizeof(sds *);
+            buf += sizeof(sds);
+            buf_size -= sizeof(sds);
         } else {
             sdswrite(buf + field_size, buf_size - field_size, SDS_TYPE_8, value, value_len);
             sdsfree(value);
@@ -237,13 +231,13 @@ entry *entryUpdate(entry *e, sds value, long long expiry) {
     sds field = (sds)e;
 
     bool update_value = value ? true : false;
-    long long ttl = entryGetExpiry(e);
-    bool update_expiry = (expiry != ttl) ? true : false;
+    long long expiration_time = entryGetExpiry(e);
+    bool update_expiry = (expiry != expiration_time) ? true : false;
     if (!update_value && !update_expiry)
         return e;
-    ttl = expiry;
+    expiration_time = expiry;
     value = update_value ? value : entryGetValue(e);
-    size_t expiry_size = ttl != EXPIRY_NONE ? sizeof(ttl) : 0;
+    size_t expiry_size = (expiration_time != EXPIRY_NONE) ? sizeof(expiration_time) : 0;
     int field_sds_type = sdsReqType(sdslen(field));
     if (field_sds_type == SDS_TYPE_5 && (expiry_size > 0)) {
         field_sds_type = SDS_TYPE_8;
@@ -257,7 +251,7 @@ entry *entryUpdate(entry *e, sds value, long long expiry) {
     /* // We will create a new entry in the following cases:
      * 1. In the case were we add or remove expiration.
      * 2. in the case were we are NOT migrating from an embedded entry to an embedded entry with ~the same size. */
-    bool create_new_entry = (update_expiry && (entryGetExpiry(e) == EXPIRY_NONE || ttl == EXPIRY_NONE)) ||
+    bool create_new_entry = (update_expiry && (entryGetExpiry(e) == EXPIRY_NONE || expiration_time == EXPIRY_NONE)) ||
                             !(update_value && !entryHasValuePtr(e) &&
                               required_embedded_size <= EMBED_VALUE_MAX_ALLOC_SIZE &&
                               required_embedded_size <= current_embedded_allocation_size &&
@@ -307,7 +301,7 @@ entry *entryUpdate(entry *e, sds value, long long expiry) {
         }
     }
 
-    entry *new_entry = entryCreate(entryGetField(e), value, ttl);
+    entry *new_entry = entryCreate(entryGetField(e), value, expiration_time);
     if (new_entry != e)
         entryFree(e);
     return new_entry;
@@ -355,7 +349,7 @@ entry *entryDefrag(entry *entry, void *(*defragfn)(void *), sds (*sdsdefragfn)(s
 
 /* Used for releasing memory to OS to avoid unnecessary CoW. Called when we've
  * forked and memory won't be used again. See zmadvise_dontneed() */
-void dismissEntry(entry *entry) {
+void entryDismissMemory(entry *entry) {
     /* Only dismiss values memory since the field size usually is small. */
     if (entryHasValuePtr(entry)) {
         dismissSds(*entryGetValueRef(entry));

src/entry.h

@@ -12,13 +12,13 @@ entry *entrySetValue(entry *entry, sds value);
 long long entryGetExpiry(const entry *entry);
 bool entryHasExpiry(const entry *entry);
 entry *entrySetExpiry(entry *entry, long long expiry);
-int entryIsExpired(entry *entry);
+bool entryIsExpired(entry *entry);
 
 void entryFree(entry *entry);
-entry *entryCreate(sds field, sds value, long long expiry);
+entry *entryCreate(const_sds field, sds value, long long expiry);
 entry *entryUpdate(entry *entry, sds value, long long expiry);
 size_t entryMemUsage(entry *entry);
 entry *entryDefrag(entry *entry, void *(*defragfn)(void *), sds (*sdsdefragfn)(sds));
-void dismissEntry(entry *entry);
+void entryDismissMemory(entry *entry);
 
 #endif

src/expire.c

@@ -877,3 +877,66 @@ void touchCommand(client *c) {
         if (lookupKeyRead(c->db, c->argv[j]) != NULL) touched++;
     addReplyLongLong(c, touched);
 }
+
+/* Returns 1 if the expire value is expired, 0 otherwise. */
+int timestampIsExpired(mstime_t when) {
+    if (when < 0) return 0; /* no expire */
+    mstime_t now = commandTimeSnapshot();
+
+    /* The time indicated by 'when' is considered expired if the current (virtual or real) time is greater
+     * than it. */
+    return now > when;
+}
+
+/* This function verify if the current conditions allow expiration of keys and fields.
+ * For some cases expiration is not allowed, but we would still like to ignore the key
+ * so to treat it as "expired" without actively deleting it. */
+expirationPolicy getExpirationPolicyWithFlags(int flags) {
+    if (server.loading) return POLICY_IGNORE_EXPIRE;
+
+    /* If we are running in the context of a replica, instead of
+     * evicting the expired key from the database, we return ASAP:
+     * the replica key expiration is controlled by the primary that will
+     * send us synthesized DEL operations for expired keys. The
+     * exception is when write operations are performed on writable
+     * replicas.
+     *
+     * Still we try to reflect the correct state to the caller,
+     * that is, POLICY_KEEP_EXPIRED so that the key will be ignored, but not deleted.
+     *
+     * When replicating commands from the primary, keys are never considered
+     * expired, so we return POLICY_IGNORE_EXPIRE */
+    if (server.primary_host != NULL) {
+        if (server.current_client && (server.current_client->flag.primary)) return POLICY_IGNORE_EXPIRE;
+        if (!(flags & EXPIRE_FORCE_DELETE_EXPIRED)) return POLICY_KEEP_EXPIRED;
+    } else if (server.import_mode) {
+        /* If we are running in the import mode on a primary, instead of
+         * evicting the expired key from the database, we return ASAP:
+         * the key expiration is controlled by the import source that will
+         * send us synthesized DEL operations for expired keys. The
+         * exception is when write operations are performed on this server
+         * because it's a primary.
+         *
+         * Notice: other clients, apart from the import source, should not access
+         * the data imported by import source.
+         *
+         * Still we try to reflect the correct state to the caller,
+         * that is, POLICY_KEEP_EXPIRED so that the key will be ignored, but not deleted.
+         *
+         * When receiving commands from the import source, keys are never considered
+         * expired, so we return POLICY_IGNORE_EXPIRE */
+        if (server.current_client && (server.current_client->flag.import_source)) return POLICY_IGNORE_EXPIRE;
+        if (!(flags & EXPIRE_FORCE_DELETE_EXPIRED)) return POLICY_KEEP_EXPIRED;
+    }
+
+    /* In some cases we're explicitly instructed to return an indication of a
+     * missing key without actually deleting it, even on primaries. */
+    if (flags & EXPIRE_AVOID_DELETE_EXPIRED) return POLICY_KEEP_EXPIRED;
+
+    /* If 'expire' action is paused, for whatever reason, then don't expire any key.
+     * Typically, at the end of the pause we will properly expire the key OR we
+     * will have failed over and the new primary will send us the expire. */
+    if (isPausedActionsWithUpdate(PAUSE_ACTION_EXPIRE)) return POLICY_KEEP_EXPIRED;
+
+    return POLICY_DELETE_EXPIRED;
+}

src/expire.h

@@ -4,6 +4,10 @@
 #include <time.h>
 #include "monotonic.h"
 
+/* Flags for expireIfNeeded */
+#define EXPIRE_FORCE_DELETE_EXPIRED 1
+#define EXPIRE_AVOID_DELETE_EXPIRED 2
+
 #define ACTIVE_EXPIRE_CYCLE_SLOW 0
 #define ACTIVE_EXPIRE_CYCLE_FAST 1
 

src/object.c

@@ -683,7 +683,7 @@ void dismissHashObject(robj *o, size_t size_hint) {
             hashtableInitIterator(&iter, ht, 0);
             void *next;
             while (hashtableNext(&iter, &next)) {
-                dismissEntry(next);
+                entryDismissMemory(next);
             }
             hashtableResetIterator(&iter);
         }