From bb36237cf4b56385614eb51bfe1038b6816ff7a3 Mon Sep 17 00:00:00 2001 From: Benny Halevy Date: Tue, 7 Feb 2023 11:31:21 +0200 Subject: [PATCH] topology: optimize compare_endpoints This function is called on the fast data path from storage_proxy when sorting multiple endpoints by proximity. This change calculates numeric node diff metrics based on each address proximity to a given node (by ) to eliminate logic branches in the function and reduce its footprint. based on objdump -d output, compare_endpoints footprint was reduced by 58.5% (3632 / 8752 bytes) with clang version 15.0.7 (Fedora 15.0.7-1.fc37) Signed-off-by: Benny Halevy --- locator/topology.cc | 61 ++++++-------------- locator/topology.hh | 3 +- test/boost/network_topology_strategy_test.cc | 19 +++--- 3 files changed, 29 insertions(+), 54 deletions(-) diff --git a/locator/topology.cc b/locator/topology.cc index 81b044f61a..90676aa71a 100644 --- a/locator/topology.cc +++ b/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; - } +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); - if (address == a2 && address != a1) { - return 1; - } + // 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; - // ...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); + 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; - 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; + return d1 <=> d2; } } // namespace locator diff --git a/locator/topology.hh b/locator/topology.hh index 3ac3bf1cd6..9f315ca320 100644 --- a/locator/topology.hh +++ b/locator/topology.hh @@ -12,6 +12,7 @@ #include #include +#include #include #include @@ -106,7 +107,7 @@ private: * 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 #include #include +#include #include #include #include "test/lib/log.hh" @@ -634,32 +635,32 @@ 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 expected; + std::optional expected; const auto& loc = get_location(address); const auto& loc1 = get_location(a1); const auto& loc2 = get_location(a2); if (a1 == a2) { - expected = 0; + expected = std::partial_ordering::equivalent; } else { if (a1 == address) { - expected = -1; + expected = std::partial_ordering::less; } else if (a2 == address) { - expected = 1; + expected = std::partial_ordering::greater; } else { if (loc1.dc == loc.dc) { if (loc2.dc != loc.dc) { - expected = -1; + expected = std::partial_ordering::less; } else { if (loc1.rack == loc.rack) { if (loc2.rack != loc.rack) { - expected = -1; + expected = std::partial_ordering::less; } } else if (loc2.rack == loc.rack) { - expected = 1; + expected = std::partial_ordering::greater; } } } else if (loc2.dc == loc.dc) { - expected = 1; + expected = std::partial_ordering::greater; } } } @@ -668,7 +669,7 @@ void topology::test_compare_endpoints(const inet_address& address, const inet_ad address, loc.dc, loc.rack, a1, loc1.dc, loc1.rack, a2, loc2.dc, loc2.rack, - res, bool(expected), expected.value_or(0)); + res, bool(expected), expected.value_or(std::partial_ordering::unordered)); if (expected) { BOOST_REQUIRE_EQUAL(res, *expected); }