Optimize CLUSTER SLOTS reply by reducing unneeded loops (#8541)

This commit more efficiently computes the cluster bulk slots response 
by looping over the entire slot space once, instead of for each node.

src/cluster.c

@@ -4313,6 +4313,30 @@ int getSlotOrReply(client *c, robj *o) {
     return (int) slot;
 }
 
+void addNodeReplyForClusterSlot(client *c, clusterNode *node, int start_slot, int end_slot) {
+    int i, nested_elements = 3; /* slots (2) + master addr (1) */
+    void *nested_replylen = addReplyDeferredLen(c);
+    addReplyLongLong(c, start_slot);
+    addReplyLongLong(c, end_slot);
+    addReplyArrayLen(c, 3);
+    addReplyBulkCString(c, node->ip);
+    addReplyLongLong(c, node->port);
+    addReplyBulkCBuffer(c, node->name, CLUSTER_NAMELEN);
+
+    /* Remaining nodes in reply are replicas for slot range */
+    for (i = 0; i < node->numslaves; i++) {
+        /* This loop is copy/pasted from clusterGenNodeDescription()
+         * with modifications for per-slot node aggregation. */
+        if (nodeFailed(node->slaves[i])) continue;
+        addReplyArrayLen(c, 3);
+        addReplyBulkCString(c, node->slaves[i]->ip);
+        addReplyLongLong(c, node->slaves[i]->port);
+        addReplyBulkCBuffer(c, node->slaves[i]->name, CLUSTER_NAMELEN);
+        nested_elements++;
+    }
+    setDeferredArrayLen(c, nested_replylen, nested_elements);
+}
+
 void clusterReplyMultiBulkSlots(client * c) {
     /* Format: 1) 1) start slot
      *            2) end slot
@@ -4324,69 +4348,29 @@ void clusterReplyMultiBulkSlots(client *c) {
      *               3) node ID
      *           ... continued until done
      */
-
+    clusterNode *n = NULL;
-    int num_masters = 0;
+    int num_masters = 0, start = -1;
     void *slot_replylen = addReplyDeferredLen(c);
 
-    dictEntry *de;
+    for (int i = 0; i <= CLUSTER_SLOTS; i++) {
-    dictIterator *di = dictGetSafeIterator(server.cluster->nodes);
+        /* Find start node and slot id. */
-    while((de = dictNext(di)) != NULL) {
+        if (n == NULL) {
-        clusterNode *node = dictGetVal(de);
+            if (i == CLUSTER_SLOTS) break;
-        int j = 0, start = -1;
+            n = server.cluster->slots[i];
-        int i, nested_elements = 0;
+            start = i;
-
+            continue;
-        /* Skip slaves (that are iterated when producing the output of their
-         * master) and  masters not serving any slot. */
-        if (!nodeIsMaster(node) || node->numslots == 0) continue;
-
-        for(i = 0; i < node->numslaves; i++) {
-            if (nodeFailed(node->slaves[i])) continue;
-            nested_elements++;
         }
 
-        for (j = 0; j < CLUSTER_SLOTS; j++) {
+        /* Add cluster slots info when occur different node with start
-            int bit, i;
+         * or end of slot. */
-
+        if (i == CLUSTER_SLOTS || n != server.cluster->slots[i]) {
-            if ((bit = clusterNodeGetSlotBit(node,j)) != 0) {
+            addNodeReplyForClusterSlot(c, n, start, i-1);
-                if (start == -1) start = j;
-            }
-            if (start != -1 && (!bit || j == CLUSTER_SLOTS-1)) {
-                addReplyArrayLen(c, nested_elements + 3); /* slots (2) + master addr (1). */
-
-                if (bit && j == CLUSTER_SLOTS-1) j++;
-
-                /* If slot exists in output map, add to it's list.
-                 * else, create a new output map for this slot */
-                if (start == j-1) {
-                    addReplyLongLong(c, start); /* only one slot; low==high */
-                    addReplyLongLong(c, start);
-                } else {
-                    addReplyLongLong(c, start); /* low */
-                    addReplyLongLong(c, j-1);   /* high */
-                }
-                start = -1;
-
-                /* First node reply position is always the master */
-                addReplyArrayLen(c, 3);
-                addReplyBulkCString(c, node->ip);
-                addReplyLongLong(c, node->port);
-                addReplyBulkCBuffer(c, node->name, CLUSTER_NAMELEN);
-
-                /* Remaining nodes in reply are replicas for slot range */
-                for (i = 0; i < node->numslaves; i++) {
-                    /* This loop is copy/pasted from clusterGenNodeDescription()
-                     * with modifications for per-slot node aggregation */
-                    if (nodeFailed(node->slaves[i])) continue;
-                    addReplyArrayLen(c, 3);
-                    addReplyBulkCString(c, node->slaves[i]->ip);
-                    addReplyLongLong(c, node->slaves[i]->port);
-                    addReplyBulkCBuffer(c, node->slaves[i]->name, CLUSTER_NAMELEN);
-                }
             num_masters++;
+            if (i == CLUSTER_SLOTS) break;
+            n = server.cluster->slots[i];
+            start = i;
         }
     }
-    }
-    dictReleaseIterator(di);
     setDeferredArrayLen(c, slot_replylen, num_masters);
 }
 

tests/cluster/tests/19-cluster-nodes-slots.tcl

@@ -37,26 +37,35 @@ set master2 [Rn 1]
 test "Continuous slots distribution" {
     assert_match "* 0-8191*" [$master1 CLUSTER NODES]
     assert_match "* 8192-16383*" [$master2 CLUSTER NODES]
+    assert_match "*0 8191*" [$master1 CLUSTER SLOTS]
+    assert_match "*8192 16383*" [$master2 CLUSTER SLOTS]
 
     $master1 CLUSTER DELSLOTS 4096
     assert_match "* 0-4095 4097-8191*" [$master1 CLUSTER NODES]
+    assert_match "*0 4095*4097 8191*" [$master1 CLUSTER SLOTS]
+
 
     $master2 CLUSTER DELSLOTS 12288
     assert_match "* 8192-12287 12289-16383*" [$master2 CLUSTER NODES]
+    assert_match "*8192 12287*12289 16383*" [$master2 CLUSTER SLOTS]
 }
 
 test "Discontinuous slots distribution" {
     # Remove middle slots
     $master1 CLUSTER DELSLOTS 4092 4094
     assert_match "* 0-4091 4093 4095 4097-8191*" [$master1 CLUSTER NODES]
+    assert_match "*0 4091*4093 4093*4095 4095*4097 8191*" [$master1 CLUSTER SLOTS]
     $master2 CLUSTER DELSLOTS 12284 12286
     assert_match "* 8192-12283 12285 12287 12289-16383*" [$master2 CLUSTER NODES]
+    assert_match "*8192 12283*12285 12285*12287 12287*12289 16383*" [$master2 CLUSTER SLOTS]
 
     # Remove head slots
     $master1 CLUSTER DELSLOTS 0 2
     assert_match "* 1 3-4091 4093 4095 4097-8191*" [$master1 CLUSTER NODES]
+    assert_match "*1 1*3 4091*4093 4093*4095 4095*4097 8191*" [$master1 CLUSTER SLOTS]
 
     # Remove tail slots
     $master2 CLUSTER DELSLOTS 16380 16382 16383
     assert_match "* 8192-12283 12285 12287 12289-16379 16381*" [$master2 CLUSTER NODES]
+    assert_match "*8192 12283*12285 12285*12287 12287*12289 16379*16381 16381*" [$master2 CLUSTER SLOTS]
 }