Merge 'Optimize topology::compare_endpoints' from Benny Halevy

The code for compare_endpoints originates at the dawn of time (locator: Convert AbstractNetworkTopologySnitch.java to C++)
and is called on the fast path from storage_proxy via `sort_by_proximity`.

This series considerably reduces the function's footprint by:
1. carefully coding the many comparisons in the function so to reduce the number of conditional banches (apparently the compiler isn't doing a good enough job at optimizing it in this case)
2. avoid sstring copy in topology::get_{datacenter,rack}

\Closes scylladb#12761
Refs scylladb#14008

Signed-off-by: Benny Halevy <bhalevy@scylladb.com>
(cherry picked from commit 6aa91c1)

locator/topology.cc

@@ -166,53 +166,26 @@ void topology::sort_by_proximity(inet_address address, inet_address_vector_repli
     }
 }
 
-int topology::compare_endpoints(const inet_address& address, const inet_address& a1, const inet_address& a2) const {
-    //
-    // if one of the Nodes IS the Node we are comparing to and the other one
-    // IS NOT - then return the appropriate result.
-    //
-    if (address == a1 && address != a2) {
-        return -1;
-    }
-
-    if (address == a2 && address != a1) {
-        return 1;
-    }
-
-    // ...otherwise perform the similar check in regard to Data Center
-    sstring address_datacenter = get_datacenter(address);
-    sstring a1_datacenter = get_datacenter(a1);
-    sstring a2_datacenter = get_datacenter(a2);
-
-    if (address_datacenter == a1_datacenter &&
-        address_datacenter != a2_datacenter) {
-        return -1;
-    } else if (address_datacenter == a2_datacenter &&
-               address_datacenter != a1_datacenter) {
-        return 1;
-    } else if (address_datacenter == a2_datacenter &&
-               address_datacenter == a1_datacenter) {
-        //
-        // ...otherwise (in case Nodes belong to the same Data Center) check
-        // the racks they belong to.
-        //
-        sstring address_rack = get_rack(address);
-        sstring a1_rack = get_rack(a1);
-        sstring a2_rack = get_rack(a2);
-
-        if (address_rack == a1_rack && address_rack != a2_rack) {
-            return -1;
-        }
-
-        if (address_rack == a2_rack && address_rack != a1_rack) {
-            return 1;
-        }
-    }
-    //
-    // We don't differentiate between Nodes if all Nodes belong to different
-    // Data Centers, thus make them equal.
-    //
-    return 0;
+std::weak_ordering topology::compare_endpoints(const inet_address& address, const inet_address& a1, const inet_address& a2) const {
+    const auto& loc = get_location(address);
+    const auto& loc1 = get_location(a1);
+    const auto& loc2 = get_location(a2);
+
+    // The farthest nodes from a given node are:
+    // 1. Nodes in other DCs then the reference node
+    // 2. Nodes in the other RACKs in the same DC as the reference node
+    // 3. Other nodes in the same DC/RACk as the reference node
+    int same_dc1 = loc1.dc == loc.dc;
+    int same_rack1 = same_dc1 & (loc1.rack == loc.rack);
+    int same_node1 = a1 == address;
+    int d1 = ((same_dc1 << 2) | (same_rack1 << 1) | same_node1) ^ 7;
+
+    int same_dc2 = loc2.dc == loc.dc;
+    int same_rack2 = same_dc2 & (loc2.rack == loc.rack);
+    int same_node2 = a2 == address;
+    int d2 = ((same_dc2 << 2) | (same_rack2 << 1) | same_node2) ^ 7;
+
+    return d1 <=> d2;
 }
 
 } // namespace locator

locator/topology.hh

@@ -12,6 +12,7 @@
 
 #include <unordered_set>
 #include <unordered_map>
+#include <compare>
 
 #include <seastar/core/future.hh>
 #include <seastar/core/sstring.hh>
@@ -100,8 +101,13 @@ private:
     /**
      * compares two endpoints in relation to the target endpoint, returning as
      * Comparator.compare would
+     *
+     * The closest nodes to a given node are:
+     * 1. The node itself
+     * 2. Nodes in the same RACK as the reference node
+     * 3. Nodes in the same DC as the reference node
      */
