Merge 'Optimize topology::compare_endpoints' from Benny Halevy

The code for compare_endpoints originates at the dawn of time (bc034ae) 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 * github.com:scylladb/scylladb: topology: optimize compare_endpoints to_string: add print operators for std::{weak,partial}_ordering utils: to_sstring: deinline std::strong_ordering print operator move to_string.hh to utils/ test: network_topology: add test_topology_compare_endpoints
bhalevy · Mar 7, 2023 · 6aa91c1 · 6aa91c1
2 parents fe14d14 + bb36237
commit 6aa91c1
Show file tree

Hide file tree

Showing 33 changed files with 199 additions and 86 deletions.
diff --git a/configure.py b/configure.py
@@ -1034,6 +1034,7 @@ def find_headers(repodir, excluded_dirs):
                 'service/broadcast_tables/experimental/lang.cc',
                 'tasks/task_manager.cc',
                 'rust/wasmtime_bindings/src/lib.rs',
+                'utils/to_string.cc',
                 ] + [Antlr3Grammar('cql3/Cql.g')] + [Thrift('interface/cassandra.thrift', 'Cassandra')] \
                   + scylla_raft_core
                )

diff --git a/cql3/functions/functions.hh b/cql3/functions/functions.hh
@@ -20,7 +20,7 @@
 #include "cql3/assignment_testable.hh"
 #include "cql3/cql3_type.hh"
 #include "cql3/column_identifier.hh"
-#include "to_string.hh"
+#include "utils/to_string.hh"
 #include "log.hh"
 #include <unordered_map>
 

diff --git a/cql3/lists.hh b/cql3/lists.hh
@@ -11,7 +11,7 @@
 #pragma once
 
 #include "cql3/abstract_marker.hh"
-#include "to_string.hh"
+#include "utils/to_string.hh"
 #include "operation.hh"
 #include "utils/chunked_vector.hh"
 

diff --git a/cql3/restrictions/bounds_slice.hh b/cql3/restrictions/bounds_slice.hh
@@ -11,7 +11,7 @@
 #pragma once
 
 #include <seastar/core/shared_ptr.hh>
-#include "to_string.hh"
+#include "utils/to_string.hh"
 #include "exceptions/exceptions.hh"
 #include "index/secondary_index_manager.hh"
 #include "cql3/expr/expression.hh"

diff --git a/cql3/restrictions/statement_restrictions.hh b/cql3/restrictions/statement_restrictions.hh
@@ -15,7 +15,7 @@
 #include "bounds_slice.hh"
 #include "cql3/expr/expression.hh"
 #include "cql3/expr/restrictions.hh"
-#include "to_string.hh"
+#include "utils/to_string.hh"
 #include "schema/schema_fwd.hh"
 #include "cql3/prepare_context.hh"
 #include "cql3/statements/statement_type.hh"

diff --git a/cql3/selection/abstract_function_selector.cc b/cql3/selection/abstract_function_selector.cc
@@ -10,7 +10,7 @@
 #include "abstract_function_selector.hh"
 #include "aggregate_function_selector.hh"
 #include "scalar_function_selector.hh"
-#include "to_string.hh"
+#include "utils/to_string.hh"
 #include "cql3/selection/selector_factories.hh"
 #include "cql3/functions/abstract_function.hh"
 

diff --git a/cql3/sets.hh b/cql3/sets.hh
@@ -14,7 +14,7 @@
 #include "maps.hh"
 #include "column_specification.hh"
 #include "column_identifier.hh"
