Unverified Commit 31227f4f authored by Filipe Oliveira (Redis)'s avatar Filipe Oliveira (Redis) Committed by GitHub
Browse files

Optimize client type check on reply hot code paths (#13516)

## Proposed improvement

This PR introduces the static inlined function `clientTypeIsSlave` which
is doing only 1 condition check vs 3 checks of `getClientType`, and also
uses the `unlikely` to tell the compiler that the most common outcome is
for the client not to be a slave.
Preliminary data show 3% improvement on the achievable ops/sec on the
specific LRANGE benchmark. After running the entire suite we see up to
5% improvement in 2 tests.
https://github.com/redis/redis/pull/13516#issuecomment-2331326052

## Context

This optimization efforts comes from analyzing the profile info from the
[memtier_benchmark-1key-list-1K-elements-lrange-all-elements](https://github.com/redis/redis-benchmarks-specification/blob/main/redis_benchmarks_specification/test-suites/memtier_benchmark-1key-list-1K-elements-lrange-all-elements.yml)
benchmark.
 
By going over it, we can see that `getClientType` consumes 2% of the cpu
time, strictly to check if the client is a slave (
https://github.com/redis/redis/blob/unstable/src/networking.c#L397 , and
https://github.com/redis/redis/blob/unstable/src/networking.c#L1254

 )


Function | CPU Time: Total | CPU Time: Self | Module | Function (Full)
-- | -- | -- | -- | --
_addReplyToBufferOrList->getClientType | 1.20% | 0.728s | redis-server |
getClientType
clientHasPendingReplies->getClientType | 0.80% | 0.482s | redis-server |
getClientType

---------
Co-authored-by: default avatardebing.sun <debing.sun@redis.com>
parent f6f11f3e
...@@ -26,6 +26,7 @@ static void setProtocolError(const char *errstr, client *c); ...@@ -26,6 +26,7 @@ static void setProtocolError(const char *errstr, client *c);
static void pauseClientsByClient(mstime_t end, int isPauseClientAll); static void pauseClientsByClient(mstime_t end, int isPauseClientAll);
int postponeClientRead(client *c); int postponeClientRead(client *c);
char *getClientSockname(client *c); char *getClientSockname(client *c);
static inline int clientTypeIsSlave(client *c);
int ProcessingEventsWhileBlocked = 0; /* See processEventsWhileBlocked(). */ int ProcessingEventsWhileBlocked = 0; /* See processEventsWhileBlocked(). */
__thread sds thread_reusable_qb = NULL; __thread sds thread_reusable_qb = NULL;
__thread int thread_reusable_qb_used = 0; /* Avoid multiple clients using reusable query __thread int thread_reusable_qb_used = 0; /* Avoid multiple clients using reusable query
...@@ -394,7 +395,7 @@ void _addReplyToBufferOrList(client *c, const char *s, size_t len) { ...@@ -394,7 +395,7 @@ void _addReplyToBufferOrList(client *c, const char *s, size_t len) {
* replication link that caused a reply to be generated we'll simply disconnect it. * replication link that caused a reply to be generated we'll simply disconnect it.
* Note this is the simplest way to check a command added a response. Replication links are used to write data but * Note this is the simplest way to check a command added a response. Replication links are used to write data but
* not for responses, so we should normally never get here on a replica client. */ * not for responses, so we should normally never get here on a replica client. */
if (getClientType(c) == CLIENT_TYPE_SLAVE) { if (unlikely(clientTypeIsSlave(c))) {
sds cmdname = c->lastcmd ? c->lastcmd->fullname : NULL; sds cmdname = c->lastcmd ? c->lastcmd->fullname : NULL;
logInvalidUseAndFreeClientAsync(c, "Replica generated a reply to command '%s'", logInvalidUseAndFreeClientAsync(c, "Replica generated a reply to command '%s'",
cmdname ? cmdname : "<unknown>"); cmdname ? cmdname : "<unknown>");
...@@ -736,7 +737,7 @@ void *addReplyDeferredLen(client *c) { ...@@ -736,7 +737,7 @@ void *addReplyDeferredLen(client *c) {
* replication link that caused a reply to be generated we'll simply disconnect it. * replication link that caused a reply to be generated we'll simply disconnect it.
* Note this is the simplest way to check a command added a response. Replication links are used to write data but * Note this is the simplest way to check a command added a response. Replication links are used to write data but
* not for responses, so we should normally never get here on a replica client. */ * not for responses, so we should normally never get here on a replica client. */
if (getClientType(c) == CLIENT_TYPE_SLAVE) { if (unlikely(clientTypeIsSlave(c))) {
sds cmdname = c->lastcmd ? c->lastcmd->fullname : NULL; sds cmdname = c->lastcmd ? c->lastcmd->fullname : NULL;
logInvalidUseAndFreeClientAsync(c, "Replica generated a reply to command '%s'", logInvalidUseAndFreeClientAsync(c, "Replica generated a reply to command '%s'",
cmdname ? cmdname : "<unknown>"); cmdname ? cmdname : "<unknown>");
...@@ -1251,7 +1252,7 @@ void copyReplicaOutputBuffer(client *dst, client *src) { ...@@ -1251,7 +1252,7 @@ void copyReplicaOutputBuffer(client *dst, client *src) {
/* Return true if the specified client has pending reply buffers to write to /* Return true if the specified client has pending reply buffers to write to
* the socket. */ * the socket. */
int clientHasPendingReplies(client *c) { int clientHasPendingReplies(client *c) {
if (getClientType(c) == CLIENT_TYPE_SLAVE) { if (unlikely(clientTypeIsSlave(c))) {
/* Replicas use global shared replication buffer instead of /* Replicas use global shared replication buffer instead of
* private output buffer. */ * private output buffer. */
serverAssert(c->bufpos == 0 && listLength(c->reply) == 0); serverAssert(c->bufpos == 0 && listLength(c->reply) == 0);
...@@ -1654,7 +1655,7 @@ void freeClient(client *c) { ...@@ -1654,7 +1655,7 @@ void freeClient(client *c) {
} }
/* Log link disconnection with slave */ /* Log link disconnection with slave */
if (getClientType(c) == CLIENT_TYPE_SLAVE) { if (clientTypeIsSlave(c)) {
serverLog(LL_NOTICE,"Connection with replica %s lost.", serverLog(LL_NOTICE,"Connection with replica %s lost.",
replicationGetSlaveName(c)); replicationGetSlaveName(c));
} }
...@@ -1736,7 +1737,7 @@ void freeClient(client *c) { ...@@ -1736,7 +1737,7 @@ void freeClient(client *c) {
/* We need to remember the time when we started to have zero /* We need to remember the time when we started to have zero
* attached slaves, as after some time we'll free the replication * attached slaves, as after some time we'll free the replication
* backlog. */ * backlog. */
if (getClientType(c) == CLIENT_TYPE_SLAVE && listLength(server.slaves) == 0) if (clientTypeIsSlave(c) && listLength(server.slaves) == 0)
server.repl_no_slaves_since = server.unixtime; server.repl_no_slaves_since = server.unixtime;
refreshGoodSlavesCount(); refreshGoodSlavesCount();
/* Fire the replica change modules event. */ /* Fire the replica change modules event. */
...@@ -1949,7 +1950,7 @@ static int _writevToClient(client *c, ssize_t *nwritten) { ...@@ -1949,7 +1950,7 @@ static int _writevToClient(client *c, ssize_t *nwritten) {
* to client. */ * to client. */
int _writeToClient(client *c, ssize_t *nwritten) { int _writeToClient(client *c, ssize_t *nwritten) {
*nwritten = 0; *nwritten = 0;
if (getClientType(c) == CLIENT_TYPE_SLAVE) { if (unlikely(clientTypeIsSlave(c))) {
serverAssert(c->bufpos == 0 && listLength(c->reply) == 0); serverAssert(c->bufpos == 0 && listLength(c->reply) == 0);
replBufBlock *o = listNodeValue(c->ref_repl_buf_node); replBufBlock *o = listNodeValue(c->ref_repl_buf_node);
...@@ -2036,7 +2037,7 @@ int writeToClient(client *c, int handler_installed) { ...@@ -2036,7 +2037,7 @@ int writeToClient(client *c, int handler_installed) {
!(c->flags & CLIENT_SLAVE)) break; !(c->flags & CLIENT_SLAVE)) break;
} }
if (getClientType(c) == CLIENT_TYPE_SLAVE) { if (unlikely(clientTypeIsSlave(c))) {
atomicIncr(server.stat_net_repl_output_bytes, totwritten); atomicIncr(server.stat_net_repl_output_bytes, totwritten);
} else { } else {
atomicIncr(server.stat_net_output_bytes, totwritten); atomicIncr(server.stat_net_output_bytes, totwritten);
...@@ -2240,7 +2241,7 @@ int processInlineBuffer(client *c) { ...@@ -2240,7 +2241,7 @@ int processInlineBuffer(client *c) {
/* Newline from slaves can be used to refresh the last ACK time. /* Newline from slaves can be used to refresh the last ACK time.
* This is useful for a slave to ping back while loading a big * This is useful for a slave to ping back while loading a big
* RDB file. */ * RDB file. */
if (querylen == 0 && getClientType(c) == CLIENT_TYPE_SLAVE) if (querylen == 0 && clientTypeIsSlave(c))
c->repl_ack_time = server.unixtime; c->repl_ack_time = server.unixtime;
/* Masters should never send us inline protocol to run actual /* Masters should never send us inline protocol to run actual
...@@ -3897,7 +3898,7 @@ void rewriteClientCommandArgument(client *c, int i, robj *newval) { ...@@ -3897,7 +3898,7 @@ void rewriteClientCommandArgument(client *c, int i, robj *newval) {
* the caller wishes. The main usage of this function currently is * the caller wishes. The main usage of this function currently is
* enforcing the client output length limits. */ * enforcing the client output length limits. */
size_t getClientOutputBufferMemoryUsage(client *c) { size_t getClientOutputBufferMemoryUsage(client *c) {
if (getClientType(c) == CLIENT_TYPE_SLAVE) { if (unlikely(clientTypeIsSlave(c))) {
size_t repl_buf_size = 0; size_t repl_buf_size = 0;
size_t repl_node_num = 0; size_t repl_node_num = 0;
size_t repl_node_size = sizeof(listNode) + sizeof(replBufBlock); size_t repl_node_size = sizeof(listNode) + sizeof(replBufBlock);
...@@ -3961,6 +3962,12 @@ int getClientType(client *c) { ...@@ -3961,6 +3962,12 @@ int getClientType(client *c) {
return CLIENT_TYPE_NORMAL; return CLIENT_TYPE_NORMAL;
} }
static inline int clientTypeIsSlave(client *c) {
if (unlikely((c->flags & CLIENT_SLAVE) && !(c->flags & CLIENT_MONITOR)))
return 1;
return 0;
}
int getClientTypeByName(char *name) { int getClientTypeByName(char *name) {
if (!strcasecmp(name,"normal")) return CLIENT_TYPE_NORMAL; if (!strcasecmp(name,"normal")) return CLIENT_TYPE_NORMAL;
else if (!strcasecmp(name,"slave")) return CLIENT_TYPE_SLAVE; else if (!strcasecmp(name,"slave")) return CLIENT_TYPE_SLAVE;
...@@ -4050,7 +4057,7 @@ int closeClientOnOutputBufferLimitReached(client *c, int async) { ...@@ -4050,7 +4057,7 @@ int closeClientOnOutputBufferLimitReached(client *c, int async) {
serverAssert(c->reply_bytes < SIZE_MAX-(1024*64)); serverAssert(c->reply_bytes < SIZE_MAX-(1024*64));
/* Note that c->reply_bytes is irrelevant for replica clients /* Note that c->reply_bytes is irrelevant for replica clients
* (they use the global repl buffers). */ * (they use the global repl buffers). */
if ((c->reply_bytes == 0 && getClientType(c) != CLIENT_TYPE_SLAVE) || if ((c->reply_bytes == 0 && !clientTypeIsSlave(c)) ||
c->flags & CLIENT_CLOSE_ASAP) return 0; c->flags & CLIENT_CLOSE_ASAP) return 0;
if (checkClientOutputBufferLimits(c)) { if (checkClientOutputBufferLimits(c)) {
sds client = catClientInfoString(sdsempty(),c); sds client = catClientInfoString(sdsempty(),c);
...@@ -4480,7 +4487,7 @@ int handleClientsWithPendingWritesUsingThreads(void) { ...@@ -4480,7 +4487,7 @@ int handleClientsWithPendingWritesUsingThreads(void) {
* buffer, to guarantee data accessing thread safe, we must put all * buffer, to guarantee data accessing thread safe, we must put all
* replicas client into io_threads_list[0] i.e. main thread handles * replicas client into io_threads_list[0] i.e. main thread handles
* sending the output buffer of all replicas. */ * sending the output buffer of all replicas. */
if (getClientType(c) == CLIENT_TYPE_SLAVE) { if (unlikely(clientTypeIsSlave(c))) {
listAddNodeTail(io_threads_list[0],c); listAddNodeTail(io_threads_list[0],c);
continue; continue;
} }
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment