From 9cb9f98d2f0d9114ac4528b2f9434a2fd2edfd60 Mon Sep 17 00:00:00 2001 From: Wang Yuan Date: Thu, 14 Jan 2021 04:36:03 +0800 Subject: [PATCH] Optimize performance of clusterGenNodesDescription for large clusters (#8182) Optimize the performance of clusterGenNodesDescription by only checking slot ownership of each slot once, instead of checking each slot for each node. --- src/cluster.c | 91 +++++++++++++++---- src/cluster.h | 1 + .../cluster/tests/18-cluster-nodes-slots.tcl | 62 +++++++++++++ 3 files changed, 134 insertions(+), 20 deletions(-) create mode 100644 tests/cluster/tests/18-cluster-nodes-slots.tcl diff --git a/src/cluster.c b/src/cluster.c index 78c36e8d1..2a06ef6dc 100644 --- a/src/cluster.c +++ b/src/cluster.c @@ -779,6 +779,7 @@ clusterNode *createClusterNode(char *nodename, int flags) { node->configEpoch = 0; node->flags = flags; memset(node->slots,0,sizeof(node->slots)); + node->slots_info = NULL; node->numslots = 0; node->numslaves = 0; node->slaves = NULL; @@ -4144,8 +4145,8 @@ sds clusterGenNodeDescription(clusterNode *node) { sds ci; /* Node coordinates */ - ci = sdscatprintf(sdsempty(),"%.40s %s:%d@%d ", - node->name, + ci = sdscatlen(sdsempty(),node->name,CLUSTER_NAMELEN); + ci = sdscatfmt(ci," %s:%i@%i ", node->ip, node->port, node->cport); @@ -4154,40 +4155,46 @@ sds clusterGenNodeDescription(clusterNode *node) { ci = representClusterNodeFlags(ci, node->flags); /* Slave of... or just "-" */ + ci = sdscatlen(ci," ",1); if (node->slaveof) - ci = sdscatprintf(ci," %.40s ",node->slaveof->name); + ci = sdscatlen(ci,node->slaveof->name,CLUSTER_NAMELEN); else - ci = sdscatlen(ci," - ",3); + ci = sdscatlen(ci,"-",1); unsigned long long nodeEpoch = node->configEpoch; if (nodeIsSlave(node) && node->slaveof) { nodeEpoch = node->slaveof->configEpoch; } /* Latency from the POV of this node, config epoch, link status */ - ci = sdscatprintf(ci,"%lld %lld %llu %s", + ci = sdscatfmt(ci," %I %I %U %s", (long long) node->ping_sent, (long long) node->pong_received, nodeEpoch, (node->link || node->flags & CLUSTER_NODE_MYSELF) ? "connected" : "disconnected"); - /* Slots served by this instance */ - start = -1; - for (j = 0; j < CLUSTER_SLOTS; j++) { - int bit; + /* Slots served by this instance. If we already have slots info, + * append it diretly, otherwise, generate slots only if it has. */ + if (node->slots_info) { + ci = sdscatsds(ci, node->slots_info); + } else if (node->numslots > 0) { + start = -1; + for (j = 0; j < CLUSTER_SLOTS; j++) { + int bit; - if ((bit = clusterNodeGetSlotBit(node,j)) != 0) { - if (start == -1) start = j; - } - if (start != -1 && (!bit || j == CLUSTER_SLOTS-1)) { - if (bit && j == CLUSTER_SLOTS-1) j++; - - if (start == j-1) { - ci = sdscatprintf(ci," %d",start); - } else { - ci = sdscatprintf(ci," %d-%d",start,j-1); + if ((bit = clusterNodeGetSlotBit(node,j)) != 0) { + if (start == -1) start = j; + } + if (start != -1 && (!bit || j == CLUSTER_SLOTS-1)) { + if (bit && j == CLUSTER_SLOTS-1) j++; + + if (start == j-1) { + ci = sdscatfmt(ci," %i",start); + } else { + ci = sdscatfmt(ci," %i-%i",start,j-1); + } + start = -1; } - start = -1; } } @@ -4208,6 +4215,41 @@ sds clusterGenNodeDescription(clusterNode *node) { return ci; } +/* Generate the slot topology for all nodes and store the string representation + * in the slots_info struct on the node. This is used to improve the efficiency + * of clusterGenNodesDescription() because it removes looping of the slot space + * for generating the slot info for each node individually. */ +void clusterGenNodesSlotsInfo(int filter) { + clusterNode *n = NULL; + int start = -1; + + for (int i = 0; i <= CLUSTER_SLOTS; i++) { + /* Find start node and slot id. */ + if (n == NULL) { + if (i == CLUSTER_SLOTS) break; + n = server.cluster->slots[i]; + start = i; + continue; + } + + /* Generate slots info when occur different node with start + * or end of slot. */ + if (i == CLUSTER_SLOTS || n != server.cluster->slots[i]) { + if (!(n->flags & filter)) { + if (n->slots_info == NULL) n->slots_info = sdsempty(); + if (start == i-1) { + n->slots_info = sdscatfmt(n->slots_info," %i",start); + } else { + n->slots_info = sdscatfmt(n->slots_info," %i-%i",start,i-1); + } + } + if (i == CLUSTER_SLOTS) break; + n = server.cluster->slots[i]; + start = i; + } + } +} + /* Generate a csv-alike representation of the nodes we are aware of, * including the "myself" node, and return an SDS string containing the * representation (it is up to the caller to free it). @@ -4225,6 +4267,9 @@ sds clusterGenNodesDescription(int filter) { dictIterator *di; dictEntry *de; + /* Generate all nodes slots info firstly. */ + clusterGenNodesSlotsInfo(filter); + di = dictGetSafeIterator(server.cluster->nodes); while((de = dictNext(di)) != NULL) { clusterNode *node = dictGetVal(de); @@ -4234,6 +4279,12 @@ sds clusterGenNodesDescription(int filter) { ci = sdscatsds(ci,ni); sdsfree(ni); ci = sdscatlen(ci,"\n",1); + + /* Release slots info. */ + if (node->slots_info) { + sdsfree(node->slots_info); + node->slots_info = NULL; + } } dictReleaseIterator(di); return ci; diff --git a/src/cluster.h b/src/cluster.h index d58f350ce..716c0d49c 100644 --- a/src/cluster.h +++ b/src/cluster.h @@ -118,6 +118,7 @@ typedef struct clusterNode { int flags; /* CLUSTER_NODE_... */ uint64_t configEpoch; /* Last configEpoch observed for this node */ unsigned char slots[CLUSTER_SLOTS/8]; /* slots handled by this node */ + sds slots_info; /* Slots info represented by string. */ int numslots; /* Number of slots handled by this node */ int numslaves; /* Number of slave nodes, if this is a master */ struct clusterNode **slaves; /* pointers to slave nodes */ diff --git a/tests/cluster/tests/18-cluster-nodes-slots.tcl b/tests/cluster/tests/18-cluster-nodes-slots.tcl new file mode 100644 index 000000000..ca0b3ce0d --- /dev/null +++ b/tests/cluster/tests/18-cluster-nodes-slots.tcl @@ -0,0 +1,62 @@ +# Optimize CLUSTER NODES command by generating all nodes slot topology firstly + +source "../tests/includes/init-tests.tcl" + +proc cluster_allocate_with_continuous_slots {n} { + set slot 16383 + set avg [expr ($slot+1) / $n] + while {$slot >= 0} { + set node [expr $slot/$avg >= $n ? $n-1 : $slot/$avg] + lappend slots_$node $slot + incr slot -1 + } + for {set j 0} {$j < $n} {incr j} { + R $j cluster addslots {*}[set slots_${j}] + } +} + +proc cluster_create_with_continuous_slots {masters slaves} { + cluster_allocate_with_continuous_slots $masters + if {$slaves} { + cluster_allocate_slaves $masters $slaves + } + assert_cluster_state ok +} + +test "Create a 2 nodes cluster" { + cluster_create_with_continuous_slots 2 2 +} + +test "Cluster should start ok" { + assert_cluster_state ok +} + +set master1 [Rn 0] +set master2 [Rn 1] + +test "Continuous slots distribution" { + assert_match "* 0-8191*" [$master1 CLUSTER NODES] + assert_match "* 8192-16383*" [$master2 CLUSTER NODES] + + $master1 CLUSTER DELSLOTS 4096 + assert_match "* 0-4095 4097-8191*" [$master1 CLUSTER NODES] + + $master2 CLUSTER DELSLOTS 12288 + assert_match "* 8192-12287 12289-16383*" [$master2 CLUSTER NODES] +} + +test "Discontinuous slots distribution" { + # Remove middle slots + $master1 CLUSTER DELSLOTS 4092 4094 + assert_match "* 0-4091 4093 4095 4097-8191*" [$master1 CLUSTER NODES] + $master2 CLUSTER DELSLOTS 12284 12286 + assert_match "* 8192-12283 12285 12287 12289-16383*" [$master2 CLUSTER NODES] + + # Remove head slots + $master1 CLUSTER DELSLOTS 0 2 + assert_match "* 1 3-4091 4093 4095 4097-8191*" [$master1 CLUSTER NODES] + + # Remove tail slots + $master2 CLUSTER DELSLOTS 16380 16382 16383 + assert_match "* 8192-12283 12285 12287 12289-16379 16381*" [$master2 CLUSTER NODES] +}