Replace dict with thin wrapper around hashtable

Replace the dict.c implementation with a header-only wrapper (dict.h)
around the hashtable API. The dict types, iterators and API functions
are now typedefs, macros and inline functions that delegate to hashtable.
This unifies the hashtable implementations in the project and removes
duplicated logic.

Changes to dict:

- Remove dict.c; dict.h is now the entire implementation
- dict, dictType and dictIterator are direct aliases for the hashtable
  counterparts.
- dictEntry is a struct allocated by dict wrapper functions to hold key
  and value. It doesn't have a next pointer anymore.
- Fix key duplication for dictTypes that had keyDup callback by
  calling sdsdup() at call sites in functions.c
- Remove unused functions, macros, includes and casts
- Move some dict defrag logic to defrag.c
- Remove obsolete dict unit tests (covered by test_hashtable.cpp)

Changes to hashtable:

- Change hashtable keyCompare convention to match dict: non-zero means
  keys are equal, so existing dict compare functions can be reused
- Add const to hashtableMemUsage parameter

Changes to server implementation:

- Deduplicate common dict/hashtable callbacks in server.c

Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

Author: zuiderkwast

cmake/Modules/SourceFiles.cmake

@@ -10,7 +10,6 @@ set(VALKEY_SERVER_SRCS
     ${CMAKE_SOURCE_DIR}/src/quicklist.c
     ${CMAKE_SOURCE_DIR}/src/ae.c
     ${CMAKE_SOURCE_DIR}/src/anet.c
-    ${CMAKE_SOURCE_DIR}/src/dict.c
     ${CMAKE_SOURCE_DIR}/src/hashtable.c
     ${CMAKE_SOURCE_DIR}/src/kvstore.c
     ${CMAKE_SOURCE_DIR}/src/sds.c
@@ -128,7 +127,7 @@ set(VALKEY_SERVER_SRCS
 set(VALKEY_CLI_SRCS
     ${CMAKE_SOURCE_DIR}/src/anet.c
     ${CMAKE_SOURCE_DIR}/src/adlist.c
-    ${CMAKE_SOURCE_DIR}/src/dict.c
+    ${CMAKE_SOURCE_DIR}/src/hashtable.c
     ${CMAKE_SOURCE_DIR}/src/sds.c
     ${CMAKE_SOURCE_DIR}/src/sha256.c
     ${CMAKE_SOURCE_DIR}/src/util.c
@@ -159,7 +158,7 @@ set(VALKEY_BENCHMARK_SRCS
     ${CMAKE_SOURCE_DIR}/src/valkey-benchmark.c
     ${CMAKE_SOURCE_DIR}/src/valkey_strtod.c
     ${CMAKE_SOURCE_DIR}/src/adlist.c
-    ${CMAKE_SOURCE_DIR}/src/dict.c
+    ${CMAKE_SOURCE_DIR}/src/hashtable.c
     ${CMAKE_SOURCE_DIR}/src/zmalloc.c
     ${CMAKE_SOURCE_DIR}/src/serverassert.c
     ${CMAKE_SOURCE_DIR}/src/release.c

src/Makefile

@@ -478,7 +478,6 @@ ENGINE_SERVER_OBJ = \
     db.o \
     debug.o \
     defrag.o \
-    dict.o \
     entry.o \
     eval.o \
     evict.o \
@@ -574,7 +573,7 @@ ENGINE_CLI_OBJ = \
     crc64.o \
     crccombine.o \
     crcspeed.o \
-    dict.o \
+    hashtable.o \
     monotonic.o \
     mt19937-64.o \
     release.o \
@@ -598,9 +597,9 @@ ENGINE_BENCHMARK_OBJ = \
     crc64.o \
     crccombine.o \
     crcspeed.o \
-    dict.o \
     fuzzer_client.o \
     fuzzer_command_generator.o \
+    hashtable.o \
     monotonic.o \
     mt19937-64.o \
     release.o \

src/cluster_legacy.c

@@ -236,39 +236,33 @@ static_assert(offsetof(clusterMsg, type) + sizeof(uint16_t) == RCVBUF_MIN_READ_L
 /* Cluster nodes hash table, mapping nodes addresses 1.2.3.4:6379 to
  * clusterNode structures. */
 dictType clusterNodesDictType = {
-    dictSdsHash,       /* hash function */
-    NULL,              /* key dup */
-    dictSdsKeyCompare, /* key compare */
-    dictSdsDestructor, /* key destructor */
-    NULL,              /* val destructor */
-    NULL               /* allow to expand */
+    .entryGetKey = dictEntryGetKey,
+    .hashFunction = dictSdsHash,
+    .keyCompare = dictSdsKeyCompare,
+    .entryDestructor = dictEntryDestructorSdsKey,
 };
 
 /* Cluster re-addition blacklist. This maps node IDs to the time
  * we can re-add this node. The goal is to avoid reading a removed
  * node for some time. */
 dictType clusterNodesBlackListDictType = {
-    dictSdsCaseHash,       /* hash function */
-    NULL,                  /* key dup */
-    dictSdsKeyCaseCompare, /* key compare */
-    dictSdsDestructor,     /* key destructor */
-    NULL,                  /* val destructor */
-    NULL                   /* allow to expand */
+    .entryGetKey = dictEntryGetKey,
+    .hashFunction = dictSdsCaseHash,
+    .keyCompare = dictSdsKeyCaseCompare,
+    .entryDestructor = dictEntryDestructorSdsKey,
 };
 
 /* Cluster shards hash table, mapping shard id to list of nodes */
 dictType clusterSdsToListType = {
-    dictSdsHash,        /* hash function */
-    NULL,               /* key dup */
-    dictSdsKeyCompare,  /* key compare */
-    dictSdsDestructor,  /* key destructor */
-    dictListDestructor, /* val destructor */
-    NULL                /* allow to expand */
+    .entryGetKey = dictEntryGetKey,
+    .hashFunction = dictSdsHash,
+    .keyCompare = dictSdsKeyCompare,
+    .entryDestructor = dictEntryDestructorSdsKeyListValue,
 };
 
 static uint64_t dictPtrHash(const void *key) {
     /* We hash the pointer value itself. */
-    return dictGenHashFunction(&key, sizeof(key));
+    return dictGenHashFunction((const char *)&key, sizeof(key));
 }
 
 static int dictPtrCompare(const void *key1, const void *key2) {
@@ -278,12 +272,10 @@ static int dictPtrCompare(const void *key1, const void *key2) {
 /* Dictionary type for mapping hash slots to cluster nodes.
  * Keys are slot numbers encoded directly as pointer values, values are clusterNode pointers. */
 dictType clusterSlotDictType = {
-    dictPtrHash,    /* hash function */
-    NULL,           /* key dup */
-    dictPtrCompare, /* key compare */
-    NULL,           /* key destructor */
-    NULL,           /* val destructor */
-    NULL            /* allow to expand */
+    .entryGetKey = dictEntryGetKey,
+    .hashFunction = dictPtrHash,
+    .keyCompare = dictPtrCompare,
+    .entryDestructor = zfree,
 };
 
 typedef struct {
@@ -4852,7 +4844,7 @@ void clusterSendPing(clusterLink *link, int type) {
     int candidates_wanted = wanted + 2; /* +2 for myself and link->node */
     if (candidates_wanted > (int)dictSize(server.cluster->nodes))
         candidates_wanted = dictSize(server.cluster->nodes);
-    dictEntry **candidates = zmalloc(sizeof(dictEntry *) * candidates_wanted);
+    void **candidates = zmalloc(sizeof(void *) * candidates_wanted);
     unsigned int ncandidates = dictGetSomeKeys(server.cluster->nodes, candidates, candidates_wanted);
 
     for (unsigned int i = 0; i < ncandidates && gossipcount < wanted; i++) {

src/config.c

@@ -1041,21 +1041,17 @@ void rewriteConfigSentinelOption(struct rewriteConfigState *state);
  * like "maxmemory" -> list of line numbers (first line is zero).
  */
 dictType optionToLineDictType = {
-    dictSdsCaseHash,       /* hash function */
-    NULL,                  /* key dup */
-    dictSdsKeyCaseCompare, /* key compare */
-    dictSdsDestructor,     /* key destructor */
-    dictListDestructor,    /* val destructor */
-    NULL                   /* allow to expand */
+    .entryGetKey = dictEntryGetKey,
+    .hashFunction = dictSdsCaseHash,
+    .keyCompare = dictSdsKeyCaseCompare,
+    .entryDestructor = dictEntryDestructorSdsKeyListValue,
 };
 
 dictType optionSetDictType = {
-    dictSdsCaseHash,       /* hash function */
-    NULL,                  /* key dup */
-    dictSdsKeyCaseCompare, /* key compare */
-    dictSdsDestructor,     /* key destructor */
-    NULL,                  /* val destructor */
-    NULL                   /* allow to expand */
+    .entryGetKey = dictEntryGetKey,
+    .hashFunction = dictSdsCaseHash,
+    .keyCompare = dictSdsKeyCaseCompare,
+    .entryDestructor = dictEntryDestructorSdsKey,
 };
 
 /* The config rewrite state. */

src/defrag.c

@@ -299,24 +299,50 @@ static void activeDefragZsetNode(void *privdata, void *entry_ref) {
 #define DEFRAG_SDS_DICT_VAL_VOID_PTR 3
 #define DEFRAG_SDS_DICT_VAL_LUA_SCRIPT 4
 
-static void activeDefragSdsDictCallback(void *privdata, const dictEntry *de) {
-    UNUSED(privdata);
-    UNUSED(de);
+typedef void *(dictDefragAllocFunction)(void *ptr);
+typedef struct {
+    dictDefragAllocFunction *defragKey;
+    dictDefragAllocFunction *defragVal;
+} dictDefragFunctions;
+
+static void activeDefragDictCallback(void *privdata, void *entry_ref) {
+    dictDefragFunctions *defragfns = privdata;
+    dictEntry **de_ref = (dictEntry **)entry_ref;
+    dictEntry *de = *de_ref;
+
+    /* Defrag the entry itself */
+    dictEntry *newentry = activeDefragAlloc(de);
+    if (newentry) {
+        de = newentry;
+        *de_ref = newentry;
+    }
+
+    /* Defrag the key */
+    if (defragfns->defragKey) {
+        void *newkey = defragfns->defragKey(de->key);
+        if (newkey) de->key = newkey;
+    }
+
+    /* Defrag the value */
+    if (defragfns->defragVal) {
+        void *newval = defragfns->defragVal(de->v.val);
+        if (newval) de->v.val = newval;
+    }
 }
 
 /* Defrag a dict with sds key and optional value (either ptr, sds or robj string) */
 static void activeDefragSdsDict(dict *d, int val_type) {
     unsigned long cursor = 0;
     dictDefragFunctions defragfns = {
-        .defragAlloc = activeDefragAlloc,
         .defragKey = (dictDefragAllocFunction *)activeDefragSds,
         .defragVal = (val_type == DEFRAG_SDS_DICT_VAL_IS_SDS       ? (dictDefragAllocFunction *)activeDefragSds
                       : val_type == DEFRAG_SDS_DICT_VAL_IS_STROB   ? (dictDefragAllocFunction *)activeDefragStringOb
                       : val_type == DEFRAG_SDS_DICT_VAL_VOID_PTR   ? (dictDefragAllocFunction *)activeDefragAlloc
                       : val_type == DEFRAG_SDS_DICT_VAL_LUA_SCRIPT ? (dictDefragAllocFunction *)evalActiveDefragScript
                                                                    : NULL)};
     do {
-        cursor = dictScanDefrag(d, cursor, activeDefragSdsDictCallback, &defragfns, NULL);
+        cursor = hashtableScanDefrag(d, cursor, activeDefragDictCallback,
+                                     &defragfns, activeDefragAlloc, HASHTABLE_SCAN_EMIT_REF);
     } while (cursor != 0);
 }