From 26a2dcb936aa10eee5988dc9af2ec0b8631b6f4f Mon Sep 17 00:00:00 2001 From: "Filipe Oliveira (Redis)" <52153106+fcostaoliveira@users.noreply.github.com> Date: Wed, 3 Jul 2024 11:23:14 +0100 Subject: [PATCH] Reduce getNodeByQuery overhead (#13221) The following PR does the following changes based upon on CPU profile info. The `getNodeByQuery` function represents 8.2% of an overhead of 12.3% when comparing single shard cluster with standalone. Proposed changes: - inlinging keyHashSlot to reduce overhead of that function call - Reduce duplicate calls to getCommandFlags within getNodeByQuery The above changes represent an improvement of approximately 5% on the achievable ops/sec. Co-authored-by: filipecosta90 --- src/cluster.c | 30 +----------------------------- src/cluster.h | 31 +++++++++++++++++++++++++++++-- src/module.c | 3 ++- src/script.c | 3 ++- src/server.c | 4 ++-- 5 files changed, 36 insertions(+), 35 deletions(-) diff --git a/src/cluster.c b/src/cluster.c index d09a455b3..017b7f261 100644 --- a/src/cluster.c +++ b/src/cluster.c @@ -23,33 +23,6 @@ * Key space handling * -------------------------------------------------------------------------- */ -/* We have 16384 hash slots. The hash slot of a given key is obtained - * as the least significant 14 bits of the crc16 of the key. - * - * However, if the key contains the {...} pattern, only the part between - * { and } is hashed. This may be useful in the future to force certain - * keys to be in the same node (assuming no resharding is in progress). */ -unsigned int keyHashSlot(char *key, int keylen) { - int s, e; /* start-end indexes of { and } */ - - for (s = 0; s < keylen; s++) - if (key[s] == '{') break; - - /* No '{' ? Hash the whole key. This is the base case. */ - if (s == keylen) return crc16(key,keylen) & 0x3FFF; - - /* '{' found? Check if we have the corresponding '}'. */ - for (e = s+1; e < keylen; e++) - if (key[e] == '}') break; - - /* No '}' or nothing between {} ? Hash the whole key. */ - if (e == keylen || e == s+1) return crc16(key,keylen) & 0x3FFF; - - /* If we are here there is both a { and a } on its right. Hash - * what is in the middle between { and }. */ - return crc16(key+s+1,e-s-1) & 0x3FFF; -} - /* If it can be inferred that the given glob-style pattern, as implemented in * stringmatchlen() in util.c, only can match keys belonging to a single slot, * that slot is returned. Otherwise -1 is returned. */ @@ -980,7 +953,7 @@ void clusterCommand(client *c) { * * CLUSTER_REDIR_DOWN_STATE and CLUSTER_REDIR_DOWN_RO_STATE if the cluster is * down but the user attempts to execute a command that addresses one or more keys. */ -clusterNode *getNodeByQuery(client *c, struct redisCommand *cmd, robj **argv, int argc, int *hashslot, int *error_code) { +clusterNode *getNodeByQuery(client *c, struct redisCommand *cmd, robj **argv, int argc, int *hashslot, uint64_t cmd_flags, int *error_code) { clusterNode *myself = getMyClusterNode(); clusterNode *n = NULL; robj *firstkey = NULL; @@ -1116,7 +1089,6 @@ clusterNode *getNodeByQuery(client *c, struct redisCommand *cmd, robj **argv, in * without redirections or errors in all the cases. */ if (n == NULL) return myself; - uint64_t cmd_flags = getCommandFlags(c); /* Cluster is globally down but we got keys? We only serve the request * if it is a read command and when allow_reads_when_down is enabled. */ if (!isClusterHealthy()) { diff --git a/src/cluster.h b/src/cluster.h index f21f1e9c1..b213bb284 100644 --- a/src/cluster.h +++ b/src/cluster.h @@ -33,6 +33,34 @@ struct clusterState; #define CLUSTER_MODULE_FLAG_NO_REDIRECTION (1<<2) /* ---------------------- API exported outside cluster.c -------------------- */ + +/* We have 16384 hash slots. The hash slot of a given key is obtained + * as the least significant 14 bits of the crc16 of the key. + * + * However, if the key contains the {...} pattern, only the part between + * { and } is hashed. This may be useful in the future to force certain + * keys to be in the same node (assuming no resharding is in progress). */ +static inline unsigned int keyHashSlot(char *key, int keylen) { + int s, e; /* start-end indexes of { and } */ + + for (s = 0; s < keylen; s++) + if (key[s] == '{') break; + + /* No '{' ? Hash the whole key. This is the base case. */ + if (likely(s == keylen)) return crc16(key,keylen) & 0x3FFF; + + /* '{' found? Check if we have the corresponding '}'. */ + for (e = s+1; e < keylen; e++) + if (key[e] == '}') break; + + /* No '}' or nothing between {} ? Hash the whole key. */ + if (e == keylen || e == s+1) return crc16(key,keylen) & 0x3FFF; + + /* If we are here there is both a { and a } on its right. Hash + * what is in the middle between { and }. */ + return crc16(key+s+1,e-s-1) & 0x3FFF; +} + /* functions requiring mechanism specific implementations */ void clusterInit(void); void clusterInitLast(void); @@ -105,11 +133,10 @@ long long clusterNodeReplOffset(clusterNode *node); clusterNode *clusterLookupNode(const char *name, int length); /* functions with shared implementations */ -clusterNode *getNodeByQuery(client *c, struct redisCommand *cmd, robj **argv, int argc, int *hashslot, int *ask); +clusterNode *getNodeByQuery(client *c, struct redisCommand *cmd, robj **argv, int argc, int *hashslot, uint64_t cmd_flags, int *error_code); int clusterRedirectBlockedClientIfNeeded(client *c); void clusterRedirectClient(client *c, clusterNode *n, int hashslot, int error_code); void migrateCloseTimedoutSockets(void); -unsigned int keyHashSlot(char *key, int keylen); int patternHashSlot(char *pattern, int length); int isValidAuxString(char *s, unsigned int length); void migrateCommand(client *c); diff --git a/src/module.c b/src/module.c index 3920f1cff..a1fa6599c 100644 --- a/src/module.c +++ b/src/module.c @@ -6515,7 +6515,8 @@ RedisModuleCallReply *RM_Call(RedisModuleCtx *ctx, const char *cmdname, const ch /* Duplicate relevant flags in the module client. */ c->flags &= ~(CLIENT_READONLY|CLIENT_ASKING); c->flags |= ctx->client->flags & (CLIENT_READONLY|CLIENT_ASKING); - if (getNodeByQuery(c,c->cmd,c->argv,c->argc,NULL,&error_code) != + const uint64_t cmd_flags = getCommandFlags(c); + if (getNodeByQuery(c,c->cmd,c->argv,c->argc,NULL,cmd_flags,&error_code) != getMyClusterNode()) { sds msg = NULL; diff --git a/src/script.c b/src/script.c index a19304ab7..b2388d739 100644 --- a/src/script.c +++ b/src/script.c @@ -470,8 +470,9 @@ static int scriptVerifyClusterState(scriptRunCtx *run_ctx, client *c, client *or /* Duplicate relevant flags in the script client. */ c->flags &= ~(CLIENT_READONLY | CLIENT_ASKING); c->flags |= original_c->flags & (CLIENT_READONLY | CLIENT_ASKING); + const uint64_t cmd_flags = getCommandFlags(c); int hashslot = -1; - if (getNodeByQuery(c, c->cmd, c->argv, c->argc, &hashslot, &error_code) != getMyClusterNode()) { + if (getNodeByQuery(c, c->cmd, c->argv, c->argc, &hashslot, cmd_flags, &error_code) != getMyClusterNode()) { if (error_code == CLUSTER_REDIR_DOWN_RO_STATE) { *err = sdsnew( "Script attempted to execute a write command while the " diff --git a/src/server.c b/src/server.c index 56a17e32a..1a7bf2700 100644 --- a/src/server.c +++ b/src/server.c @@ -3949,7 +3949,7 @@ int processCommand(client *c) { } } - uint64_t cmd_flags = getCommandFlags(c); + const uint64_t cmd_flags = getCommandFlags(c); int is_read_command = (cmd_flags & CMD_READONLY) || (c->cmd->proc == execCommand && (c->mstate.cmd_flags & CMD_READONLY)); @@ -4004,7 +4004,7 @@ int processCommand(client *c) { { int error_code; clusterNode *n = getNodeByQuery(c,c->cmd,c->argv,c->argc, - &c->slot,&error_code); + &c->slot,cmd_flags,&error_code); if (n == NULL || !clusterNodeIsMyself(n)) { if (c->cmd->proc == execCommand) { discardTransaction(c);