Skip to content

Commit

Permalink
Minor optimization in kvstoreDictAddRaw when dict exists (redis#13054)
Browse files Browse the repository at this point in the history
Usually, the probability that a dict exists is much greater than the
probability that it does not exist. In kvstoreDictAddRaw, we will call
kvstoreGetDict multiple times. Based on this assumption, we change
createDictIfNeeded to something like get or create function:
```
before:
dict exist: 2 kvstoreGetDict
dict non-exist: 2 kvstoreGetDict

after:
dict exist: 1 kvstoreGetDict
dict non-exist: 3 kvstoreGetDict
```

A possible 3% performance improvement was observed:

In addition, some typos/comments i saw have been cleaned up.
  • Loading branch information
enjoy-binbin authored Feb 15, 2024
1 parent 063de67 commit c854873
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 11 deletions.
22 changes: 12 additions & 10 deletions src/kvstore.c
Original file line number Diff line number Diff line change
Expand Up @@ -155,11 +155,14 @@ static void cumulativeKeyCountAdd(kvstore *kvs, int didx, long delta) {
}
}

static void createDictIfNeeded(kvstore *kvs, int didx) {
if (kvstoreGetDict(kvs, didx))
return;
/* Create the dict if it does not exist and return it. */
static dict *createDictIfNeeded(kvstore *kvs, int didx) {
dict *d = kvstoreGetDict(kvs, didx);
if (d) return d;

kvs->dicts[didx] = dictCreate(&kvs->dtype);
kvs->allocated_dicts++;
return kvs->dicts[didx];
}

static void freeDictIfNeeded(kvstore *kvs, int didx) {
Expand Down Expand Up @@ -236,7 +239,7 @@ kvstore *kvstoreCreate(dictType *type, int num_dicts_bits, int flags) {
memcpy(&kvs->dtype, type, sizeof(kvs->dtype));
kvs->flags = flags;

/* kvstore must be the one to set this callbacks, so we make sure the
/* kvstore must be the one to set these callbacks, so we make sure the
* caller didn't do it */
assert(!type->userdata);
assert(!type->dictMetadataBytes);
Expand Down Expand Up @@ -402,7 +405,7 @@ unsigned long long kvstoreScan(kvstore *kvs, unsigned long long cursor,
* Based on the parameter `try_expand`, appropriate dict expand API is invoked.
* if try_expand is set to 1, `dictTryExpand` is used else `dictExpand`.
* The return code is either `DICT_OK`/`DICT_ERR` for both the API(s).
* `DICT_OK` response is for successful expansion. However ,`DICT_ERR` response signifies failure in allocation in
* `DICT_OK` response is for successful expansion. However, `DICT_ERR` response signifies failure in allocation in
* `dictTryExpand` call and in case of `dictExpand` call it signifies no expansion was performed.
*/
int kvstoreExpand(kvstore *kvs, uint64_t newsize, int try_expand, kvstoreExpandShouldSkipDictIndex *skip_cb) {
Expand Down Expand Up @@ -581,7 +584,7 @@ dictEntry *kvstoreIteratorNext(kvstoreIterator *kvs_it) {
return de;
}

/* This method traverses through kvstore dictionaries and triggers a resize .
/* This method traverses through kvstore dictionaries and triggers a resize.
* It first tries to shrink if needed, and if it isn't, it tries to expand. */
void kvstoreTryResizeDicts(kvstore *kvs, int limit) {
if (limit > kvs->num_dicts)
Expand All @@ -599,7 +602,7 @@ void kvstoreTryResizeDicts(kvstore *kvs, int limit) {

/* Our hash table implementation performs rehashing incrementally while
* we write/read from the hash table. Still if the server is idle, the hash
* table will use two tables for a long time. So we try to use 1 millisecond
* table will use two tables for a long time. So we try to use threshold_us
* of CPU time at every call of this function to perform some rehashing.
*
* The function returns the amount of microsecs spent if some rehashing was
Expand All @@ -608,7 +611,7 @@ uint64_t kvstoreIncrementallyRehash(kvstore *kvs, uint64_t threshold_us) {
if (listLength(kvs->rehashing) == 0)
return 0;

/* Our goal is to rehash as many dictionaries as we can before reaching predefined threshold,
/* Our goal is to rehash as many dictionaries as we can before reaching threshold_us,
* after each dictionary completes rehashing, it removes itself from the list. */
listNode *node;
monotime timer;
Expand Down Expand Up @@ -754,8 +757,7 @@ dictEntry *kvstoreDictFind(kvstore *kvs, int didx, void *key) {
}

dictEntry *kvstoreDictAddRaw(kvstore *kvs, int didx, void *key, dictEntry **existing) {
createDictIfNeeded(kvs, didx);
dict *d = kvstoreGetDict(kvs, didx);
dict *d = createDictIfNeeded(kvs, didx);
dictEntry *ret = dictAddRaw(d, key, existing);
if (ret)
cumulativeKeyCountAdd(kvs, didx, 1);
Expand Down
2 changes: 1 addition & 1 deletion src/kvstore.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ dictEntry *kvstoreIteratorNext(kvstoreIterator *kvs_it);

/* Rehashing */
void kvstoreTryResizeDicts(kvstore *kvs, int limit);
uint64_t kvstoreIncrementallyRehash(kvstore *kvs, uint64_t threshold_ms);
uint64_t kvstoreIncrementallyRehash(kvstore *kvs, uint64_t threshold_us);

/* Specific dict access by dict-index */
unsigned long kvstoreDictSize(kvstore *kvs, int didx);
Expand Down

0 comments on commit c854873

Please sign in to comment.