From 26108322a34d4781d46fb1e7a63b81af1a20f541 Mon Sep 17 00:00:00 2001
From: Harkrishn Patro <harkrisp@amazon.com>
Date: Tue, 16 May 2023 16:17:05 -0700
Subject: [PATCH 01/21] Embed key into dict entry

Signed-off-by: Harkrishn Patro <harkrisp@amazon.com>
---
 src/db.c      |   1 -
 src/defrag.c  |  58 ++++++++++++++++++----------
 src/dict.c    | 102 +++++++++++++++++++++++++++++++++++---------------
 src/dict.h    |  16 ++++++--
 src/kvstore.c |   6 ---
 src/kvstore.h |   1 -
 src/object.c  |   2 +-
 src/rdb.c     |   3 ++
 src/sds.c     |  11 ------
 src/sds.h     |  11 ++++++
 src/server.c  |  17 ++++++++-
 src/server.h  |   2 +
 12 files changed, 154 insertions(+), 76 deletions(-)

diff --git a/src/db.c b/src/db.c
index fa07deeb4b..cb478503ab 100644
--- a/src/db.c
+++ b/src/db.c
@@ -201,7 +201,6 @@ static void dbAddInternal(serverDb *db, robj *key, robj *val, int update_if_exis
         return;
     }
     serverAssertWithInfo(NULL, key, de != NULL);
-    kvstoreDictSetKey(db->keys, slot, de, sdsdup(key->ptr));
     initObjectLRUOrLFU(val);
     kvstoreDictSetVal(db->keys, slot, de, val);
     signalKeyAsReady(db, key, val->type);
