From 6faa48a358ead15ea4b86798aa832a4820dfbe4e Mon Sep 17 00:00:00 2001 From: Madelyn Olson Date: Fri, 14 Jun 2024 08:42:00 -0700 Subject: [PATCH] Don't initialize the key buffer in getKeysResult (#631) getKeysResults is typically initialized with 2kb of zeros (16 * 256), which isn't strictly necessary since the only thing we have to initialize is some of the metadata fields. The rest of the data can remain junk as long as we don't access it. This was a bit of a regression in 7.0 with the keyspecs, since we doubled the size of the zeros, but hopefully this recovers a lot of the performance drop. I saw a modest performance bump for deep pipeline of cluster mode (~8%). I think we would see some comparable improvements in the other places where we are using it such as tracking and ACLs. --------- Signed-off-by: Madelyn Olson --- src/acl.c | 5 +++-- src/cluster.c | 3 ++- src/db.c | 2 +- src/module.c | 3 ++- src/server.c | 3 ++- src/server.h | 11 ++++++++--- src/tracking.c | 3 ++- 7 files changed, 20 insertions(+), 10 deletions(-) diff --git a/src/acl.c b/src/acl.c index 533782aca..bda449e8d 100644 --- a/src/acl.c +++ b/src/acl.c @@ -1713,7 +1713,7 @@ static int ACLSelectorCheckCmd(aclSelector *selector, * mentioned in the command arguments. */ if (!(selector->flags & SELECTOR_FLAG_ALLKEYS) && doesCommandHaveKeys(cmd)) { if (!(cache->keys_init)) { - cache->keys = (getKeysResult)GETKEYS_RESULT_INIT; + initGetKeysResult(&(cache->keys)); getKeysFromCommandWithSpecs(cmd, argv, argc, GET_KEYSPEC_DEFAULT, &(cache->keys)); cache->keys_init = 1; } @@ -1733,7 +1733,8 @@ static int ACLSelectorCheckCmd(aclSelector *selector, * mentioned in the command arguments */ const int channel_flags = CMD_CHANNEL_PUBLISH | CMD_CHANNEL_SUBSCRIBE; if (!(selector->flags & SELECTOR_FLAG_ALLCHANNELS) && doesCommandHaveChannelsWithFlags(cmd, channel_flags)) { - getKeysResult channels = (getKeysResult)GETKEYS_RESULT_INIT; + getKeysResult channels; + initGetKeysResult(&channels); getChannelsFromCommand(cmd, argv, argc, &channels); keyReference *channelref = channels.keys; for (int j = 0; j < channels.numkeys; j++) { diff --git a/src/cluster.c b/src/cluster.c index 3a4dccdff..00f3c2d88 100644 --- a/src/cluster.c +++ b/src/cluster.c @@ -1017,7 +1017,8 @@ getNodeByQuery(client *c, struct serverCommand *cmd, robj **argv, int argc, int margc = ms->commands[i].argc; margv = ms->commands[i].argv; - getKeysResult result = GETKEYS_RESULT_INIT; + getKeysResult result; + initGetKeysResult(&result); numkeys = getKeysFromCommand(mcmd, margv, margc, &result); keyindex = result.keys; diff --git a/src/db.c b/src/db.c index 1843395d8..c879b2ffb 100644 --- a/src/db.c +++ b/src/db.c @@ -1894,7 +1894,7 @@ unsigned long long dbScan(serverDb *db, unsigned long long cursor, dictScanFunct * the result, and can be called repeatedly to enlarge the result array. */ keyReference *getKeysPrepareResult(getKeysResult *result, int numkeys) { - /* GETKEYS_RESULT_INIT initializes keys to NULL, point it to the pre-allocated stack + /* initGetKeysResult initializes keys to NULL, point it to the pre-allocated stack * buffer here. */ if (!result->keys) { serverAssert(!result->numkeys); diff --git a/src/module.c b/src/module.c index f149443c8..c98c837a5 100644 --- a/src/module.c +++ b/src/module.c @@ -13247,7 +13247,8 @@ int *VM_GetCommandKeysWithFlags(ValkeyModuleCtx *ctx, return NULL; } - getKeysResult result = GETKEYS_RESULT_INIT; + getKeysResult result; + initGetKeysResult(&result); getKeysFromCommand(cmd, argv, argc, &result); *num_keys = result.numkeys; diff --git a/src/server.c b/src/server.c index 5065e40dd..e5df60be6 100644 --- a/src/server.c +++ b/src/server.c @@ -4838,7 +4838,8 @@ void addReplyCommandDocs(client *c, struct serverCommand *cmd) { /* Helper for COMMAND GETKEYS and GETKEYSANDFLAGS */ void getKeysSubcommandImpl(client *c, int with_flags) { struct serverCommand *cmd = lookupCommand(c->argv + 2, c->argc - 2); - getKeysResult result = GETKEYS_RESULT_INIT; + getKeysResult result; + initGetKeysResult(&result); int j; if (!cmd) { diff --git a/src/server.h b/src/server.h index bf16a1893..ae2d23b99 100644 --- a/src/server.h +++ b/src/server.h @@ -2136,12 +2136,17 @@ typedef struct { * for returning channel information. */ typedef struct { - keyReference keysbuf[MAX_KEYS_BUFFER]; /* Pre-allocated buffer, to save heap allocations */ - keyReference *keys; /* Key indices array, points to keysbuf or heap */ int numkeys; /* Number of key indices return */ int size; /* Available array size */ + keyReference *keys; /* Key indices array, points to keysbuf or heap */ + keyReference keysbuf[MAX_KEYS_BUFFER]; /* Pre-allocated buffer, to save heap allocations */ } getKeysResult; -#define GETKEYS_RESULT_INIT {{{0}}, NULL, 0, MAX_KEYS_BUFFER} + +static inline void initGetKeysResult(getKeysResult *result) { + result->numkeys = 0; + result->size = MAX_KEYS_BUFFER; + result->keys = NULL; +} /* Key specs definitions. * diff --git a/src/tracking.c b/src/tracking.c index e33b9f46a..b95ca05b3 100644 --- a/src/tracking.c +++ b/src/tracking.c @@ -216,7 +216,8 @@ void trackingRememberKeys(client *tracking, client *executing) { uint64_t caching_given = tracking->flags & CLIENT_TRACKING_CACHING; if ((optin && !caching_given) || (optout && caching_given)) return; - getKeysResult result = GETKEYS_RESULT_INIT; + getKeysResult result; + initGetKeysResult(&result); int numkeys = getKeysFromCommand(executing->cmd, executing->argv, executing->argc, &result); if (!numkeys) { getKeysFreeResult(&result);