Skip to content

Commit

Permalink
Don't initialize the key buffer in getKeysResult (#631)
Browse files Browse the repository at this point in the history
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 <[email protected]>
  • Loading branch information
madolson authored Jun 14, 2024
1 parent d5496e4 commit 6faa48a
Show file tree
Hide file tree
Showing 7 changed files with 20 additions and 10 deletions.
5 changes: 3 additions & 2 deletions src/acl.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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++) {
Expand Down
3 changes: 2 additions & 1 deletion src/cluster.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
2 changes: 1 addition & 1 deletion src/db.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
3 changes: 2 additions & 1 deletion src/module.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
3 changes: 2 additions & 1 deletion src/server.c
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
11 changes: 8 additions & 3 deletions src/server.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down
3 changes: 2 additions & 1 deletion src/tracking.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down

0 comments on commit 6faa48a

Please sign in to comment.