-#include "to_string.hh"
+#include "utils/to_string.hh"
 #include <unordered_set>
 
 namespace cql3 {

diff --git a/cql3/user_types.hh b/cql3/user_types.hh
@@ -14,7 +14,7 @@
 #include "column_specification.hh"
 #include "column_identifier.hh"
 #include "operation.hh"
-#include "to_string.hh"
+#include "utils/to_string.hh"
 
 namespace cql3 {
 

diff --git a/db/hints/host_filter.cc b/db/hints/host_filter.cc
@@ -9,7 +9,7 @@
 #include <string_view>
 #include <boost/algorithm/string.hpp>
 #include "locator/token_metadata.hh"
-#include "to_string.hh"
+#include "utils/to_string.hh"
 #include "host_filter.hh"
 
 namespace db {

diff --git a/generic_server.cc b/generic_server.cc
@@ -8,7 +8,7 @@
 
 #include "generic_server.hh"
 
-#include "to_string.hh"
+#include "utils/to_string.hh"
 
 #include <seastar/core/when_all.hh>
 #include <seastar/core/loop.hh>

diff --git a/gms/feature_service.cc b/gms/feature_service.cc
@@ -14,7 +14,7 @@
 #include "gms/feature_service.hh"
 #include "db/system_keyspace.hh"
 #include "db/query_context.hh"
-#include "to_string.hh"
+#include "utils/to_string.hh"
 #include <boost/algorithm/string/classification.hpp>
 #include <boost/algorithm/string/split.hpp>
 #include <boost/range/adaptor/map.hpp>

diff --git a/gms/inet_address.cc b/gms/inet_address.cc
@@ -15,7 +15,7 @@
 #include <seastar/core/print.hh>
 #include <seastar/core/future.hh>
 #include "inet_address.hh"
-#include "to_string.hh"
+#include "utils/to_string.hh"
 
 using namespace seastar;
 

diff --git a/gms/versioned_value.hh b/gms/versioned_value.hh
@@ -16,7 +16,7 @@
 #include "version_generator.hh"
 #include "gms/inet_address.hh"
 #include "dht/i_partitioner.hh"
-#include "to_string.hh"
+#include "utils/to_string.hh"
 #include "version.hh"
 #include "cdc/generation_id.hh"
 #include <unordered_set>

diff --git a/init.cc b/init.cc
@@ -8,7 +8,7 @@
 
 #include "init.hh"
 #include "gms/failure_detector.hh"
-#include "to_string.hh"
+#include "utils/to_string.hh"
 #include "gms/inet_address.hh"
 #include "utils/fb_utilities.hh"
 #include "seastarx.hh"

diff --git 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;
-    }
-
-    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
diff --git a/locator/topology.hh b/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
diff --git a/query.cc b/query.cc
@@ -15,7 +15,7 @@
 #include "query-result-set.hh"
 #include "seastar/core/shared_ptr.hh"
 #include "seastar/core/thread.hh"
-#include "to_string.hh"
+#include "utils/to_string.hh"
 #include "bytes.hh"
 #include "mutation/mutation_partition_serializer.hh"
 #include "query-result-reader.hh"

diff --git a/raft/raft.cc b/raft/raft.cc
@@ -6,7 +6,7 @@
  * SPDX-License-Identifier: AGPL-3.0-or-later
  */
 #include "raft.hh"
-#include "to_string.hh"
+#include "utils/to_string.hh"
 
 namespace raft {
 

diff --git a/repair/row_level.cc b/repair/row_level.cc
@@ -16,7 +16,7 @@
 #include "mutation_writer/multishard_writer.hh"
 #include "dht/i_partitioner.hh"
 #include "dht/sharder.hh"
-#include "to_string.hh"
+#include "utils/to_string.hh"
 #include "utils/xx_hasher.hh"
 #include "utils/UUID.hh"
 #include "utils/hash.hh"

diff --git a/replica/database.cc b/replica/database.cc
@@ -15,7 +15,7 @@
 #include "db/system_distributed_keyspace.hh"
 #include "db/commitlog/commitlog.hh"
 #include "db/config.hh"
-#include "to_string.hh"
+#include "utils/to_string.hh"
 #include "cql3/functions/functions.hh"
 #include "cql3/functions/user_function.hh"
 #include "cql3/functions/user_aggregate.hh"

diff --git a/service/pager/query_pagers.cc b/service/pager/query_pagers.cc
@@ -15,7 +15,7 @@
 #include "cql3/restrictions/statement_restrictions.hh"
 #include "log.hh"
 #include "service/storage_proxy.hh"
-#include "to_string.hh"
+#include "utils/to_string.hh"
 #include "utils/result_combinators.hh"
 #include "view_info.hh"
 #include "db/view/delete_ghost_rows_visitor.hh"

diff --git a/service/storage_service.cc b/service/storage_service.cc
@@ -24,7 +24,7 @@
 #include "log.hh"
 #include "service/migration_manager.hh"
 #include "service/raft/raft_group0.hh"
-#include "to_string.hh"
+#include "utils/to_string.hh"
 #include "gms/gossiper.hh"
 #include "gms/failure_detector.hh"
 #include "gms/feature_service.hh"

diff --git a/test/boost/auth_resource_test.cc b/test/boost/auth_resource_test.cc
@@ -14,7 +14,7 @@
 
 #include <boost/test/unit_test.hpp>
 
-#include "to_string.hh"
+#include "utils/to_string.hh"
 
 BOOST_AUTO_TEST_CASE(root_of) {
     //

diff --git a/test/boost/enum_set_test.cc b/test/boost/enum_set_test.cc
@@ -16,7 +16,7 @@
 
 #include <boost/test/unit_test.hpp>
 
-#include "to_string.hh"
+#include "utils/to_string.hh"
 
 enum class fruit { apple = 3, pear = 7, banana = 8 };