From 239b08574cab1757733d037edf717ce1867ffea2 Mon Sep 17 00:00:00 2001 From: "zhaozhao.zz" Date: Thu, 30 Nov 2017 18:11:05 +0800 Subject: [PATCH 1/2] networking: optimize unlinkClient() in freeClient() --- src/networking.c | 14 ++++++++++---- src/replication.c | 1 + src/server.h | 1 + 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/networking.c b/src/networking.c index 2cab29d4..0dfc0767 100644 --- a/src/networking.c +++ b/src/networking.c @@ -135,7 +135,12 @@ client *createClient(int fd) { c->peerid = NULL; listSetFreeMethod(c->pubsub_patterns,decrRefCountVoid); listSetMatchMethod(c->pubsub_patterns,listMatchObjects); - if (fd != -1) listAddNodeTail(server.clients,c); + if (fd != -1) { + listAddNodeTail(server.clients,c); + c->client_list_node = listLast(server.clients); + } else { + c->client_list_node = NULL; + } initClientMultiState(c); return c; } @@ -752,9 +757,10 @@ void unlinkClient(client *c) { * fd is already set to -1. */ if (c->fd != -1) { /* Remove from the list of active clients. */ - ln = listSearchKey(server.clients,c); - serverAssert(ln != NULL); - listDelNode(server.clients,ln); + if (c->client_list_node) { + listDelNode(server.clients,c->client_list_node); + c->client_list_node = NULL; + } /* Unregister async I/O handlers and close the socket. */ aeDeleteFileEvent(server.el,c->fd,AE_READABLE); diff --git a/src/replication.c b/src/replication.c index 3b4059c3..773d6e70 100644 --- a/src/replication.c +++ b/src/replication.c @@ -2215,6 +2215,7 @@ void replicationResurrectCachedMaster(int newfd) { /* Re-add to the list of clients. */ listAddNodeTail(server.clients,server.master); + server.master->client_list_node = listLast(server.clients); if (aeCreateFileEvent(server.el, newfd, AE_READABLE, readQueryFromClient, server.master)) { serverLog(LL_WARNING,"Error resurrecting the cached master, impossible to add the readable handler: %s", strerror(errno)); diff --git a/src/server.h b/src/server.h index 5abb8bdc..2100d707 100644 --- a/src/server.h +++ b/src/server.h @@ -723,6 +723,7 @@ typedef struct client { dict *pubsub_channels; /* channels a client is interested in (SUBSCRIBE) */ list *pubsub_patterns; /* patterns a client is interested in (SUBSCRIBE) */ sds peerid; /* Cached peer ID. */ + listNode *client_list_node; /* list node in client list */ /* Response buffer */ int bufpos; From 1908aba73d88346ecbdd44fe5322c61813520c54 Mon Sep 17 00:00:00 2001 From: antirez Date: Tue, 5 Dec 2017 15:59:56 +0100 Subject: [PATCH 2/2] add linkClient(): adds the client and caches the list node. We have this operation in two places: when caching the master and when linking a new client after the client creation. By having an API for this we avoid incurring in errors when modifying one of the two places forgetting the other. The function is also a good place where to document why we cache the linked list node. Related to #4497 and #4210. --- src/networking.c | 18 ++++++++++++------ src/replication.c | 3 +-- src/server.h | 1 + 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/networking.c b/src/networking.c index 0dfc0767..82c763c9 100644 --- a/src/networking.c +++ b/src/networking.c @@ -67,6 +67,16 @@ int listMatchObjects(void *a, void *b) { return equalStringObjects(a,b); } +/* This function links the client to the global linked list of clients. + * unlinkClient() does the opposite, among other things. */ +void linkClient(client *c) { + listAddNodeTail(server.clients,c); + /* Note that we remember the linked list node where the client is stored, + * this way removing the client in unlinkClient() will not require + * a linear scan, but just a constant time operation. */ + c->client_list_node = listLast(server.clients); +} + client *createClient(int fd) { client *c = zmalloc(sizeof(client)); @@ -133,14 +143,10 @@ client *createClient(int fd) { c->pubsub_channels = dictCreate(&objectKeyPointerValueDictType,NULL); c->pubsub_patterns = listCreate(); c->peerid = NULL; + c->client_list_node = NULL; listSetFreeMethod(c->pubsub_patterns,decrRefCountVoid); listSetMatchMethod(c->pubsub_patterns,listMatchObjects); - if (fd != -1) { - listAddNodeTail(server.clients,c); - c->client_list_node = listLast(server.clients); - } else { - c->client_list_node = NULL; - } + if (fd != -1) linkClient(c); initClientMultiState(c); return c; } diff --git a/src/replication.c b/src/replication.c index 773d6e70..17a8bb35 100644 --- a/src/replication.c +++ b/src/replication.c @@ -2214,8 +2214,7 @@ void replicationResurrectCachedMaster(int newfd) { server.repl_down_since = 0; /* Re-add to the list of clients. */ - listAddNodeTail(server.clients,server.master); - server.master->client_list_node = listLast(server.clients); + linkClient(server.master); if (aeCreateFileEvent(server.el, newfd, AE_READABLE, readQueryFromClient, server.master)) { serverLog(LL_WARNING,"Error resurrecting the cached master, impossible to add the readable handler: %s", strerror(errno)); diff --git a/src/server.h b/src/server.h index 2100d707..7b0d22ea 100644 --- a/src/server.h +++ b/src/server.h @@ -1394,6 +1394,7 @@ int handleClientsWithPendingWrites(void); int clientHasPendingReplies(client *c); void unlinkClient(client *c); int writeToClient(int fd, client *c, int handler_installed); +void linkClient(client *c); #ifdef __GNUC__ void addReplyErrorFormat(client *c, const char *fmt, ...)