diff --git a/src/defrag.c b/src/defrag.c
index 2de1c061e8..638ead1c94 100644
--- a/src/defrag.c
+++ b/src/defrag.c
@@ -41,6 +41,7 @@
 typedef struct defragCtx {
     void *privdata;
     int slot;
+    void *aux;
 } defragCtx;
 
 typedef struct defragPubSubCtx {
@@ -75,6 +76,36 @@ void *activeDefragAlloc(void *ptr) {
     return newptr;
 }
 
+/* This method captures the expiry db dict entry which refers to data stored in keys db dict entry. */
+void defragEntryStartCbForKeys(void *ctx, void *oldptr) {
+    defragCtx *defragctx = (defragCtx *)ctx;
+    serverDb *db = defragctx->privdata;
+    sds oldsds = (sds)dictGetKey((dictEntry *)oldptr);
+    int slot = defragctx->slot;
+    if (kvstoreDictSize(db->expires, slot)) {
+        dictEntry *expire_de = kvstoreDictFind(db->expires, slot, oldsds);
+        defragctx->aux = expire_de;
+    }
+}
+
+/* This method updates the key of expiry db dict entry. The key might be no longer valid
+ * as it could have been cleaned up during the defrag-realloc of the main dictionary. */
+void defragEntryFinishCbForKeys(void *ctx, void *newptr) {
+    defragCtx *defragctx = (defragCtx *)ctx;
+    dictEntry *expire_de = (dictEntry *)defragctx->aux;
+    /* Item doesn't have TTL associated to it. */
+    if (!expire_de) return;
+    /* No reallocation happened. */
+    if (!newptr) {
+        expire_de = NULL;
+        return;
+    }
+    serverDb *db = defragctx->privdata;
+    sds newsds = (sds)dictGetKey((dictEntry *)newptr);
+    int slot = defragctx->slot;
+    kvstoreDictSetKey(db->expires, slot, expire_de, newsds);
+}
+
 /*Defrag helper for sds strings
  *
  * returns NULL in case the allocation wasn't moved.
@@ -649,26 +680,11 @@ void defragModule(serverDb *db, dictEntry *kde) {
 
 /* for each key we scan in the main dict, this function will attempt to defrag
  * all the various pointers it has. */
-void defragKey(defragCtx *ctx, dictEntry *de) {
-    sds keysds = dictGetKey(de);
-    robj *newob, *ob;
-    unsigned char *newzl;
-    sds newsds;
+void defragItem(defragCtx *ctx, dictEntry *de) {
     serverDb *db = ctx->privdata;
     int slot = ctx->slot;
-    /* Try to defrag the key name. */
-    newsds = activeDefragSds(keysds);
-    if (newsds) {
-        kvstoreDictSetKey(db->keys, slot, de, newsds);
-        if (kvstoreDictSize(db->expires, slot)) {
-            /* We can't search in db->expires for that key after we've released
-             * the pointer it holds, since it won't be able to do the string
-             * compare, but we can find the entry using key hash and pointer. */
-            uint64_t hash = kvstoreGetHash(db->expires, newsds);
-            dictEntry *expire_de = kvstoreDictFindEntryByPtrAndHash(db->expires, slot, keysds, hash);
-            if (expire_de) kvstoreDictSetKey(db->expires, slot, expire_de, newsds);
-        }
-    }
+    robj *newob, *ob;
+    unsigned char *newzl;
 
     /* Try to defrag robj and / or string value. */
     ob = dictGetVal(de);
@@ -724,7 +740,7 @@ void defragKey(defragCtx *ctx, dictEntry *de) {
 /* Defrag scan callback for the main db dictionary. */
 void defragScanCallback(void *privdata, const dictEntry *de) {
     long long hits_before = server.stat_active_defrag_hits;
-    defragKey((defragCtx *)privdata, (dictEntry *)de);
+    defragItem((defragCtx *)privdata, (dictEntry *)de);
     if (server.stat_active_defrag_hits != hits_before)
         server.stat_active_defrag_key_hits++;
     else
@@ -984,7 +1000,9 @@ void activeDefragCycle(void) {
     endtime = start + timelimit;
     latencyStartMonitor(latency);
 
-    dictDefragFunctions defragfns = {.defragAlloc = activeDefragAlloc};
+    dictDefragFunctions defragfns = {.defragAlloc = activeDefragAlloc,
+                                     .defragEntryStartCb = defragEntryStartCbForKeys,
+                                     .defragEntryFinishCb = defragEntryFinishCbForKeys};
     do {
         /* if we're not continuing a scan from the last call or loop, start a new one */
         if (!defrag_stage && !defrag_cursor && (slot < 0)) {
diff --git a/src/dict.c b/src/dict.c
index 119c60ab57..e065b06a21 100644
--- a/src/dict.c
+++ b/src/dict.c
@@ -74,6 +74,17 @@ struct dictEntry {
     struct dictEntry *next; /* Next entry in the same hash bucket. */
 };
 
+typedef struct {
+    union {
+        void *val;
+        uint64_t u64;
+        int64_t s64;
+        double d;
+    } v;
+    struct dictEntry *next; /* Next entry in the same hash bucket. */
+    unsigned char data[];
+} embeddedDictEntry;
+
 typedef struct {
     void *key;
     dictEntry *next;
@@ -124,6 +135,7 @@ uint64_t dictGenCaseHashFunction(const unsigned char *buf, size_t len) {
 #define ENTRY_PTR_MASK 7     /* 111 */
 #define ENTRY_PTR_NORMAL 0   /* 000 */
 #define ENTRY_PTR_NO_VALUE 2 /* 010 */
+#define ENTRY_PTR_EMBEDDED 4 /* 100 */
 
 /* Returns 1 if the entry pointer is a pointer to a key, rather than to an
  * allocated entry. Returns 0 otherwise. */
@@ -143,6 +155,11 @@ static inline int entryIsNoValue(const dictEntry *de) {
     return ((uintptr_t)(void *)de & ENTRY_PTR_MASK) == ENTRY_PTR_NO_VALUE;
 }
 
+
+static inline int entryIsEmbedded(const dictEntry *de) {
+    return ((uintptr_t)(void *)de & ENTRY_PTR_MASK) == ENTRY_PTR_EMBEDDED;
+}
+
 /* Creates an entry without a value field. */
 static inline dictEntry *createEntryNoValue(void *key, dictEntry *next) {
     dictEntryNoValue *entry = zmalloc(sizeof(*entry));
@@ -151,6 +168,15 @@ static inline dictEntry *createEntryNoValue(void *key, dictEntry *next) {
     return (dictEntry *)(void *)((uintptr_t)(void *)entry | ENTRY_PTR_NO_VALUE);
 }
 
+static inline dictEntry *createEmbeddedEntry(void *key, dictEntry *next, dictType *dt) {
+    size_t keylen = dt->keyLen(key);
+    embeddedDictEntry *entry = zmalloc(sizeof(*entry) + keylen + ENTRY_METADATA_BYTES);
+    size_t bytes_written = dt->keyToBytes(entry->data + ENTRY_METADATA_BYTES, key, &entry->data[0]);
+    assert(bytes_written == keylen);
+    entry->next = next;
+    return (dictEntry *)(void *)((uintptr_t)(void *)entry | ENTRY_PTR_EMBEDDED);
+}
+
 static inline dictEntry *encodeMaskedPtr(const void *ptr, unsigned int bits) {
     assert(((uintptr_t)ptr & ENTRY_PTR_MASK) == 0);
     return (dictEntry *)(void *)((uintptr_t)ptr | bits);
@@ -161,15 +187,24 @@ static inline void *decodeMaskedPtr(const dictEntry *de) {
     return (void *)((uintptr_t)(void *)de & ~ENTRY_PTR_MASK);
 }
 
+static inline void *getEmbeddedKey(const dictEntry *de) {
+    embeddedDictEntry *entry = (embeddedDictEntry *)decodeMaskedPtr(de);
+    return &entry->data[ENTRY_METADATA_BYTES + entry->data[0]];
+}
+
 /* Decodes the pointer to an entry without value, when you know it is an entry
  * without value. Hint: Use entryIsNoValue to check. */
 static inline dictEntryNoValue *decodeEntryNoValue(const dictEntry *de) {
     return decodeMaskedPtr(de);
 }
 
+static inline embeddedDictEntry *decodeEmbeddedEntry(const dictEntry *de) {
+    return decodeMaskedPtr(de);
+}
+
 /* Returns 1 if the entry has a value field and 0 otherwise. */
 static inline int entryHasValue(const dictEntry *de) {
-    return entryIsNormal(de);
+    return entryIsNormal(de) || entryIsEmbedded(de);
 }
 
 /* ----------------------------- API implementation ------------------------- */
@@ -509,6 +544,8 @@ dictEntry *dictInsertAtPosition(dict *d, void *key, void *position) {
             /* Allocate an entry without value. */
             entry = createEntryNoValue(key, *bucket);
         }
+    } else if (d->type->embedded_entry) {
+        entry = createEmbeddedEntry(key, *bucket, d->type);
     } else {
         /* Allocate the memory and store the new entry.
          * Insert the element in top, with the assumption that in a database
@@ -656,6 +693,9 @@ void dictFreeUnlinkedEntry(dict *d, dictEntry *he) {
     if (he == NULL) return;
     dictFreeKey(d, he);
     dictFreeVal(d, he);
+    /* Clear the embedded data */
+    if (entryIsEmbedded(he)) zfree(decodeEmbeddedEntry(he)->data);
+    /* Clear the dictEntry */
     if (!entryIsKey(he)) zfree(decodeMaskedPtr(he));
 }
 
@@ -801,7 +841,12 @@ void dictSetKey(dict *d, dictEntry *de, void *key) {
 
 void dictSetVal(dict *d, dictEntry *de, void *val) {
     assert(entryHasValue(de));
-    de->v.val = d->type->valDup ? d->type->valDup(d, val) : val;
+    void *v = d->type->valDup ? d->type->valDup(d, val) : val;
+    if (entryIsEmbedded(de)) {
+        decodeEmbeddedEntry(de)->v.val = v;
+    } else {
+        de->v.val = v;
+    }
 }
 
 void dictSetSignedIntegerVal(dictEntry *de, int64_t val) {
@@ -837,11 +882,15 @@ double dictIncrDoubleVal(dictEntry *de, double val) {
 void *dictGetKey(const dictEntry *de) {
     if (entryIsKey(de)) return (void *)de;
     if (entryIsNoValue(de)) return decodeEntryNoValue(de)->key;
+    if (entryIsEmbedded(de)) return getEmbeddedKey(de);
     return de->key;
 }
 
 void *dictGetVal(const dictEntry *de) {
     assert(entryHasValue(de));
+    if (entryIsEmbedded(de)) {
+        return decodeEmbeddedEntry(de)->v.val;
+    }
     return de->v.val;
 }
 
@@ -871,6 +920,7 @@ double *dictGetDoubleValPtr(dictEntry *de) {
 static dictEntry *dictGetNext(const dictEntry *de) {
     if (entryIsKey(de)) return NULL; /* there's no next */
     if (entryIsNoValue(de)) return decodeEntryNoValue(de)->next;
+    if (entryIsEmbedded(de)) return decodeEmbeddedEntry(de)->next;
     return de->next;
 }
 
@@ -879,14 +929,16 @@ static dictEntry *dictGetNext(const dictEntry *de) {
 static dictEntry **dictGetNextRef(dictEntry *de) {
     if (entryIsKey(de)) return NULL;
     if (entryIsNoValue(de)) return &decodeEntryNoValue(de)->next;
+    if (entryIsEmbedded(de)) return &decodeEmbeddedEntry(de)->next;
     return &de->next;
 }
 
 static void dictSetNext(dictEntry *de, dictEntry *next) {
     assert(!entryIsKey(de));
     if (entryIsNoValue(de)) {
-        dictEntryNoValue *entry = decodeEntryNoValue(de);
-        entry->next = next;
+        decodeEntryNoValue(de)->next = next;
+    } else if (entryIsEmbedded(de)) {
+        decodeEmbeddedEntry(de)->next = next;
     } else {
         de->next = next;
     }
@@ -1164,7 +1216,7 @@ unsigned int dictGetSomeKeys(dict *d, dictEntry **des, unsigned int count) {
 
 /* Reallocate the dictEntry, key and value allocations in a bucket using the
  * provided allocation functions in order to defrag them. */
-static void dictDefragBucket(dictEntry **bucketref, dictDefragFunctions *defragfns) {
+static void dictDefragBucket(dictEntry **bucketref, dictDefragFunctions *defragfns, void *privdata) {
     dictDefragAllocFunction *defragalloc = defragfns->defragAlloc;
     dictDefragAllocFunction *defragkey = defragfns->defragKey;
     dictDefragAllocFunction *defragval = defragfns->defragVal;
@@ -1182,6 +1234,17 @@ static void dictDefragBucket(dictEntry **bucketref, dictDefragFunctions *defragf
                 entry = newentry;
             }
             if (newkey) entry->key = newkey;
+        } else if (entryIsEmbedded(de)) {
+            defragfns->defragEntryStartCb(privdata, de);
+            embeddedDictEntry *entry = decodeEmbeddedEntry(de), *newentry;
+            if ((newentry = defragalloc(entry))) {
+                newde = encodeMaskedPtr(newentry, ENTRY_PTR_EMBEDDED);
+                entry = newentry;
+                defragfns->defragEntryFinishCb(privdata, newde);
+            } else {
+                defragfns->defragEntryFinishCb(privdata, NULL);
+            }
+            if (newval) entry->v.val = newval;
         } else {
             assert(entryIsNormal(de));
             newde = defragalloc(de);
@@ -1345,7 +1408,7 @@ dictScanDefrag(dict *d, unsigned long v, dictScanFunction *fn, dictDefragFunctio
 
         /* Emit entries at cursor */
         if (defragfns) {
-            dictDefragBucket(&d->ht_table[htidx0][v & m0], defragfns);
+            dictDefragBucket(&d->ht_table[htidx0][v & m0], defragfns, privdata);
         }
         de = d->ht_table[htidx0][v & m0];
         while (de) {
@@ -1378,7 +1441,7 @@ dictScanDefrag(dict *d, unsigned long v, dictScanFunction *fn, dictDefragFunctio
 
         /* Emit entries at cursor */
         if (defragfns) {
-            dictDefragBucket(&d->ht_table[htidx0][v & m0], defragfns);
+            dictDefragBucket(&d->ht_table[htidx0][v & m0], defragfns, privdata);
         }
         de = d->ht_table[htidx0][v & m0];
         while (de) {
@@ -1392,7 +1455,7 @@ dictScanDefrag(dict *d, unsigned long v, dictScanFunction *fn, dictDefragFunctio
         do {
             /* Emit entries at cursor */
             if (defragfns) {
-                dictDefragBucket(&d->ht_table[htidx1][v & m1], defragfns);
+                dictDefragBucket(&d->ht_table[htidx1][v & m1], defragfns, privdata);
             }
             de = d->ht_table[htidx1][v & m1];
             while (de) {
@@ -1565,29 +1628,6 @@ uint64_t dictGetHash(dict *d, const void *key) {
     return dictHashKey(d, key);
 }
 
-/* Finds the dictEntry using pointer and pre-calculated hash.
- * oldkey is a dead pointer and should not be accessed.
- * the hash value should be provided using dictGetHash.
- * no string / key comparison is performed.
- * return value is a pointer to the dictEntry if found, or NULL if not found. */
-dictEntry *dictFindEntryByPtrAndHash(dict *d, const void *oldptr, uint64_t hash) {
-    dictEntry *he;
-    unsigned long idx, table;
-
-    if (dictSize(d) == 0) return NULL; /* dict is empty */
-    for (table = 0; table <= 1; table++) {
-        idx = hash & DICTHT_SIZE_MASK(d->ht_size_exp[table]);
-        if (table == 0 && (long)idx < d->rehashidx) continue;
-        he = d->ht_table[table][idx];
-        while (he) {
-            if (oldptr == dictGetKey(he)) return he;
-            he = dictGetNext(he);
-        }
-        if (!dictIsRehashing(d)) return NULL;
-    }
-    return NULL;
-}
-
 /* Provides the old and new ht size for a given dictionary during rehashing. This method
  * should only be invoked during initialization/rehashing. */
 void dictRehashingInfo(dict *d, unsigned long long *from_size, unsigned long long *to_size) {
diff --git a/src/dict.h b/src/dict.h
index 7ba22edf1e..7b734b2fa6 100644
--- a/src/dict.h
+++ b/src/dict.h
@@ -67,6 +67,9 @@ typedef struct dictType {
     /* Allow a dict to carry extra caller-defined metadata. The
      * extra memory is initialized to 0 when a dict is allocated. */
     size_t (*dictMetadataBytes)(dict *d);
+    size_t (*keyLen)(const void *key);
+    size_t (*keyToBytes)(unsigned char *buf, const void *key, unsigned char *header_size);
+
 
     /* Data */
     void *userdata;
@@ -83,6 +86,7 @@ typedef struct dictType {
     unsigned int keys_are_odd : 1;
     /* TODO: Add a 'keys_are_even' flag and use a similar optimization if that
      * flag is set. */
+    unsigned int embedded_entry : 1;
 } dictType;
 
 #define DICTHT_SIZE(exp) ((exp) == -1 ? 0 : (unsigned long)1 << (exp))
@@ -128,12 +132,17 @@ typedef struct dictStats {
 
 typedef void(dictScanFunction)(void *privdata, const dictEntry *de);
 typedef void *(dictDefragAllocFunction)(void *ptr);
+typedef void(dictDefragEntryCb)(void *privdata, void *ptr);
 typedef struct {
-    dictDefragAllocFunction *defragAlloc; /* Used for entries etc. */
-    dictDefragAllocFunction *defragKey;   /* Defrag-realloc keys (optional) */
-    dictDefragAllocFunction *defragVal;   /* Defrag-realloc values (optional) */
+    dictDefragAllocFunction *defragAlloc;   /* Used for entries etc. */
+    dictDefragAllocFunction *defragKey;     /* Defrag-realloc keys (optional) */
+    dictDefragAllocFunction *defragVal;     /* Defrag-realloc values (optional) */
+    dictDefragEntryCb *defragEntryStartCb;  /* Callback invoked prior to the start of defrag of dictEntry. */
+    dictDefragEntryCb *defragEntryFinishCb; /* Callback invoked after the defrag of dictEntry is tried. */
 } dictDefragFunctions;
 
+static const int ENTRY_METADATA_BYTES = 1;
+
 /* This is the initial size of every hash table */
 #define DICT_HT_INITIAL_EXP 2
 #define DICT_HT_INITIAL_SIZE (1 << (DICT_HT_INITIAL_EXP))
@@ -237,7 +246,6 @@ unsigned long dictScan(dict *d, unsigned long v, dictScanFunction *fn, void *pri
 unsigned long
 dictScanDefrag(dict *d, unsigned long v, dictScanFunction *fn, dictDefragFunctions *defragfns, void *privdata);
 uint64_t dictGetHash(dict *d, const void *key);
-dictEntry *dictFindEntryByPtrAndHash(dict *d, const void *oldptr, uint64_t hash);
 void dictRehashingInfo(dict *d, unsigned long long *from_size, unsigned long long *to_size);
 
 size_t dictGetStatsMsg(char *buf, size_t bufsize, dictStats *stats, int full);
diff --git a/src/kvstore.c b/src/kvstore.c
index 70e2043157..a2bcb5d4f4 100644
--- a/src/kvstore.c
+++ b/src/kvstore.c
@@ -722,12 +722,6 @@ dictEntry *kvstoreDictGetFairRandomKey(kvstore *kvs, int didx) {
     return dictGetFairRandomKey(d);
 }
 
-dictEntry *kvstoreDictFindEntryByPtrAndHash(kvstore *kvs, int didx, const void *oldptr, uint64_t hash) {
-    dict *d = kvstoreGetDict(kvs, didx);
-    if (!d) return NULL;
-    return dictFindEntryByPtrAndHash(d, oldptr, hash);
-}
-
 unsigned int kvstoreDictGetSomeKeys(kvstore *kvs, int didx, dictEntry **des, unsigned int count) {
     dict *d = kvstoreGetDict(kvs, didx);
     if (!d) return 0;
diff --git a/src/kvstore.h b/src/kvstore.h
index d3c5949d1f..31317006b5 100644
--- a/src/kvstore.h
+++ b/src/kvstore.h
@@ -59,7 +59,6 @@ void kvstoreReleaseDictIterator(kvstoreDictIterator *kvs_id);
 dictEntry *kvstoreDictIteratorNext(kvstoreDictIterator *kvs_di);
 dictEntry *kvstoreDictGetRandomKey(kvstore *kvs, int didx);
 dictEntry *kvstoreDictGetFairRandomKey(kvstore *kvs, int didx);
-dictEntry *kvstoreDictFindEntryByPtrAndHash(kvstore *kvs, int didx, const void *oldptr, uint64_t hash);
 unsigned int kvstoreDictGetSomeKeys(kvstore *kvs, int didx, dictEntry **des, unsigned int count);
 int kvstoreDictExpand(kvstore *kvs, int didx, unsigned long size);
 unsigned long kvstoreDictScanDefrag(kvstore *kvs,
diff --git a/src/object.c b/src/object.c
index 1a335edd6d..25eb341240 100644
--- a/src/object.c
+++ b/src/object.c
@@ -1559,7 +1559,7 @@ NULL
             return;
         }
         size_t usage = objectComputeSize(c->argv[2], dictGetVal(de), samples, c->db->id);
-        usage += sdsZmallocSize(dictGetKey(de));
+        usage += sdsAllocSize((sds)dictGetKey(de));
         usage += dictEntryMemUsage();
         addReplyLongLong(c, usage);
     } else if (!strcasecmp(c->argv[1]->ptr, "stats") && c->argc == 2) {
diff --git a/src/rdb.c b/src/rdb.c
index 5e398dee74..b411de1a08 100644
--- a/src/rdb.c
+++ b/src/rdb.c
@@ -3261,6 +3261,9 @@ int rdbLoadRioWithLoadingCtx(rio *rdb, int rdbflags, rdbSaveInfo *rsi, rdbLoadin
 
             /* call key space notification on key loaded for modules only */
             moduleNotifyKeyspaceEvent(NOTIFY_LOADED, "loaded", &keyobj, db->id);
+
+            /* Release key (sds), dictEntry stores a copy of it in embedded data */
+            sdsfree(key);
         }
 
         /* Loading the database more slowly is useful in order to test
diff --git a/src/sds.c b/src/sds.c
index d957ef4bab..7e15f501bb 100644
--- a/src/sds.c
+++ b/src/sds.c
@@ -42,17 +42,6 @@
 
 const char *SDS_NOINIT = "SDS_NOINIT";
 
-static inline int sdsHdrSize(char type) {
-    switch (type & SDS_TYPE_MASK) {
-    case SDS_TYPE_5: return sizeof(struct sdshdr5);
-    case SDS_TYPE_8: return sizeof(struct sdshdr8);
-    case SDS_TYPE_16: return sizeof(struct sdshdr16);
-    case SDS_TYPE_32: return sizeof(struct sdshdr32);
-    case SDS_TYPE_64: return sizeof(struct sdshdr64);
-    }
-    return 0;
-}
-
 static inline char sdsReqType(size_t string_size) {
     if (string_size < 1 << 5) return SDS_TYPE_5;
     if (string_size < 1 << 8) return SDS_TYPE_8;
diff --git a/src/sds.h b/src/sds.h
index 20d598829a..48401ba760 100644
--- a/src/sds.h
+++ b/src/sds.h
@@ -177,6 +177,17 @@ static inline void sdssetalloc(sds s, size_t newlen) {
     }
 }
 
+static inline int sdsHdrSize(char type) {
+    switch (type & SDS_TYPE_MASK) {
+    case SDS_TYPE_5: return sizeof(struct sdshdr5);
+    case SDS_TYPE_8: return sizeof(struct sdshdr8);
+    case SDS_TYPE_16: return sizeof(struct sdshdr16);
+    case SDS_TYPE_32: return sizeof(struct sdshdr32);
+    case SDS_TYPE_64: return sizeof(struct sdshdr64);
+    }
+    return 0;
+}
+
 sds sdsnewlen(const void *init, size_t initlen);
 sds sdstrynewlen(const void *init, size_t initlen);
 sds sdsnew(const char *init);
diff --git a/src/server.c b/src/server.c
index 3e6dc56d6d..32c38cba63 100644
--- a/src/server.c
+++ b/src/server.c
@@ -290,6 +290,18 @@ int dictSdsKeyCompare(dict *d, const void *key1, const void *key2) {
     return memcmp(key1, key2, l1) == 0;
 }
 
+size_t dictSdsKeyLen(const void *key) { 
+    return sdsAllocSize((sds)key);
+}
+
+size_t dictSdsKeyToBytes(unsigned char *buf, const void *key, unsigned char *header_size) {
+    sds keysds = (sds)key;
+    size_t n_bytes = sdsAllocSize(keysds);
+    memcpy(buf, sdsAllocPtr(keysds), n_bytes);
+    *header_size = sdsHdrSize(keysds[-1]);
+    return n_bytes;
+}
+
 /* A case insensitive version used for the command lookup table and other
  * places where case insensitive non binary-safe comparison is needed. */
 int dictSdsKeyCaseCompare(dict *d, const void *key1, const void *key2) {
@@ -474,9 +486,12 @@ dictType dbDictType = {
     NULL,                 /* key dup */
     NULL,                 /* val dup */
     dictSdsKeyCompare,    /* key compare */
-    dictSdsDestructor,    /* key destructor */
+    NULL,                 /* key is embedded in the dictEntry and freed internally */
     dictObjectDestructor, /* val destructor */
     dictResizeAllowed,    /* allow to resize */
+    .keyLen = dictSdsKeyLen,
+    .keyToBytes = dictSdsKeyToBytes,
+    .embedded_entry = 1
 };
 
 /* Db->expires */
diff --git a/src/server.h b/src/server.h
index abf66fbf5a..c2a4e5bf8f 100644
--- a/src/server.h
+++ b/src/server.h
@@ -3541,6 +3541,8 @@ int dictSdsKeyCaseCompare(dict *d, const void *key1, const void *key2);
 void dictSdsDestructor(dict *d, void *val);
 void dictListDestructor(dict *d, void *val);
 void *dictSdsDup(dict *d, const void *key);
+size_t dictSdsKeyToBytes(unsigned char *buf, const void *key, unsigned char *header_size);
+size_t dictSdsKeyLen(const void *key);
 
 /* Git SHA1 */
 char *serverGitSHA1(void);

From 0936730463fb352004c6e0faf815ee09af91e389 Mon Sep 17 00:00:00 2001
From: Harkrishn Patro <harkrisp@amazon.com>
Date: Wed, 26 Jun 2024 23:10:57 +0000
Subject: [PATCH 02/21] Address feedback pass 1

Signed-off-by: Harkrishn Patro <harkrisp@amazon.com>
---
 src/dict.c   | 42 ++++++++++++++++++++----------------------
 src/dict.h   |  9 +++------
 src/object.c |  3 +--
 src/sds.h    |  2 +-
 src/server.c | 26 +++++++++++++++++---------
 src/server.h |  1 -
 6 files changed, 42 insertions(+), 41 deletions(-)

diff --git a/src/dict.c b/src/dict.c
index e065b06a21..a00341d354 100644
--- a/src/dict.c
+++ b/src/dict.c
@@ -74,7 +74,7 @@ struct dictEntry {
     struct dictEntry *next; /* Next entry in the same hash bucket. */
 };
 
-typedef struct {
+typedef struct __attribute__((packed)) {
     union {
         void *val;
         uint64_t u64;
@@ -82,7 +82,8 @@ typedef struct {
         double d;
     } v;
     struct dictEntry *next; /* Next entry in the same hash bucket. */
-    unsigned char data[];
+    uint8_t key_header_size; /* offset into key_buf where the key is located at. */
+    unsigned char key_buf[]; /* buffer to embed the key. */
 } embeddedDictEntry;
 
 typedef struct {
@@ -160,36 +161,35 @@ static inline int entryIsEmbedded(const dictEntry *de) {
     return ((uintptr_t)(void *)de & ENTRY_PTR_MASK) == ENTRY_PTR_EMBEDDED;
 }
 
+static inline dictEntry *encodeMaskedPtr(const void *ptr, unsigned int bits) {
+    assert(((uintptr_t)ptr & ENTRY_PTR_MASK) == 0);
+    return (dictEntry *)(void *)((uintptr_t)ptr | bits);
+}
+
+static inline void *decodeMaskedPtr(const dictEntry *de) {
+    assert(!entryIsKey(de));
+    return (void *)((uintptr_t)(void *)de & ~ENTRY_PTR_MASK);
+}
+
 /* Creates an entry without a value field. */
 static inline dictEntry *createEntryNoValue(void *key, dictEntry *next) {
     dictEntryNoValue *entry = zmalloc(sizeof(*entry));
     entry->key = key;
     entry->next = next;
-    return (dictEntry *)(void *)((uintptr_t)(void *)entry | ENTRY_PTR_NO_VALUE);
+    return encodeMaskedPtr(entry, ENTRY_PTR_NO_VALUE);
 }
 
 static inline dictEntry *createEmbeddedEntry(void *key, dictEntry *next, dictType *dt) {
-    size_t keylen = dt->keyLen(key);
-    embeddedDictEntry *entry = zmalloc(sizeof(*entry) + keylen + ENTRY_METADATA_BYTES);
-    size_t bytes_written = dt->keyToBytes(entry->data + ENTRY_METADATA_BYTES, key, &entry->data[0]);
-    assert(bytes_written == keylen);
+    size_t key_len = dt->embedKey(NULL, 0, key, NULL);
+    embeddedDictEntry *entry = zmalloc(sizeof(*entry) + key_len);
+    dt->embedKey(entry->key_buf, key_len, key, &entry->key_header_size);
     entry->next = next;
-    return (dictEntry *)(void *)((uintptr_t)(void *)entry | ENTRY_PTR_EMBEDDED);
-}
-
-static inline dictEntry *encodeMaskedPtr(const void *ptr, unsigned int bits) {
-    assert(((uintptr_t)ptr & ENTRY_PTR_MASK) == 0);
-    return (dictEntry *)(void *)((uintptr_t)ptr | bits);
-}
-
-static inline void *decodeMaskedPtr(const dictEntry *de) {
-    assert(!entryIsKey(de));
-    return (void *)((uintptr_t)(void *)de & ~ENTRY_PTR_MASK);
+    return encodeMaskedPtr(entry, ENTRY_PTR_EMBEDDED);
 }
 
 static inline void *getEmbeddedKey(const dictEntry *de) {
     embeddedDictEntry *entry = (embeddedDictEntry *)decodeMaskedPtr(de);
-    return &entry->data[ENTRY_METADATA_BYTES + entry->data[0]];
+    return &entry->key_buf[entry->key_header_size];
 }
 
 /* Decodes the pointer to an entry without value, when you know it is an entry
@@ -204,7 +204,7 @@ static inline embeddedDictEntry *decodeEmbeddedEntry(const dictEntry *de) {
 
 /* Returns 1 if the entry has a value field and 0 otherwise. */
 static inline int entryHasValue(const dictEntry *de) {
-    return entryIsNormal(de) || entryIsEmbedded(de);
+    return !entryIsNoValue(de);
 }
 
 /* ----------------------------- API implementation ------------------------- */
@@ -693,8 +693,6 @@ void dictFreeUnlinkedEntry(dict *d, dictEntry *he) {
     if (he == NULL) return;
     dictFreeKey(d, he);
     dictFreeVal(d, he);
-    /* Clear the embedded data */
-    if (entryIsEmbedded(he)) zfree(decodeEmbeddedEntry(he)->data);
     /* Clear the dictEntry */
     if (!entryIsKey(he)) zfree(decodeMaskedPtr(he));
 }
diff --git a/src/dict.h b/src/dict.h
index 7b734b2fa6..6b36e278a7 100644
--- a/src/dict.h
+++ b/src/dict.h
@@ -67,8 +67,7 @@ typedef struct dictType {
     /* Allow a dict to carry extra caller-defined metadata. The
      * extra memory is initialized to 0 when a dict is allocated. */
     size_t (*dictMetadataBytes)(dict *d);
-    size_t (*keyLen)(const void *key);
-    size_t (*keyToBytes)(unsigned char *buf, const void *key, unsigned char *header_size);
+    size_t (*embedKey)(unsigned char *buf, size_t buf_len, const void *key, unsigned char *header_size);
 
 
     /* Data */
@@ -84,8 +83,8 @@ typedef struct dictType {
      * enables one more optimization: to store a key without an allocated
      * dictEntry. */
     unsigned int keys_are_odd : 1;
-    /* TODO: Add a 'keys_are_even' flag and use a similar optimization if that
-     * flag is set. */
+    /* If embedded_entry flag is set, it indicates that the key is embedded as part of
+     * the dict entry. */
     unsigned int embedded_entry : 1;
 } dictType;
 
@@ -141,8 +140,6 @@ typedef struct {
     dictDefragEntryCb *defragEntryFinishCb; /* Callback invoked after the defrag of dictEntry is tried. */
 } dictDefragFunctions;
 
-static const int ENTRY_METADATA_BYTES = 1;
-
 /* This is the initial size of every hash table */
 #define DICT_HT_INITIAL_EXP 2
 #define DICT_HT_INITIAL_SIZE (1 << (DICT_HT_INITIAL_EXP))
diff --git a/src/object.c b/src/object.c
index 25eb341240..74140e86c6 100644
--- a/src/object.c
+++ b/src/object.c
@@ -1559,8 +1559,7 @@ NULL
             return;
         }
         size_t usage = objectComputeSize(c->argv[2], dictGetVal(de), samples, c->db->id);
-        usage += sdsAllocSize((sds)dictGetKey(de));
-        usage += dictEntryMemUsage();
+        usage += zmalloc_size(de);
         addReplyLongLong(c, usage);
     } else if (!strcasecmp(c->argv[1]->ptr, "stats") && c->argc == 2) {
         struct serverMemOverhead *mh = getMemoryOverheadData();
diff --git a/src/sds.h b/src/sds.h
index 48401ba760..c16399caa0 100644
--- a/src/sds.h
+++ b/src/sds.h
@@ -177,7 +177,7 @@ static inline void sdssetalloc(sds s, size_t newlen) {
     }
 }
 
-static inline int sdsHdrSize(char type) {
+static inline uint8_t sdsHdrSize(char type) {
     switch (type & SDS_TYPE_MASK) {
     case SDS_TYPE_5: return sizeof(struct sdshdr5);
     case SDS_TYPE_8: return sizeof(struct sdshdr8);
diff --git a/src/server.c b/src/server.c
index 32c38cba63..1790b8b846 100644
--- a/src/server.c
+++ b/src/server.c
@@ -290,16 +290,25 @@ int dictSdsKeyCompare(dict *d, const void *key1, const void *key2) {
     return memcmp(key1, key2, l1) == 0;
 }
 
-size_t dictSdsKeyLen(const void *key) { 
-    return sdsAllocSize((sds)key);
+/*
+ * This method returns the minimum amount of bytes required to store the sds (header + data + NULL terminator).
+*/
+static inline size_t sdsKeyLen(const void *key) { 
+    sds keysds = ((sds)key);
+    return sdslen(keysds) + sdsHdrSize(keysds[-1]) + 1;
 }
 
-size_t dictSdsKeyToBytes(unsigned char *buf, const void *key, unsigned char *header_size) {
+size_t dictSdsEmbedKey(unsigned char *buf, size_t buf_len, const void *key, uint8_t *key_offset) {
+    size_t keylen = sdsKeyLen(key);
+    if (buf == NULL) {
+        return keylen;
+    }
     sds keysds = (sds)key;
-    size_t n_bytes = sdsAllocSize(keysds);
-    memcpy(buf, sdsAllocPtr(keysds), n_bytes);
-    *header_size = sdsHdrSize(keysds[-1]);
-    return n_bytes;
+    size_t reqd_keylen = sdsKeyLen(key);
+    serverAssert(reqd_keylen <= buf_len);
+    memcpy(buf, sdsAllocPtr(keysds), reqd_keylen);
+    *key_offset = sdsHdrSize(keysds[-1]);
+    return reqd_keylen;
 }
 
 /* A case insensitive version used for the command lookup table and other
@@ -489,8 +498,7 @@ dictType dbDictType = {
     NULL,                 /* key is embedded in the dictEntry and freed internally */
     dictObjectDestructor, /* val destructor */
     dictResizeAllowed,    /* allow to resize */
-    .keyLen = dictSdsKeyLen,
-    .keyToBytes = dictSdsKeyToBytes,
+    .embedKey = dictSdsEmbedKey,
     .embedded_entry = 1
 };
 
diff --git a/src/server.h b/src/server.h
index c2a4e5bf8f..ff833c75e8 100644
--- a/src/server.h
+++ b/src/server.h
@@ -3542,7 +3542,6 @@ void dictSdsDestructor(dict *d, void *val);
 void dictListDestructor(dict *d, void *val);
 void *dictSdsDup(dict *d, const void *key);
 size_t dictSdsKeyToBytes(unsigned char *buf, const void *key, unsigned char *header_size);
-size_t dictSdsKeyLen(const void *key);
 
 /* Git SHA1 */
 char *serverGitSHA1(void);

From f4c54c9b5c1fa0979001f1b2806c1b0a6a771556 Mon Sep 17 00:00:00 2001
From: Harkrishn Patro <harkrisp@amazon.com>
Date: Thu, 27 Jun 2024 17:36:19 +0000
Subject: [PATCH 03/21] Address feedback pass 2

Signed-off-by: Harkrishn Patro <harkrisp@amazon.com>
---
 src/defrag.c |  4 ++--
 src/sds.c    | 30 ++++++++++++++++++++++++++++++
 src/sds.h    | 12 +-----------
 src/server.c | 19 +------------------
 4 files changed, 34 insertions(+), 31 deletions(-)

diff --git a/src/defrag.c b/src/defrag.c
index 638ead1c94..5a54875864 100644
--- a/src/defrag.c
+++ b/src/defrag.c
@@ -680,7 +680,7 @@ void defragModule(serverDb *db, dictEntry *kde) {
 
 /* for each key we scan in the main dict, this function will attempt to defrag
  * all the various pointers it has. */
-void defragItem(defragCtx *ctx, dictEntry *de) {
+void defragKey(defragCtx *ctx, dictEntry *de) {
     serverDb *db = ctx->privdata;
     int slot = ctx->slot;
     robj *newob, *ob;
@@ -740,7 +740,7 @@ void defragItem(defragCtx *ctx, dictEntry *de) {
 /* Defrag scan callback for the main db dictionary. */
 void defragScanCallback(void *privdata, const dictEntry *de) {
     long long hits_before = server.stat_active_defrag_hits;
-    defragItem((defragCtx *)privdata, (dictEntry *)de);
+    defragKey((defragCtx *)privdata, (dictEntry *)de);
     if (server.stat_active_defrag_hits != hits_before)
         server.stat_active_defrag_key_hits++;
     else
diff --git a/src/sds.c b/src/sds.c
index 7e15f501bb..358a8bc4c1 100644
--- a/src/sds.c
+++ b/src/sds.c
@@ -42,6 +42,17 @@
 
 const char *SDS_NOINIT = "SDS_NOINIT";
 
+static inline int sdsHdrSize(char type) {
+    switch (type & SDS_TYPE_MASK) {
+    case SDS_TYPE_5: return sizeof(struct sdshdr5);
+    case SDS_TYPE_8: return sizeof(struct sdshdr8);
+    case SDS_TYPE_16: return sizeof(struct sdshdr16);
+    case SDS_TYPE_32: return sizeof(struct sdshdr32);
+    case SDS_TYPE_64: return sizeof(struct sdshdr64);
+    }
+    return 0;
+}
+
 static inline char sdsReqType(size_t string_size) {
     if (string_size < 1 << 5) return SDS_TYPE_5;
     if (string_size < 1 << 8) return SDS_TYPE_8;
@@ -163,6 +174,25 @@ sds sdsdup(const sds s) {
     return sdsnewlen(s, sdslen(s));
 }
 
+/*
+ * This method returns the minimum amount of bytes required to store the sds (header + data + NULL terminator).
+ */
+static inline size_t sdsminlen(sds s) {
+    return sdslen(s) + sdsHdrSize(s[-1]) + 1;
+}
+
+/* This method copies the sds `s` into `buf` which is the target character buffer. */
+size_t sdscopytobuffer(unsigned char *buf, size_t buf_len, sds s, uint8_t *hdr_size) {
+    size_t reqd_keylen = sdsminlen(s);
+    if (buf == NULL) {
+        return reqd_keylen;
+    }
+    assert(buf_len >= reqd_keylen);
+    memcpy(buf, sdsAllocPtr(s), reqd_keylen);
+    *hdr_size = sdsHdrSize(s[-1]);
+    return reqd_keylen;
+}
+
 /* Free an sds string. No operation is performed if 's' is NULL. */
 void sdsfree(sds s) {
     if (s == NULL) return;
diff --git a/src/sds.h b/src/sds.h
index c16399caa0..a12b8dd89e 100644
--- a/src/sds.h
+++ b/src/sds.h
@@ -177,22 +177,12 @@ static inline void sdssetalloc(sds s, size_t newlen) {
     }
 }
 
-static inline uint8_t sdsHdrSize(char type) {
-    switch (type & SDS_TYPE_MASK) {
-    case SDS_TYPE_5: return sizeof(struct sdshdr5);
-    case SDS_TYPE_8: return sizeof(struct sdshdr8);
-    case SDS_TYPE_16: return sizeof(struct sdshdr16);
-    case SDS_TYPE_32: return sizeof(struct sdshdr32);
-    case SDS_TYPE_64: return sizeof(struct sdshdr64);
-    }
-    return 0;
-}
-
 sds sdsnewlen(const void *init, size_t initlen);
 sds sdstrynewlen(const void *init, size_t initlen);
 sds sdsnew(const char *init);
 sds sdsempty(void);
 sds sdsdup(const sds s);
+size_t sdscopytobuffer(unsigned char *buf, size_t buf_len, sds s, uint8_t *hdr_size);
 void sdsfree(sds s);
 sds sdsgrowzero(sds s, size_t len);
 sds sdscatlen(sds s, const void *t, size_t len);
diff --git a/src/server.c b/src/server.c
index 1790b8b846..e7223d881e 100644
--- a/src/server.c
+++ b/src/server.c
@@ -290,25 +290,8 @@ int dictSdsKeyCompare(dict *d, const void *key1, const void *key2) {
     return memcmp(key1, key2, l1) == 0;
 }
 
-/*
- * This method returns the minimum amount of bytes required to store the sds (header + data + NULL terminator).
-*/
-static inline size_t sdsKeyLen(const void *key) { 
-    sds keysds = ((sds)key);
-    return sdslen(keysds) + sdsHdrSize(keysds[-1]) + 1;
-}
-
 size_t dictSdsEmbedKey(unsigned char *buf, size_t buf_len, const void *key, uint8_t *key_offset) {
-    size_t keylen = sdsKeyLen(key);
-    if (buf == NULL) {
-        return keylen;
-    }
-    sds keysds = (sds)key;
-    size_t reqd_keylen = sdsKeyLen(key);
-    serverAssert(reqd_keylen <= buf_len);
-    memcpy(buf, sdsAllocPtr(keysds), reqd_keylen);
-    *key_offset = sdsHdrSize(keysds[-1]);
-    return reqd_keylen;
+    return sdscopytobuffer(buf, buf_len, (sds)key, key_offset);
 }
 
 /* A case insensitive version used for the command lookup table and other

From a56e2c9d850b3775fc371ba75659c149cac5c7fe Mon Sep 17 00:00:00 2001
From: Harkrishn Patro <harkrisp@amazon.com>
Date: Thu, 27 Jun 2024 18:52:52 +0000
Subject: [PATCH 04/21] Address feedback pass 3

Signed-off-by: Harkrishn Patro <harkrisp@amazon.com>
---
 src/dict.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/dict.c b/src/dict.c
index a00341d354..69ea252550 100644
--- a/src/dict.c
+++ b/src/dict.c
@@ -137,6 +137,7 @@ uint64_t dictGenCaseHashFunction(const unsigned char *buf, size_t len) {
 #define ENTRY_PTR_NORMAL 0   /* 000 */
 #define ENTRY_PTR_NO_VALUE 2 /* 010 */
 #define ENTRY_PTR_EMBEDDED 4 /* 100 */
+/*      ENTRY_PTR_IS_KEY        xx1 */
 
 /* Returns 1 if the entry pointer is a pointer to a key, rather than to an
  * allocated entry. Returns 0 otherwise. */

From 029917f56ae71a822d1f10889544936bc530dad6 Mon Sep 17 00:00:00 2001
From: Harkrishn Patro <harkrisp@amazon.com>
Date: Thu, 27 Jun 2024 21:20:28 +0000
Subject: [PATCH 05/21] Compact allocation for embedded dict entry

Signed-off-by: Harkrishn Patro <harkrisp@amazon.com>
---
 src/dict.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/src/dict.c b/src/dict.c
index 69ea252550..5a75bd78d6 100644
--- a/src/dict.c
+++ b/src/dict.c
@@ -35,6 +35,7 @@
 
 #include "fmacros.h"
 
+#include <stddef.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <stdint.h>
@@ -48,6 +49,10 @@
 #include "serverassert.h"
 #include "monotonic.h"
 
+#ifndef static_assert
+#define static_assert(expr, lit) extern char __static_assert_failure[(expr) ? 1 : -1]
+#endif
+
 /* Using dictSetResizeEnabled() we make possible to disable
  * resizing and rehashing of the hash table as needed. This is very important
  * for us, as we use copy-on-write and don't want to move too much memory
@@ -74,7 +79,7 @@ struct dictEntry {
     struct dictEntry *next; /* Next entry in the same hash bucket. */
 };
 
-typedef struct __attribute__((packed)) {
+typedef struct {
     union {
         void *val;
         uint64_t u64;
@@ -86,11 +91,24 @@ typedef struct __attribute__((packed)) {
     unsigned char key_buf[]; /* buffer to embed the key. */
 } embeddedDictEntry;
 
+static_assert(sizeof(embeddedDictEntry) == 24, "unexpected total size of embeddedDictEntry");
+static_assert(offsetof(embeddedDictEntry, v) == 0, "unexpected field offset");
+static_assert(offsetof(embeddedDictEntry, next) == 8, "unexpected field offset");
+static_assert(offsetof(embeddedDictEntry, key_header_size) == 16, "unexpected field offset");
+static_assert(offsetof(embeddedDictEntry, key_buf) == 17, "unexpected field offset");
+
+/* The minimum amount of bytes required for embedded dict entry. */
+static inline size_t compactSizeEmbeddedDictEntry(void) {
+    return offsetof(embeddedDictEntry, key_buf);
+}
+
 typedef struct {
     void *key;
     dictEntry *next;
 } dictEntryNoValue;
 
+/* Helper and validation for `embeddedDictEntry` */
+
 /* -------------------------- private prototypes ---------------------------- */
 
 static void _dictExpandIfNeeded(dict *d);
@@ -182,7 +200,7 @@ static inline dictEntry *createEntryNoValue(void *key, dictEntry *next) {
 
 static inline dictEntry *createEmbeddedEntry(void *key, dictEntry *next, dictType *dt) {
     size_t key_len = dt->embedKey(NULL, 0, key, NULL);
-    embeddedDictEntry *entry = zmalloc(sizeof(*entry) + key_len);
+    embeddedDictEntry *entry = zmalloc(compactSizeEmbeddedDictEntry() + key_len);
     dt->embedKey(entry->key_buf, key_len, key, &entry->key_header_size);
     entry->next = next;
     return encodeMaskedPtr(entry, ENTRY_PTR_EMBEDDED);

From 3c7d958b18537c300b4c4217f7d1ade9be50e582 Mon Sep 17 00:00:00 2001
From: Harkrishn Patro <harkrisp@amazon.com>
Date: Thu, 27 Jun 2024 21:22:08 +0000
Subject: [PATCH 06/21] Update code comment

Signed-off-by: Harkrishn Patro <harkrisp@amazon.com>
---
 src/dict.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/dict.c b/src/dict.c
index 5a75bd78d6..9679fa7932 100644
--- a/src/dict.c
+++ b/src/dict.c
@@ -91,6 +91,7 @@ typedef struct {
     unsigned char key_buf[]; /* buffer to embed the key. */
 } embeddedDictEntry;
 
+/* Validation and helper for `embeddedDictEntry` */
 static_assert(sizeof(embeddedDictEntry) == 24, "unexpected total size of embeddedDictEntry");
 static_assert(offsetof(embeddedDictEntry, v) == 0, "unexpected field offset");
 static_assert(offsetof(embeddedDictEntry, next) == 8, "unexpected field offset");
@@ -107,8 +108,6 @@ typedef struct {
     dictEntry *next;
 } dictEntryNoValue;
 
-/* Helper and validation for `embeddedDictEntry` */
-
 /* -------------------------- private prototypes ---------------------------- */
 
 static void _dictExpandIfNeeded(dict *d);

From 7801c7e6b0cf69e1b7c983d2e54a405adbdc3a2a Mon Sep 17 00:00:00 2001
From: Harkrishn Patro <harkrisp@amazon.com>
Date: Thu, 27 Jun 2024 22:04:21 +0000
Subject: [PATCH 07/21] Address feedback

Signed-off-by: Harkrishn Patro <harkrisp@amazon.com>
---
 src/dict.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/dict.c b/src/dict.c
index 9679fa7932..a39d49bca5 100644
--- a/src/dict.c
+++ b/src/dict.c
@@ -50,7 +50,7 @@
 #include "monotonic.h"
 
 #ifndef static_assert
-#define static_assert(expr, lit) extern char __static_assert_failure[(expr) ? 1 : -1]
+#define static_assert(expr, lit) _Static_assert(expr, lit)
 #endif
 
 /* Using dictSetResizeEnabled() we make possible to disable

From e714f2cbe61b88afad5588c15a172779eb7b3b7e Mon Sep 17 00:00:00 2001
From: Harkrishn Patro <harkrisp@amazon.com>
Date: Thu, 27 Jun 2024 22:10:36 +0000
Subject: [PATCH 08/21] Add code comment

Signed-off-by: Harkrishn Patro <harkrisp@amazon.com>
---
 src/dict.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/dict.h b/src/dict.h
index 541ade7915..2baba3b1cb 100644
--- a/src/dict.h
+++ b/src/dict.h
@@ -66,6 +66,8 @@ typedef struct dictType {
     /* Allow a dict to carry extra caller-defined metadata. The
      * extra memory is initialized to 0 when a dict is allocated. */
     size_t (*dictMetadataBytes)(dict *d);
+    /* Method for copying a given key into a buffer of buf_len. Also used for
+     * computing the length of the key + header when buf is NULL. */
     size_t (*embedKey)(unsigned char *buf, size_t buf_len, const void *key, unsigned char *header_size);
 
 

From 4d30109c3f8de79869fb80b0f50983f39f801ca4 Mon Sep 17 00:00:00 2001
From: Harkrishn Patro <harkrisp@amazon.com>
Date: Thu, 27 Jun 2024 22:59:32 +0000
Subject: [PATCH 09/21] Accurate dictEntry memory usage calculation

Signed-off-by: Harkrishn Patro <harkrisp@amazon.com>
---
 src/debug.c   |  2 +-
 src/dict.c    | 11 +++++++++--
 src/dict.h    |  2 +-
 src/kvstore.c |  2 +-
 src/object.c  |  8 ++++----
 5 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/src/debug.c b/src/debug.c
index c625ab5150..94890d4620 100644
--- a/src/debug.c
+++ b/src/debug.c
@@ -864,7 +864,7 @@ void debugCommand(client *c) {
         sds sizes = sdsempty();
         sizes = sdscatprintf(sizes, "bits:%d ", (sizeof(void *) == 8) ? 64 : 32);
         sizes = sdscatprintf(sizes, "robj:%d ", (int)sizeof(robj));
-        sizes = sdscatprintf(sizes, "dictentry:%d ", (int)dictEntryMemUsage());
+        sizes = sdscatprintf(sizes, "dictentry:%d ", (int)dictEntryMemUsage(NULL));
         sizes = sdscatprintf(sizes, "sdshdr5:%d ", (int)sizeof(struct sdshdr5));
         sizes = sdscatprintf(sizes, "sdshdr8:%d ", (int)sizeof(struct sdshdr8));
         sizes = sdscatprintf(sizes, "sdshdr16:%d ", (int)sizeof(struct sdshdr16));
diff --git a/src/dict.c b/src/dict.c
index ac6744dbbf..0776929171 100644
--- a/src/dict.c
+++ b/src/dict.c
@@ -968,8 +968,15 @@ size_t dictMemUsage(const dict *d) {
     return dictSize(d) * sizeof(dictEntry) + dictBuckets(d) * sizeof(dictEntry *);
 }
 
-size_t dictEntryMemUsage(void) {
-    return sizeof(dictEntry);
+/* Returns the memory usage in bytes of dictEntry based on the type. if `de` is NULL, return the size of
+ * regular dict entry else return based on the type. */
+size_t dictEntryMemUsage(dictEntry *de) {
+    if (de == NULL || entryIsNormal(de)) return sizeof(dictEntry);
+    else if (entryIsKey(de)) return 0;
+    else if (entryIsNoValue(de)) return sizeof(dictEntryNoValue);
+    else if (entryIsEmbedded(de)) return zmalloc_size(decodeEmbeddedEntry(de));
+    else assert("Entry type not supported");
+    return 0;
 }
 
 /* A fingerprint is a 64 bit number that represents the state of the dictionary
diff --git a/src/dict.h b/src/dict.h
index 2baba3b1cb..6f0eef8371 100644
--- a/src/dict.h
+++ b/src/dict.h
@@ -220,7 +220,7 @@ uint64_t dictGetUnsignedIntegerVal(const dictEntry *de);
 double dictGetDoubleVal(const dictEntry *de);
 double *dictGetDoubleValPtr(dictEntry *de);
 size_t dictMemUsage(const dict *d);
-size_t dictEntryMemUsage(void);
+size_t dictEntryMemUsage(dictEntry *de);
 dictIterator *dictGetIterator(dict *d);
 dictIterator *dictGetSafeIterator(dict *d);
 void dictInitIterator(dictIterator *iter, dict *d);
diff --git a/src/kvstore.c b/src/kvstore.c
index afe3d3d733..00d2d7a58f 100644
--- a/src/kvstore.c
+++ b/src/kvstore.c
@@ -340,7 +340,7 @@ size_t kvstoreMemUsage(kvstore *kvs) {
     size_t mem = sizeof(*kvs);
 
     unsigned long long keys_count = kvstoreSize(kvs);
-    mem += keys_count * dictEntryMemUsage() + kvstoreBuckets(kvs) * sizeof(dictEntry *) +
+    mem += keys_count * dictEntryMemUsage(NULL) + kvstoreBuckets(kvs) * sizeof(dictEntry *) +
            kvs->allocated_dicts * (sizeof(dict) + kvstoreDictMetadataSize(NULL));
 
     /* Values are dict* shared with kvs->dicts */
diff --git a/src/object.c b/src/object.c
index f1b5e27d13..b366984d06 100644
--- a/src/object.c
+++ b/src/object.c
@@ -1010,7 +1010,7 @@ size_t objectComputeSize(robj *key, robj *o, size_t sample_size, int dbid) {
             asize = sizeof(*o) + sizeof(dict) + (sizeof(struct dictEntry *) * dictBuckets(d));
             while ((de = dictNext(di)) != NULL && samples < sample_size) {
                 ele = dictGetKey(de);
-                elesize += dictEntryMemUsage() + sdsZmallocSize(ele);
+                elesize += dictEntryMemUsage(de) + sdsZmallocSize(ele);
                 samples++;
             }
             dictReleaseIterator(di);
@@ -1033,7 +1033,7 @@ size_t objectComputeSize(robj *key, robj *o, size_t sample_size, int dbid) {
                     (sizeof(struct dictEntry *) * dictBuckets(d)) + zmalloc_size(zsl->header);
             while (znode != NULL && samples < sample_size) {
                 elesize += sdsZmallocSize(znode->ele);
-                elesize += dictEntryMemUsage() + zmalloc_size(znode);
+                elesize += dictEntryMemUsage(NULL) + zmalloc_size(znode);
                 samples++;
                 znode = znode->level[0].forward;
             }
@@ -1052,7 +1052,7 @@ size_t objectComputeSize(robj *key, robj *o, size_t sample_size, int dbid) {
                 ele = dictGetKey(de);
                 ele2 = dictGetVal(de);
                 elesize += sdsZmallocSize(ele) + sdsZmallocSize(ele2);
-                elesize += dictEntryMemUsage();
+                elesize += dictEntryMemUsage(de);
                 samples++;
             }
             dictReleaseIterator(di);
@@ -1552,7 +1552,7 @@ NULL
             return;
         }
         size_t usage = objectComputeSize(c->argv[2], dictGetVal(de), samples, c->db->id);
-        usage += zmalloc_size(de);
+        usage += dictEntryMemUsage(de);
         addReplyLongLong(c, usage);
     } else if (!strcasecmp(c->argv[1]->ptr, "stats") && c->argc == 2) {
         struct serverMemOverhead *mh = getMemoryOverheadData();

From 88c7a4d27e5b02dd77965cb3bfeb51b76780589e Mon Sep 17 00:00:00 2001
From: Harkrishn Patro <harkrisp@amazon.com>
Date: Thu, 27 Jun 2024 23:02:17 +0000
Subject: [PATCH 10/21] Format

Signed-off-by: Harkrishn Patro <harkrisp@amazon.com>
---
 src/dict.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/src/dict.c b/src/dict.c
index 0776929171..c5f5d4ac1c 100644
--- a/src/dict.c
+++ b/src/dict.c
@@ -88,7 +88,7 @@ typedef struct {
         int64_t s64;
         double d;
     } v;
-    struct dictEntry *next; /* Next entry in the same hash bucket. */
+    struct dictEntry *next;  /* Next entry in the same hash bucket. */
     uint8_t key_header_size; /* offset into key_buf where the key is located at. */
     unsigned char key_buf[]; /* buffer to embed the key. */
 } embeddedDictEntry;
@@ -971,11 +971,16 @@ size_t dictMemUsage(const dict *d) {
 /* Returns the memory usage in bytes of dictEntry based on the type. if `de` is NULL, return the size of
  * regular dict entry else return based on the type. */
 size_t dictEntryMemUsage(dictEntry *de) {
-    if (de == NULL || entryIsNormal(de)) return sizeof(dictEntry);
-    else if (entryIsKey(de)) return 0;
-    else if (entryIsNoValue(de)) return sizeof(dictEntryNoValue);
-    else if (entryIsEmbedded(de)) return zmalloc_size(decodeEmbeddedEntry(de));
-    else assert("Entry type not supported");
+    if (de == NULL || entryIsNormal(de))
+        return sizeof(dictEntry);
+    else if (entryIsKey(de))
+        return 0;
+    else if (entryIsNoValue(de))
+        return sizeof(dictEntryNoValue);
+    else if (entryIsEmbedded(de))
+        return zmalloc_size(decodeEmbeddedEntry(de));
+    else
+        assert("Entry type not supported");
     return 0;
 }
 

From 417b15a1641c7a01146c4a7fb0512f1259a22eb6 Mon Sep 17 00:00:00 2001
From: Harkrishn Patro <harkrisp@amazon.com>
Date: Thu, 27 Jun 2024 23:13:16 +0000
Subject: [PATCH 11/21] Address feedback

Signed-off-by: Harkrishn Patro <harkrisp@amazon.com>
---
 src/db.c   | 7 +++----
 src/dict.c | 2 +-
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/src/db.c b/src/db.c
index 1a909827ac..956283498e 100644
--- a/src/db.c
+++ b/src/db.c
@@ -240,15 +240,14 @@ int getKeySlot(sds key) {
 
 /* This is a special version of dbAdd() that is used only when loading
  * keys from the RDB file: the key is passed as an SDS string that is
- * retained by the function (and not freed by the caller).
+ * copied by the function and freed by the caller.
  *
  * Moreover this function will not abort if the key is already busy, to
  * give more control to the caller, nor will signal the key as ready
  * since it is not useful in this context.
  *
- * The function returns 1 if the key was added to the database, taking
- * ownership of the SDS string, otherwise 0 is returned, and is up to the
- * caller to free the SDS string. */
+ * The function returns 1 if the key was added to the database, making a
+ * copy of the SDS string, otherwise 0 is returned, The caller should free the SDS string. */
 int dbAddRDBLoad(serverDb *db, sds key, robj *val) {
     int slot = getKeySlot(key);
     dictEntry *de = kvstoreDictAddRaw(db->keys, slot, key, NULL);
diff --git a/src/dict.c b/src/dict.c
index c5f5d4ac1c..600efe0de0 100644
--- a/src/dict.c
+++ b/src/dict.c
@@ -224,7 +224,7 @@ static inline embeddedDictEntry *decodeEmbeddedEntry(const dictEntry *de) {
 
 /* Returns 1 if the entry has a value field and 0 otherwise. */
 static inline int entryHasValue(const dictEntry *de) {
-    return !entryIsNoValue(de);
+    return entryIsNormal(de) || entryIsEmbedded(de);
 }
 
 /* ----------------------------- API implementation ------------------------- */

From e00a6338cbe47d9bdc2b2efb2765d15e2718deae Mon Sep 17 00:00:00 2001
From: Harkrishn Patro <harkrisp@amazon.com>
Date: Thu, 27 Jun 2024 23:20:58 +0000
Subject: [PATCH 12/21] Try to make 32 bit compilation happy

Signed-off-by: Harkrishn Patro <harkrisp@amazon.com>
---
 src/dict.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/dict.c b/src/dict.c
index 600efe0de0..13b4f90fa7 100644
--- a/src/dict.c
+++ b/src/dict.c
@@ -94,11 +94,13 @@ typedef struct {
 } embeddedDictEntry;
 
 /* Validation and helper for `embeddedDictEntry` */
+
 static_assert(sizeof(embeddedDictEntry) == 24, "unexpected total size of embeddedDictEntry");
 static_assert(offsetof(embeddedDictEntry, v) == 0, "unexpected field offset");
-static_assert(offsetof(embeddedDictEntry, next) == 8, "unexpected field offset");
-static_assert(offsetof(embeddedDictEntry, key_header_size) == 16, "unexpected field offset");
-static_assert(offsetof(embeddedDictEntry, key_buf) == 17, "unexpected field offset");
+static_assert(offsetof(embeddedDictEntry, next) == sizeof(void *), "unexpected field offset");
+static_assert(offsetof(embeddedDictEntry, key_header_size) == 2 * sizeof(void *), "unexpected field offset");
+/* key_buf is located after two pointers `v.val`, `next` and uint8_t field `key_header_size` */
+static_assert(offsetof(embeddedDictEntry, key_buf) == (2 * sizeof(void *)) + sizeof(uint8_t), "unexpected field offset");
 
 /* The minimum amount of bytes required for embedded dict entry. */
 static inline size_t compactSizeEmbeddedDictEntry(void) {

From d9d1cd35ac939e4083deb693f043ecdaa229ac2b Mon Sep 17 00:00:00 2001
From: Harkrishn Patro <harkrisp@amazon.com>
Date: Thu, 27 Jun 2024 23:27:11 +0000
Subject: [PATCH 13/21] Remove total size check

Signed-off-by: Harkrishn Patro <harkrisp@amazon.com>
---
 src/dict.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/dict.c b/src/dict.c
index 13b4f90fa7..00ac118089 100644
--- a/src/dict.c
+++ b/src/dict.c
@@ -95,7 +95,6 @@ typedef struct {
 
 /* Validation and helper for `embeddedDictEntry` */
 
-static_assert(sizeof(embeddedDictEntry) == 24, "unexpected total size of embeddedDictEntry");
 static_assert(offsetof(embeddedDictEntry, v) == 0, "unexpected field offset");
 static_assert(offsetof(embeddedDictEntry, next) == sizeof(void *), "unexpected field offset");
 static_assert(offsetof(embeddedDictEntry, key_header_size) == 2 * sizeof(void *), "unexpected field offset");

From 32cafc6aa94c74473211ed55971dc72120958fa1 Mon Sep 17 00:00:00 2001
From: Harkrishn Patro <harkrisp@amazon.com>
Date: Thu, 27 Jun 2024 23:30:14 +0000
Subject: [PATCH 14/21] clang format

Signed-off-by: Harkrishn Patro <harkrisp@amazon.com>
---
 src/dict.c   |  3 ++-
 src/server.c | 18 ++++++++----------
 2 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/src/dict.c b/src/dict.c
index 00ac118089..ab6afa1feb 100644
--- a/src/dict.c
+++ b/src/dict.c
@@ -99,7 +99,8 @@ static_assert(offsetof(embeddedDictEntry, v) == 0, "unexpected field offset");
 static_assert(offsetof(embeddedDictEntry, next) == sizeof(void *), "unexpected field offset");
 static_assert(offsetof(embeddedDictEntry, key_header_size) == 2 * sizeof(void *), "unexpected field offset");
 /* key_buf is located after two pointers `v.val`, `next` and uint8_t field `key_header_size` */
-static_assert(offsetof(embeddedDictEntry, key_buf) == (2 * sizeof(void *)) + sizeof(uint8_t), "unexpected field offset");
+static_assert(offsetof(embeddedDictEntry, key_buf) == (2 * sizeof(void *)) + sizeof(uint8_t),
+              "unexpected field offset");
 
 /* The minimum amount of bytes required for embedded dict entry. */
 static inline size_t compactSizeEmbeddedDictEntry(void) {
diff --git a/src/server.c b/src/server.c
index 7caf184ebe..a68cc1a0e7 100644
--- a/src/server.c
+++ b/src/server.c
@@ -468,16 +468,14 @@ dictType zsetDictType = {
 };
 
 /* Db->dict, keys are sds strings, vals are Objects. */
-dictType dbDictType = {
-    dictSdsHash,          /* hash function */
-    NULL,                 /* key dup */
-    dictSdsKeyCompare,    /* key compare */
-    NULL,                 /* key is embedded in the dictEntry and freed internally */
-    dictObjectDestructor, /* val destructor */
-    dictResizeAllowed,    /* allow to resize */
-    .embedKey = dictSdsEmbedKey,
-    .embedded_entry = 1
-};
+dictType dbDictType = {dictSdsHash,          /* hash function */
+                       NULL,                 /* key dup */
+                       dictSdsKeyCompare,    /* key compare */
+                       NULL,                 /* key is embedded in the dictEntry and freed internally */
+                       dictObjectDestructor, /* val destructor */
+                       dictResizeAllowed,    /* allow to resize */
+                       .embedKey = dictSdsEmbedKey,
+                       .embedded_entry = 1};
 
 /* Db->expires */
 dictType dbExpiresDictType = {

From 67872b1cae2d2d2dba3d1ce46491b14185d0d24f Mon Sep 17 00:00:00 2001
From: Harkrishn Patro <harkrisp@amazon.com>
Date: Fri, 28 Jun 2024 04:12:55 +0000
Subject: [PATCH 15/21] Fix offset calculation for 32 bit

Signed-off-by: Harkrishn Patro <harkrisp@amazon.com>
---
 src/dict.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/dict.c b/src/dict.c
index ab6afa1feb..6f41db2fde 100644
--- a/src/dict.c
+++ b/src/dict.c
@@ -96,10 +96,10 @@ typedef struct {
 /* Validation and helper for `embeddedDictEntry` */
 
 static_assert(offsetof(embeddedDictEntry, v) == 0, "unexpected field offset");
-static_assert(offsetof(embeddedDictEntry, next) == sizeof(void *), "unexpected field offset");
-static_assert(offsetof(embeddedDictEntry, key_header_size) == 2 * sizeof(void *), "unexpected field offset");
-/* key_buf is located after two pointers `v.val`, `next` and uint8_t field `key_header_size` */
-static_assert(offsetof(embeddedDictEntry, key_buf) == (2 * sizeof(void *)) + sizeof(uint8_t),
+static_assert(offsetof(embeddedDictEntry, next) == sizeof(double), "unexpected field offset");
+static_assert(offsetof(embeddedDictEntry, key_header_size) == sizeof(double) + sizeof(void *), "unexpected field offset");
+/* key_buf is located after a union with a double value  `v.d`, a pointer `next` and uint8_t field `key_header_size` */
+static_assert(offsetof(embeddedDictEntry, key_buf) == sizeof(double) + sizeof(void *) + sizeof(uint8_t),
               "unexpected field offset");
 
 /* The minimum amount of bytes required for embedded dict entry. */

From 1c7fc685fa864ded5f1e05b28b9c5f58c3518402 Mon Sep 17 00:00:00 2001
From: Harkrishn Patro <harkrisp@amazon.com>
Date: Fri, 28 Jun 2024 04:19:09 +0000
Subject: [PATCH 16/21] Make clang format happy

Signed-off-by: Harkrishn Patro <harkrisp@amazon.com>
---
 src/dict.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/dict.c b/src/dict.c
index 6f41db2fde..4d4bd419bc 100644
--- a/src/dict.c
+++ b/src/dict.c
@@ -97,7 +97,8 @@ typedef struct {
 
 static_assert(offsetof(embeddedDictEntry, v) == 0, "unexpected field offset");
 static_assert(offsetof(embeddedDictEntry, next) == sizeof(double), "unexpected field offset");
-static_assert(offsetof(embeddedDictEntry, key_header_size) == sizeof(double) + sizeof(void *), "unexpected field offset");
+static_assert(offsetof(embeddedDictEntry, key_header_size) == sizeof(double) + sizeof(void *),
+              "unexpected field offset");
 /* key_buf is located after a union with a double value  `v.d`, a pointer `next` and uint8_t field `key_header_size` */
 static_assert(offsetof(embeddedDictEntry, key_buf) == sizeof(double) + sizeof(void *) + sizeof(uint8_t),
               "unexpected field offset");

From c61bd70dfb9a2d7819b3a692993ae497b10a70cb Mon Sep 17 00:00:00 2001
From: Harkrishn Patro <harkrisp@amazon.com>
Date: Fri, 28 Jun 2024 05:17:56 +0000
Subject: [PATCH 17/21] Update code comment

Signed-off-by: Harkrishn Patro <harkrisp@amazon.com>
---
 src/dict.c | 2 +-
 src/dict.h | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/dict.c b/src/dict.c
index 4d4bd419bc..b550d63afc 100644
--- a/src/dict.c
+++ b/src/dict.c
@@ -90,7 +90,7 @@ typedef struct {
     } v;
     struct dictEntry *next;  /* Next entry in the same hash bucket. */
     uint8_t key_header_size; /* offset into key_buf where the key is located at. */
-    unsigned char key_buf[]; /* buffer to embed the key. */
+    unsigned char key_buf[]; /* buffer with embedded key. */
 } embeddedDictEntry;
 
 /* Validation and helper for `embeddedDictEntry` */
diff --git a/src/dict.h b/src/dict.h
index 6f0eef8371..a7c5c71826 100644
--- a/src/dict.h
+++ b/src/dict.h
@@ -84,8 +84,8 @@ typedef struct dictType {
      * enables one more optimization: to store a key without an allocated
      * dictEntry. */
     unsigned int keys_are_odd : 1;
-    /* If embedded_entry flag is set, it indicates that the key is embedded as part of
-     * the dict entry. */
+    /* If embedded_entry flag is set, it indicates that a copy of the key is created and the key is embedded
+     * as part of the dict entry. */
     unsigned int embedded_entry : 1;
 } dictType;
 

From ce76a16ef40e81e7a0c08dd902f7961dbcaa3da0 Mon Sep 17 00:00:00 2001
From: Harkrishn Patro <harkrisp@amazon.com>
Date: Fri, 28 Jun 2024 20:30:30 +0000
Subject: [PATCH 18/21] Update code comment and variable naming

Signed-off-by: Harkrishn Patro <harkrisp@amazon.com>
---
 src/db.c      | 18 +++++++++++++++---
 src/dict.c    |  4 ++++
 src/kvstore.h |  6 ++++++
 src/sds.c     | 10 +++++-----
 src/server.h  |  1 -
 5 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/src/db.c b/src/db.c
index 956283498e..e7bad3f41c 100644
--- a/src/db.c
+++ b/src/db.c
@@ -190,7 +190,14 @@ robj *lookupKeyWriteOrReply(client *c, robj *key, robj *reply) {
     return o;
 }
 
-/* Add the key to the DB. It's up to the caller to increment the reference
+/* Add the key to the DB.
+ *
+ * The kvstore handles `key` based on `dictType` during initialization:
+ * - If `dictType.embedded-entry` is 1, it clones the `key`.
+ * - Otherwise, it assumes ownership of the `key`.
+ * In this case a copy of `key` is made in kvstore, the caller must ensure the `key` is properly freed.
+ *
+ * It's up to the caller to increment the reference
  * counter of the value if needed.
  *
  * If the update_if_existing argument is false, the program is aborted
@@ -246,8 +253,13 @@ int getKeySlot(sds key) {
  * give more control to the caller, nor will signal the key as ready
  * since it is not useful in this context.
  *
- * The function returns 1 if the key was added to the database, making a
- * copy of the SDS string, otherwise 0 is returned, The caller should free the SDS string. */
+ * The function returns 1 if the key was added to the database, otherwise 0 is returned.
+ *
+ * The kvstore handles `key` based on `dictType` during initialization:
+ * - If `dictType.embedded-entry` is 1, it clones the `key`.
+ * - Otherwise, it assumes ownership of the `key`.
+ * In this case a copy of `key` is made in kvstore, the caller must ensure the `key` is properly freed.
+ */
 int dbAddRDBLoad(serverDb *db, sds key, robj *val) {
     int slot = getKeySlot(key);
     dictEntry *de = kvstoreDictAddRaw(db->keys, slot, key, NULL);
diff --git a/src/dict.c b/src/dict.c
index b550d63afc..3ed74e5689 100644
--- a/src/dict.c
+++ b/src/dict.c
@@ -529,6 +529,10 @@ int dictAdd(dict *d, void *key, void *val) {
  * with the existing entry if existing is not NULL.
  *
  * If key was added, the hash entry is returned to be manipulated by the caller.
+ *
+ * The dict handles `key` based on `dictType` during initialization:
+ * - If `dictType.embedded-entry` is 1, it clones the `key`.
+ * - Otherwise, it assumes ownership of the `key`.
  */
 dictEntry *dictAddRaw(dict *d, void *key, dictEntry **existing) {
     /* Get the position for the new key or NULL if the key already exists. */
diff --git a/src/kvstore.h b/src/kvstore.h
index a94f366b6b..c2bb572b01 100644
--- a/src/kvstore.h
+++ b/src/kvstore.h
@@ -70,6 +70,12 @@ typedef dict *(kvstoreDictLUTDefragFunction)(dict *d);
 void kvstoreDictLUTDefrag(kvstore *kvs, kvstoreDictLUTDefragFunction *defragfn);
 void *kvstoreDictFetchValue(kvstore *kvs, int didx, const void *key);
 dictEntry *kvstoreDictFind(kvstore *kvs, int didx, void *key);
+/*
+ * The kvstore handles `key` based on `dictType` during initialization:
+ * - If `dictType.embedded-entry` is 1, it clones the `key`.
+ * - Otherwise, it assumes ownership of the `key`.
+ * The caller must ensure the `key` is properly freed.
+ */
 dictEntry *kvstoreDictAddRaw(kvstore *kvs, int didx, void *key, dictEntry **existing);
 void kvstoreDictSetKey(kvstore *kvs, int didx, dictEntry *de, void *key);
 void kvstoreDictSetVal(kvstore *kvs, int didx, dictEntry *de, void *val);
diff --git a/src/sds.c b/src/sds.c
index bae5b3d5b2..ba3362e88a 100644
--- a/src/sds.c
+++ b/src/sds.c
@@ -201,14 +201,14 @@ static inline size_t sdsminlen(sds s) {
 
 /* This method copies the sds `s` into `buf` which is the target character buffer. */
 size_t sdscopytobuffer(unsigned char *buf, size_t buf_len, sds s, uint8_t *hdr_size) {
-    size_t reqd_keylen = sdsminlen(s);
+    size_t required_keylen = sdsminlen(s);
     if (buf == NULL) {
-        return reqd_keylen;
+        return required_keylen;
     }
-    assert(buf_len >= reqd_keylen);
-    memcpy(buf, sdsAllocPtr(s), reqd_keylen);
+    assert(buf_len >= required_keylen);
+    memcpy(buf, sdsAllocPtr(s), required_keylen);
     *hdr_size = sdsHdrSize(s[-1]);
-    return reqd_keylen;
+    return required_keylen;
 }
 
 /* Free an sds string. No operation is performed if 's' is NULL. */
diff --git a/src/server.h b/src/server.h
index 8b5f7a0e9b..90efc6aa9c 100644
--- a/src/server.h
+++ b/src/server.h
@@ -3558,7 +3558,6 @@ int dictSdsKeyCaseCompare(dict *d, const void *key1, const void *key2);
 void dictSdsDestructor(dict *d, void *val);
 void dictListDestructor(dict *d, void *val);
 void *dictSdsDup(dict *d, const void *key);
-size_t dictSdsKeyToBytes(unsigned char *buf, const void *key, unsigned char *header_size);
 
 /* Git SHA1 */
 char *serverGitSHA1(void);

From 11400e2e0fa0d62248ae63915662706bda5fb40e Mon Sep 17 00:00:00 2001
From: Madelyn Olson <madelyneolson@gmail.com>
Date: Sat, 29 Jun 2024 20:57:00 -0700
Subject: [PATCH 19/21] Revert some formatting changes

Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
---
 src/server.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/src/server.c b/src/server.c
index a68cc1a0e7..ac1ccbfb74 100644
--- a/src/server.c
+++ b/src/server.c
@@ -468,14 +468,16 @@ dictType zsetDictType = {
 };
 
 /* Db->dict, keys are sds strings, vals are Objects. */
-dictType dbDictType = {dictSdsHash,          /* hash function */
-                       NULL,                 /* key dup */
-                       dictSdsKeyCompare,    /* key compare */
-                       NULL,                 /* key is embedded in the dictEntry and freed internally */
-                       dictObjectDestructor, /* val destructor */
-                       dictResizeAllowed,    /* allow to resize */
-                       .embedKey = dictSdsEmbedKey,
-                       .embedded_entry = 1};
+dictType dbDictType = {
+    dictSdsHash,          /* hash function */
+    NULL,                 /* key dup */
+    dictSdsKeyCompare,    /* key compare */
+    NULL,                 /* key is embedded in the dictEntry and freed internally */
+    dictObjectDestructor, /* val destructor */
+    dictResizeAllowed,    /* allow to resize */
+    .embedKey = dictSdsEmbedKey,
+    .embedded_entry = 1,
+};
 
 /* Db->expires */
 dictType dbExpiresDictType = {

From 4ca43ce7f124d3f4792e6a59a1fe514d9222ee0f Mon Sep 17 00:00:00 2001
From: Harkrishn Patro <harkrisp@amazon.com>
Date: Mon, 1 Jul 2024 17:35:34 +0000
Subject: [PATCH 20/21] Update code comment

Signed-off-by: Harkrishn Patro <harkrisp@amazon.com>
---
 src/db.c      | 10 ++--------
 src/kvstore.c | 18 +++++++++++++++++-
 src/kvstore.h |  6 ------
 3 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/src/db.c b/src/db.c
index e7bad3f41c..87977d3c56 100644
--- a/src/db.c
+++ b/src/db.c
@@ -192,10 +192,7 @@ robj *lookupKeyWriteOrReply(client *c, robj *key, robj *reply) {
 
 /* Add the key to the DB.
  *
- * The kvstore handles `key` based on `dictType` during initialization:
- * - If `dictType.embedded-entry` is 1, it clones the `key`.
- * - Otherwise, it assumes ownership of the `key`.
- * In this case a copy of `key` is made in kvstore, the caller must ensure the `key` is properly freed.
+ * In this case a copy of `key` is copied in kvstore, the caller must ensure the `key` is properly freed.
  *
  * It's up to the caller to increment the reference
  * counter of the value if needed.
@@ -255,10 +252,7 @@ int getKeySlot(sds key) {
  *
  * The function returns 1 if the key was added to the database, otherwise 0 is returned.
  *
- * The kvstore handles `key` based on `dictType` during initialization:
- * - If `dictType.embedded-entry` is 1, it clones the `key`.
- * - Otherwise, it assumes ownership of the `key`.
- * In this case a copy of `key` is made in kvstore, the caller must ensure the `key` is properly freed.
+ * In this case a copy of `key` is copied in kvstore, the caller must ensure the `key` is properly freed.
  */
 int dbAddRDBLoad(serverDb *db, sds key, robj *val) {
     int slot = getKeySlot(key);
diff --git a/src/kvstore.c b/src/kvstore.c
index 00d2d7a58f..16cc8e4822 100644
--- a/src/kvstore.c
+++ b/src/kvstore.c
@@ -241,7 +241,12 @@ static size_t kvstoreDictMetadataSize(dict *d) {
 
 /* Create an array of dictionaries
  * num_dicts_bits is the log2 of the amount of dictionaries needed (e.g. 0 for 1 dict,
- * 3 for 8 dicts, etc.) */
+ * 3 for 8 dicts, etc.)
+ *
+ * The kvstore handles `key` based on `dictType` during initialization:
+ * - If `dictType.embedded-entry` is 1, it clones the `key`.
+ * - Otherwise, it assumes ownership of the `key`.
+ */
 kvstore *kvstoreCreate(dictType *type, int num_dicts_bits, int flags) {
     /* We can't support more than 2^16 dicts because we want to save 48 bits
      * for the dict cursor, see kvstoreScan */
@@ -770,6 +775,17 @@ dictEntry *kvstoreDictFind(kvstore *kvs, int didx, void *key) {
     return dictFind(d, key);
 }
 
+/*
+ * The kvstore handles `key` based on `dictType` during initialization:
+ * - If `dictType.embedded-entry` is 1, it clones the `key`.
+ * - Otherwise, it assumes ownership of the `key`.
+ * The caller must ensure the `key` is properly freed.
+ *
+ * kvstore current usage:
+ *
+ * 1. keyspace (db.keys) kvstore - creates a copy of the key.
+ * 2. expiry (db.expires), pubsub_channels and pubsubshard_channels kvstore - takes ownership of the key.
+ */
 dictEntry *kvstoreDictAddRaw(kvstore *kvs, int didx, void *key, dictEntry **existing) {
     dict *d = createDictIfNeeded(kvs, didx);
     dictEntry *ret = dictAddRaw(d, key, existing);
diff --git a/src/kvstore.h b/src/kvstore.h
index c2bb572b01..a94f366b6b 100644
--- a/src/kvstore.h
+++ b/src/kvstore.h
@@ -70,12 +70,6 @@ typedef dict *(kvstoreDictLUTDefragFunction)(dict *d);
 void kvstoreDictLUTDefrag(kvstore *kvs, kvstoreDictLUTDefragFunction *defragfn);
 void *kvstoreDictFetchValue(kvstore *kvs, int didx, const void *key);
 dictEntry *kvstoreDictFind(kvstore *kvs, int didx, void *key);
-/*
- * The kvstore handles `key` based on `dictType` during initialization:
- * - If `dictType.embedded-entry` is 1, it clones the `key`.
- * - Otherwise, it assumes ownership of the `key`.
- * The caller must ensure the `key` is properly freed.
- */
 dictEntry *kvstoreDictAddRaw(kvstore *kvs, int didx, void *key, dictEntry **existing);
 void kvstoreDictSetKey(kvstore *kvs, int didx, dictEntry *de, void *key);
 void kvstoreDictSetVal(kvstore *kvs, int didx, dictEntry *de, void *val);

From 825c82f1f20319fd7a04b170233d39d76e8fa8e4 Mon Sep 17 00:00:00 2001
From: Harkrishn Patro <harkrisp@amazon.com>
Date: Mon, 1 Jul 2024 18:13:55 +0000
Subject: [PATCH 21/21] Add runtime check on dict type member dependency

Signed-off-by: Harkrishn Patro <harkrisp@amazon.com>
---
 src/dict.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/src/dict.c b/src/dict.c
index 3ed74e5689..b6a06eb36a 100644
--- a/src/dict.c
+++ b/src/dict.c
@@ -123,6 +123,19 @@ static dictEntry *dictGetNext(const dictEntry *de);
 static dictEntry **dictGetNextRef(dictEntry *de);
 static void dictSetNext(dictEntry *de, dictEntry *next);
 
+/* -------------------------- Utility functions -------------------------------- */
+
+/* Validates dict type members dependencies. */
+static inline void validateDictType(dictType *type) {
+    if (type->embedded_entry) {
+        assert(type->embedKey);
+        assert(!type->keyDup);
+        assert(!type->keyDestructor);
+    } else {
+        assert(!type->embedKey);
+    }
+}
+
 /* -------------------------- hash functions -------------------------------- */
 
 static uint8_t dict_hash_function_seed[16];
@@ -241,6 +254,7 @@ static void _dictReset(dict *d, int htidx) {
 
 /* Create a new hash table */
 dict *dictCreate(dictType *type) {
+    validateDictType(type);
     size_t metasize = type->dictMetadataBytes ? type->dictMetadataBytes(NULL) : 0;
     dict *d = zmalloc(sizeof(*d) + metasize);
     if (metasize > 0) {