-    int compare_endpoints(const inet_address& address, const inet_address& a1, const inet_address& a2) const;
+    std::weak_ordering compare_endpoints(const inet_address& address, const inet_address& a1, const inet_address& a2) const;
 
     /** multi-map: DC -> endpoints in that DC */
     std::unordered_map<sstring,
@@ -123,6 +129,9 @@ private:
     std::unordered_set<sstring> _datacenters;
 
     void calculate_datacenters();
+
+public:
+    void test_compare_endpoints(const inet_address& address, const inet_address& a1, const inet_address& a2) const;
 };
 
 } // namespace locator

test/boost/network_topology_strategy_test.cc

@@ -631,3 +631,92 @@ SEASTAR_TEST_CASE(test_invalid_dcs) {
     });
 }
 
+namespace locator {
+
+void topology::test_compare_endpoints(const inet_address& address, const inet_address& a1, const inet_address& a2) const {
+    std::optional<int> expected;
+    const auto& loc = get_location(address);
+    const auto& loc1 = get_location(a1);
+    const auto& loc2 = get_location(a2);
+    if (a1 == a2) {
+        expected = 0;
+    } else {
+        if (a1 == address) {
+            expected = -1;
+        } else if (a2 == address) {
+            expected = 1;
+        } else {
+            if (loc1.dc == loc.dc) {
+                if (loc2.dc != loc.dc) {
+                    expected = -1;
+                } else {
+                    if (loc1.rack == loc.rack) {
+                        if (loc2.rack != loc.rack) {
+                            expected = -1;
+                        }
+                    } else if (loc2.rack == loc.rack) {
+                        expected = 1;
+                    }
+                }
+            } else if (loc2.dc == loc.dc) {
+                expected = 1;
+            }
+        }
+    }
+    auto order = compare_endpoints(address, a1, a2);
+    auto res = order < 0 ? -1 : (order > 0);
+    testlog.debug("compare_endpoint: address={} [{}/{}] a1={} [{}/{}] a2={} [{}/{}]: res={} expected={} expected_value={}",
+            address, loc.dc, loc.rack,
+            a1, loc1.dc, loc1.rack,
+            a2, loc2.dc, loc2.rack,
+            res, bool(expected), expected.value_or(0));
+    if (expected) {
+        BOOST_REQUIRE_EQUAL(res, *expected);
+    }
+}
+
+} // namespace locator
+
+SEASTAR_THREAD_TEST_CASE(test_topology_compare_endpoints) {
+    utils::fb_utilities::set_broadcast_address(gms::inet_address("localhost"));
+    utils::fb_utilities::set_broadcast_rpc_address(gms::inet_address("localhost"));
+
+    constexpr size_t NODES = 10;
+
+    std::unordered_map<sstring, size_t> datacenters = {
+                    { "rf1", 1 },
+                    { "rf2", 2 },
+                    { "rf3", 3 },
+    };
+    std::vector<inet_address> nodes;
+    nodes.reserve(NODES);
+
+    auto make_address = [] (unsigned i) {
+        return inet_address((127u << 24) | i);
+    };
+
+    std::generate_n(std::back_inserter(nodes), NODES, [&, i = 0u]() mutable {
+        return make_address(++i);
+    });
+    auto bogus_address = make_address(NODES + 1);
+
+    semaphore sem(1);
+    shared_token_metadata stm([&sem] () noexcept { return get_units(sem, 1); }, locator::token_metadata::config{});
+    auto topo = generate_topology(datacenters, nodes);
+
+    const auto& address = nodes[tests::random::get_int<size_t>(0, NODES-1)];
+    const auto& a1 = nodes[tests::random::get_int<size_t>(0, NODES-1)];
+    const auto& a2 = nodes[tests::random::get_int<size_t>(0, NODES-1)];
+
+    topo->test_compare_endpoints(address, address, address);
+    topo->test_compare_endpoints(address, address, a1);
+    topo->test_compare_endpoints(address, a1, address);
+    topo->test_compare_endpoints(address, a1, a1);
+    topo->test_compare_endpoints(address, a1, a2);
+    topo->test_compare_endpoints(address, a2, a1);
+
+    topo->test_compare_endpoints(bogus_address, bogus_address, bogus_address);
+    topo->test_compare_endpoints(address, bogus_address, bogus_address);
+    topo->test_compare_endpoints(address, a1, bogus_address);
+    topo->test_compare_endpoints(address, bogus_address, a2);
+}