Skip to content

[CLEAN] Synthetic Benchmark PR #14646 - Replace fragile dict stored-key API with getKeyId callback #89

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tomerqodo wants to merge 1 commit into
base: base_pr_14646_20260121_3876
Choose a base branch
from
Open

[CLEAN] Synthetic Benchmark PR #14646 - Replace fragile dict stored-key API with getKeyId callback #89

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions src/defrag.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,10 +277,8 @@ void *activeDefragHfieldAndUpdateRef(void *ptr, void *privdata) {

/* Before the key is released, obtain the link to
* ensure we can safely access and update the key. */
dictUseStoredKeyApi(d, 1);
link = dictFindLink(d, ptr, NULL);
serverAssert(link);
dictUseStoredKeyApi(d, 0);

Entry *newEntry = activeDefragEntry(ptr);
if (newEntry)
Expand Down Expand Up @@ -481,7 +479,6 @@ void activeDefragLuaScriptDictCallback(void *privdata, const dictEntry *de, dict
}

void activeDefragHfieldDictCallback(void *privdata, const dictEntry *de, dictEntryLink plink) {
UNUSED(plink);
dict *d = privdata;
Entry *newEntry = NULL, *entry = dictGetKey(de);

Expand All @@ -490,7 +487,7 @@ void activeDefragHfieldDictCallback(void *privdata, const dictEntry *de, dictEnt
* during the hash expiry ebuckets defragmentation phase. */
if (entryGetExpiry(entry) == EB_EXPIRE_TIME_INVALID) {
if ((newEntry = activeDefragEntry(entry))) {
/* Hash dicts use no_value=1, so we must use dictSetKeyAtLink */
/* Hash dicts use no_value=1, so we must use dictSetKeyAtLink */
dictSetKeyAtLink(d, newEntry, &plink, 0);
}
}
Expand Down
66 changes: 30 additions & 36 deletions src/dict.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ static void dictSetNext(dictEntry *de, dictEntry *next);
static int dictDefaultCompare(dictCmpCache *cache, const void *key1, const void *key2);
static dictEntryLink dictFindLinkInternal(dict *d, const void *key, dictEntryLink *bucket);
dictEntryLink dictFindLinkForInsert(dict *d, const void *key, dictEntry **existing);
static dictEntry *dictInsertKeyAtLink(dict *d, void *key, dictEntryLink link);
static dictEntry *dictInsertKeyAtLink(dict *d, void *key __stored_key, dictEntryLink link);

/* -------------------------- unused --------------------------- */
void dictSetSignedIntegerVal(dictEntry *de, int64_t val);
Expand All @@ -89,18 +89,13 @@ int64_t dictIncrSignedIntegerVal(dictEntry *de, int64_t val);

typedef int (*keyCmpFunc)(dictCmpCache *cache, const void *key1, const void *key2);
static inline keyCmpFunc dictGetCmpFunc(dict *d) {
if (d->useStoredKeyApi && d->type->storedKeyCompare)
return d->type->storedKeyCompare;
if (d->type->keyCompare)
return d->type->keyCompare;
return dictDefaultCompare;
}

static inline uint64_t dictHashKey(dict *d, const void *key, int isStoredKey) {
if (isStoredKey && d->type->storedHashFunction)
return d->type->storedHashFunction(key);
else
return d->type->hashFunction(key);
static const void *dictStoredKey2Key(dict *d, const void *key __stored_key) {
return (d->type->keyFromStoredKey) ? d->type->keyFromStoredKey(key) : key;
}

/* -------------------------- hash functions -------------------------------- */
Expand All @@ -118,7 +113,7 @@ uint64_t siphash(const uint8_t *in, const size_t inlen, const uint8_t *k);
uint64_t siphash_nocase(const uint8_t *in, const size_t inlen, const uint8_t *k);

uint64_t dictGenHashFunction(const void *key, size_t len) {
return siphash(key,len,dict_hash_function_seed);
return siphash(key, len, dict_hash_function_seed);
}

uint64_t dictGenCaseHashFunction(const unsigned char *buf, size_t len) {
Expand Down Expand Up @@ -150,7 +145,7 @@ static inline int entryIsNormal(const dictEntry *de) {
}

/* Creates an entry without a value field. */
static inline dictEntry *createEntryNoValue(void *key, dictEntry *next) {
static inline dictEntry *createEntryNoValue(void *key __stored_key, dictEntry *next) {
dictEntryNoValue *entry = zmalloc(sizeof(*entry));
entry->key = key;
entry->next = next;
Expand Down Expand Up @@ -222,7 +217,6 @@ int _dictInit(dict *d, dictType *type)
d->rehashidx = -1;
d->pauserehash = 0;
d->pauseAutoResize = 0;
d->useStoredKeyApi = 0;
return DICT_OK;
}

Expand Down Expand Up @@ -333,10 +327,11 @@ static void rehashEntriesInBucketAtIndex(dict *d, uint64_t idx) {
dictEntry *nextde;
while (de) {
nextde = dictGetNext(de);
void *key = dictGetKey(de);
void *storedKey = dictGetKey(de);
/* Get the index in the new hash table */
if (d->ht_size_exp[1] > d->ht_size_exp[0]) {
h = dictHashKey(d, key, 1) & DICTHT_SIZE_MASK(d->ht_size_exp[1]);
const void *key = dictStoredKey2Key(d, storedKey);
h = dictGetHash(d, key) & DICTHT_SIZE_MASK(d->ht_size_exp[1]);
} else {
/* We're shrinking the table. The tables sizes are powers of
* two, so we simply mask the bucket index in the larger table
Expand All @@ -351,13 +346,13 @@ static void rehashEntriesInBucketAtIndex(dict *d, uint64_t idx) {
if (!entryIsKey(de)) zfree(decodeMaskedPtr(de));

if (d->type->keys_are_odd)
de = key; /* ENTRY_PTR_IS_ODD_KEY trivially set by the odd key. */
de = storedKey; /* ENTRY_PTR_IS_ODD_KEY trivially set by the odd key. */
else
de = encodeMaskedPtr(key, ENTRY_PTR_IS_EVEN_KEY);
de = encodeMaskedPtr(storedKey, ENTRY_PTR_IS_EVEN_KEY);

} else if (entryIsKey(de)) {
/* We don't have an allocated entry but we need one. */
de = createEntryNoValue(key, d->ht_table[1][h]);
de = createEntryNoValue(storedKey, d->ht_table[1][h]);
} else {
dictSetNext(de, d->ht_table[1][h]);
}
Expand Down Expand Up @@ -486,7 +481,7 @@ int _dictBucketRehash(dict *d, uint64_t idx) {
}

/* Add an element to the target hash table */
int dictAdd(dict *d, void *key, void *val)
int dictAdd(dict *d, void *key __stored_key, void *val)
{
dictEntry *entry = dictAddRaw(d,key,NULL);

Expand Down Expand Up @@ -519,10 +514,10 @@ int dictCompareKeys(dict *d, const void *key1, const void *key2) {
*
* If key was added, the hash entry is returned to be manipulated by the caller.
*/
dictEntry *dictAddRaw(dict *d, void *key, dictEntry **existing)
dictEntry *dictAddRaw(dict *d, void *key __stored_key, dictEntry **existing)
{
/* Get the position for the new key or NULL if the key already exists. */
void *position = dictFindLinkForInsert(d, key, existing);
void *position = dictFindLinkForInsert(d, dictStoredKey2Key(d, key), existing);
if (!position) return NULL;

/* Dup the key if necessary. */
Expand All @@ -535,7 +530,7 @@ dictEntry *dictAddRaw(dict *d, void *key, dictEntry **existing)
* call to dictFindLinkForInsert(). This is a low level function which allows
* splitting dictAddRaw in two parts. Normally, dictAddRaw or dictAdd should be
* used instead. It assumes that dictExpandIfNeeded() was called before. */
dictEntry *dictInsertKeyAtLink(dict *d, void *key, dictEntryLink link) {
dictEntry *dictInsertKeyAtLink(dict *d, void *key __stored_key, dictEntryLink link) {
dictEntryLink bucket = link; /* It's a bucket, but the API hides that. */
dictEntry *entry;
/* If rehashing is ongoing, we insert in table 1, otherwise in table 0.
Expand Down Expand Up @@ -580,7 +575,7 @@ dictEntry *dictInsertKeyAtLink(dict *d, void *key, dictEntryLink link) {
* Return 1 if the key was added from scratch, 0 if there was already an
* element with such key and dictReplace() just performed a value update
* operation. */
int dictReplace(dict *d, void *key, void *val)
int dictReplace(dict *d, void *key __stored_key, void *val)
{
dictEntry *entry, *existing;

Expand Down Expand Up @@ -611,7 +606,7 @@ int dictReplace(dict *d, void *key, void *val)
* existing key is returned.)
*
* See dictAddRaw() for more information. */
dictEntry *dictAddOrFind(dict *d, void *key) {
dictEntry *dictAddOrFind(dict *d, void *key __stored_key) {
dictEntry *entry, *existing;
entry = dictAddRaw(d,key,&existing);
return entry ? entry : existing;
Expand All @@ -629,7 +624,7 @@ static dictEntry *dictGenericDelete(dict *d, const void *key, int nofree) {
/* dict is empty */
if (dictSize(d) == 0) return NULL;

h = dictHashKey(d, key, d->useStoredKeyApi);
h = dictGetHash(d, key);
idx = h & DICTHT_SIZE_MASK(d->ht_size_exp[0]);

/* Rehash the hash table if needed */
Expand All @@ -643,7 +638,7 @@ static dictEntry *dictGenericDelete(dict *d, const void *key, int nofree) {
he = d->ht_table[table][idx];
prevHe = NULL;
while(he) {
void *he_key = dictGetKey(he);
const void *he_key = dictStoredKey2Key(d, dictGetKey(he));
if (key == he_key || cmpFunc(&cmpCache, key, he_key)) {
/* Unlink the element from the list */
if (prevHe)
Expand Down Expand Up @@ -772,7 +767,7 @@ static dictEntryLink dictFindLinkInternal(dict *d, const void *key, dictEntryLin
if (dictSize(d) == 0) return NULL;
}

const uint64_t hash = dictHashKey(d, key, d->useStoredKeyApi);
const uint64_t hash = dictGetHash(d, key);
idx = hash & DICTHT_SIZE_MASK(d->ht_size_exp[0]);
keyCmpFunc cmpFunc = dictGetCmpFunc(d);

Expand All @@ -790,7 +785,7 @@ static dictEntryLink dictFindLinkInternal(dict *d, const void *key, dictEntryLin
link = &(d->ht_table[table][idx]);
if (bucket) *bucket = link;
while(link && *link) {
void *visitedKey = dictGetKey(*link);
const void *visitedKey = dictStoredKey2Key(d, dictGetKey(*link));

/* Prefetch the next entry to improve cache efficiency */
redis_prefetch_read(dictGetNext(*link));
Expand Down Expand Up @@ -880,7 +875,7 @@ dictEntryLink dictFindLink(dict *d, const void *key, dictEntryLink *bucket) {
* newItem: 1 = Add a key with a new dictEntry.
* 0 = Set a key to an existing dictEntry.
*/
void dictSetKeyAtLink(dict *d, void *key, dictEntryLink *link, int newItem) {
void dictSetKeyAtLink(dict *d, void *key __stored_key, dictEntryLink *link, int newItem) {
dictEntryLink dummy = NULL;
if (link == NULL) link = &dummy;
void *addedKey = (d->type->keyDup) ? d->type->keyDup(d, key) : key;
Expand All @@ -895,9 +890,7 @@ void dictSetKeyAtLink(dict *d, void *key, dictEntryLink *link, int newItem) {
if (snap[0] != d->ht_size_exp[0] || snap[1] != d->ht_size_exp[1] || *link == NULL) {
dictEntryLink bucket;
/* Bypass dictFindLink() to search bucket even if dict is empty!!! */
dictUseStoredKeyApi(d, 1);
*link = dictFindLinkInternal(d, key, &bucket);
dictUseStoredKeyApi(d, 0);
*link = dictFindLinkInternal(d, dictStoredKey2Key(d, key), &bucket);
assert(bucket != NULL);
assert(*link == NULL);
*link = bucket; /* On newItem the link should be the bucket */
Expand Down Expand Up @@ -959,15 +952,15 @@ dictEntryLink dictTwoPhaseUnlinkFind(dict *d, const void *key, int *table_index)
if (dictSize(d) == 0) return NULL; /* dict is empty */
if (dictIsRehashing(d)) _dictRehashStep(d);

h = dictHashKey(d, key, d->useStoredKeyApi);
h = dictGetHash(d, key);
keyCmpFunc cmpFunc = dictGetCmpFunc(d);

for (table = 0; table <= 1; table++) {
idx = h & DICTHT_SIZE_MASK(d->ht_size_exp[table]);
if (table == 0 && (long)idx < d->rehashidx) continue;
dictEntry **ref = &d->ht_table[table][idx];
while (ref && *ref) {
void *de_key = dictGetKey(*ref);
const void *de_key = dictStoredKey2Key(d, dictGetKey(*ref));
if (key == de_key || cmpFunc(&cmpCache, key, de_key)) {
*table_index = table;
dictPauseRehashing(d);
Expand All @@ -993,7 +986,7 @@ void dictTwoPhaseUnlinkFree(dict *d, dictEntryLink plink, int table_index) {
dictResumeRehashing(d);
}

void dictSetKey(dict *d, dictEntry* de, void *key) {
void dictSetKey(dict *d, dictEntry* de, void *key __stored_key) {
assert(!d->type->no_value);
if (d->type->keyDup)
de->key = d->type->keyDup(d, key);
Expand Down Expand Up @@ -1747,7 +1740,7 @@ dictEntryLink dictFindLinkForInsert(dict *d, const void *key, dictEntry **existi
unsigned long idx, table;
dictCmpCache cmpCache = {0};
dictEntry *he;
uint64_t hash = dictHashKey(d, key, d->useStoredKeyApi);
uint64_t hash = dictGetHash(d, key);
if (existing) *existing = NULL;
idx = hash & DICTHT_SIZE_MASK(d->ht_size_exp[0]);

Expand All @@ -1764,7 +1757,7 @@ dictEntryLink dictFindLinkForInsert(dict *d, const void *key, dictEntry **existi
/* Search if this slot does not already contain the given key */
he = d->ht_table[table][idx];
while(he) {
void *he_key = dictGetKey(he);
const void *he_key = dictStoredKey2Key(d, dictGetKey(he));
if (key == he_key || cmpFunc(&cmpCache, key, he_key)) {
if (existing) *existing = he;
return NULL;
Expand Down Expand Up @@ -1802,8 +1795,9 @@ void dictSetResizeEnabled(dictResizeEnable enable) {
dict_can_resize = enable;
}

/* Compiler inlines this for internal calls within dict.c (verified with -O3). */
uint64_t dictGetHash(dict *d, const void *key) {
return dictHashKey(d, key, d->useStoredKeyApi);
return d->type->hashFunction(key);
}

/* Provides the old and new ht size for a given dictionary during rehashing. This method
Expand Down
64 changes: 29 additions & 35 deletions src/dict.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,19 @@
/* Hash table parameters */
#define HASHTABLE_MIN_FILL 8 /* Minimal hash table fill 12.5%(100/8) */

/* stored-key vs. key
* ------------------
* If dictType.keyFromStoredKey is non-NULL, then dict distinguishes between the
* lookup key and the actual stored-key object. In this case, "key" is used to
* locate entries, while "storedKey" is the actual element stored in the dict.
* If dictType.keyFromStoredKey is NULL, the lookup "key" and the stored-key are the
* same. This API is primarily relevant for no_value=1 dicts, where the key and value
* might be packed together. When values are stored separately, this identity
* distinction does not arise. The marker __stored_key is used to indicate that
* the pointer refers to the stored-key rather than the lookup key.
*/
#define __stored_key

typedef struct dictEntry dictEntry; /* opaque */
typedef struct dict dict;
typedef dictEntry **dictEntryLink; /* See description of dictFindLink() */
Expand All @@ -78,10 +91,10 @@ typedef struct dictCmpCache {
typedef struct dictType {
/* Callbacks */
uint64_t (*hashFunction)(const void *key);
void *(*keyDup)(dict *d, const void *key);
void *(*keyDup)(dict *d, const void *key __stored_key);
void *(*valDup)(dict *d, const void *obj);
int (*keyCompare)(dictCmpCache *cache, const void *key1, const void *key2);
void (*keyDestructor)(dict *d, void *key);
void (*keyDestructor)(dict *d, void *key __stored_key);
void (*valDestructor)(dict *d, void *obj);
int (*resizeAllowed)(size_t moreMem, double usedRatio);
/* Invoked at the start of dict initialization/rehashing (old and new ht are already created) */
Expand Down Expand Up @@ -112,30 +125,13 @@ typedef struct dictType {

/* Ensures that the entire hash table is rehashed at once if set. */
unsigned int force_full_rehash:1;

/* Sometimes we want the ability to store a key in a given way inside the hash
* function, and lookup it in some other way without resorting to any kind of
* conversion. For instance the key may be stored as a structure also
* representing other things, but the lookup happens via just a pointer to a
* null terminated string. Optionally providing additional hash/cmp functions,
* dict supports such usage. In that case we'll have a hashFunction() that will
* expect a null terminated C string, and a storedHashFunction() that will
* instead expect the structure. Similarly, the two comparison functions will
* work differently. The keyCompare() will treat the first argument as a pointer
* to a C string and the other as a structure (this way we can directly lookup
* the structure key using the C string). While the storedKeyCompare() will
* check if two pointers to the key in structure form are the same.
*
* However, functions of dict that gets key as argument (void *key) don't get
* any indication whether it is a lookup or stored key. To indicate that
* you intend to use key of type stored-key, and, consequently, use
* dedicated compare and hash functions of stored-key, is by calling
* dictUseStoredKeyApi(1) before using any of the dict functions that gets
* key as a parameter and then call again dictUseStoredKeyApi(0) once done.
*
* Set to NULL both functions, if you don't want to support this feature. */
uint64_t (*storedHashFunction)(const void *key);
int (*storedKeyCompare)(dictCmpCache *cache, const void *key1, const void *key2);

/* Callback to extract key from stored-key object. When set, the dict can
* store keys in one format (e.g., a structure) but look them up using a
* different format, extracted from the stored-key. (e.g., sds or integer).
* Set to NULL if key and stored-key object are the same. Relevant only for
* no_value=1 dicts. */
const void *(*keyFromStoredKey)(const void *key __stored_key);

/* Optional callback called when the dict is destroyed. */
void (*onDictRelease)(dict *d);
Expand All @@ -158,8 +154,7 @@ struct dict {

/* Keep small vars at end for optimal (minimal) struct padding */
signed char ht_size_exp[2]; /* exponent of size. (size = 1<<exp) */
signed pauseAutoResize: 15; /* If >0 automatic resizing is disallowed (<0 indicates coding error) */
unsigned useStoredKeyApi: 1; /* See comment of storedHashFunction above */
int16_t pauseAutoResize; /* If >0 automatic resizing is disallowed (<0 indicates coding error) */
void *metadata[];
};

Expand Down Expand Up @@ -221,7 +216,6 @@ typedef struct {
#define dictIsRehashingPaused(d) ((d)->pauserehash > 0)
#define dictPauseAutoResize(d) ((d)->pauseAutoResize++)
#define dictResumeAutoResize(d) ((d)->pauseAutoResize--)
#define dictUseStoredKeyApi(d, flag) ((d)->useStoredKeyApi = (flag))

/* If our unsigned long type can store a 64 bit number, use a 64 bit PRNG. */
#if ULONG_MAX >= 0xffffffffffffffff
Expand All @@ -242,10 +236,10 @@ void dictTypeAddMeta(dict **d, dictType *typeWithMeta);
int dictExpand(dict *d, unsigned long size);
int dictTryExpand(dict *d, unsigned long size);
int dictShrink(dict *d, unsigned long size);
int dictAdd(dict *d, void *key, void *val);
dictEntry *dictAddRaw(dict *d, void *key, dictEntry **existing);
dictEntry *dictAddOrFind(dict *d, void *key);
int dictReplace(dict *d, void *key, void *val);
int dictAdd(dict *d, void *key __stored_key, void *val);
dictEntry *dictAddRaw(dict *d, void *key __stored_key, dictEntry **existing);
dictEntry *dictAddOrFind(dict *d, void *key __stored_key);
int dictReplace(dict *d, void *key __stored_key, void *val);
int dictDelete(dict *d, const void *key);
dictEntry *dictUnlink(dict *d, const void *key);
void dictFreeUnlinkedEntry(dict *d, dictEntry *he);
Expand Down Expand Up @@ -291,10 +285,10 @@ void dictCombineStats(dictStats *from, dictStats *into);
void dictFreeStats(dictStats *stats);

dictEntryLink dictFindLink(dict *d, const void *key, dictEntryLink *bucket);
void dictSetKeyAtLink(dict *d, void *key, dictEntryLink *link, int newItem);
void dictSetKeyAtLink(dict *d, void *key __stored_key, dictEntryLink *link, int newItem);

/* API relevant only when dict is used as a hash-map (no_value=0) */
void dictSetKey(dict *d, dictEntry* de, void *key);
void dictSetKey(dict *d, dictEntry* de, void *key __stored_key);
void dictSetVal(dict *d, dictEntry *de, void *val);
void *dictGetVal(const dictEntry *de);
void dictSetDoubleVal(dictEntry *de, double val);
Expand Down
Loading
Loading