Avoid address stringification in common lookup path

Precompute internal address hash over <cluster, type, index> tuple.
No other fields are included in the hash, as this is only used for
storage API lookups.
Remove automatic resending field from address, as we never use MBus
resending functionality in the backend communication protocols.

4 files changed, 68 insertions, 11 deletions

diff --git a/storageapi/src/tests/messageapi/CMakeLists.txt b/storageapi/src/tests/messageapi/CMakeLists.txt
index 4833dc45acf..50f0b306191 100644
--- a/storageapi/src/tests/messageapi/CMakeLists.txt
+++ b/storageapi/src/tests/messageapi/CMakeLists.txt
@@ -1,5 +1,8 @@
 # Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
-vespa_add_library(storageapi_testmessageapi INTERFACE
+vespa_add_library(storageapi_testmessageapi
     SOURCES
+    storage_message_address_test.cpp
     DEPENDS
+    storageapi
+    GTest::GTest
 )
diff --git a/storageapi/src/tests/messageapi/storage_message_address_test.cpp b/storageapi/src/tests/messageapi/storage_message_address_test.cpp
new file mode 100644
index 00000000000..c340cba4b28
--- /dev/null
+++ b/storageapi/src/tests/messageapi/storage_message_address_test.cpp
@@ -0,0 +1,36 @@
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+
+#include <vespa/storageapi/messageapi/storagemessage.h>
+#include <vespa/vespalib/gtest/gtest.h>
+
+using namespace ::testing;
+
+namespace storage::api {
+
+namespace {
+
+size_t hash_of(vespalib::stringref cluster, const lib::NodeType& type, uint16_t index) {
+    return StorageMessageAddress(cluster, type, index).internal_storage_hash();
+}
+
+}
+
+TEST(StorageMessageAddressTest, storage_hash_covers_all_expected_fields) {
+    EXPECT_EQ(hash_of("foo", lib::NodeType::STORAGE, 0),
+              hash_of("foo", lib::NodeType::STORAGE, 0));
+    EXPECT_EQ(hash_of("foo", lib::NodeType::DISTRIBUTOR, 0),
+              hash_of("foo", lib::NodeType::DISTRIBUTOR, 0));
+    EXPECT_EQ(hash_of("foo", lib::NodeType::STORAGE, 123),
+              hash_of("foo", lib::NodeType::STORAGE, 123));
+
+    // These tests are all true with extremely high probability, though they do
+    // depend on a hash function that may inherently cause collisions.
+    EXPECT_NE(hash_of("foo", lib::NodeType::STORAGE, 0),
+              hash_of("bar", lib::NodeType::STORAGE, 0));
+    EXPECT_NE(hash_of("foo", lib::NodeType::STORAGE, 0),
+              hash_of("foo", lib::NodeType::DISTRIBUTOR, 0));
+    EXPECT_NE(hash_of("foo", lib::NodeType::STORAGE, 0),
+              hash_of("foo", lib::NodeType::STORAGE, 1));
+}
+
+} // storage::api
diff --git a/storageapi/src/vespa/storageapi/messageapi/storagemessage.cpp b/storageapi/src/vespa/storageapi/messageapi/storagemessage.cpp
index d1bd24f5087..8276587834a 100644
--- a/storageapi/src/vespa/storageapi/messageapi/storagemessage.cpp
+++ b/storageapi/src/vespa/storageapi/messageapi/storagemessage.cpp
@@ -6,6 +6,7 @@
 #include <vespa/vespalib/util/exceptions.h>
 #include <vespa/vespalib/util/sync.h>
 #include <vespa/vespalib/stllike/asciistream.h>
+#include <vespa/vespalib/stllike/hash_fun.h>
 #include <sstream>
 #include <cassert>
 #include <atomic>
@@ -141,8 +142,8 @@ MessageType::print(std::ostream& out, bool verbose, const std::string& indent) c
 
 StorageMessageAddress::StorageMessageAddress(const mbus::Route& route)
     : _route(route),
-      _retryEnabled(false),
       _protocol(DOCUMENT),
+      _precomputed_storage_hash(0),
       _cluster(""),
       _type(nullptr),
       _index(0xFFFF)
@@ -160,17 +161,34 @@ createAddress(vespalib::stringref cluster, const lib::NodeType& type, uint16_t i
     return os.str();
 }
 
+// TODO we ideally want this removed. Currently just in place to support usage as map key when emplacement not available
+StorageMessageAddress::StorageMessageAddress()
+    : _route(),
+      _protocol(Protocol::STORAGE),
+      _precomputed_storage_hash(0),
+      _cluster(),
+      _type(nullptr),
+      _index(0)
+{}
+
+
 StorageMessageAddress::StorageMessageAddress(vespalib::stringref cluster, const lib::NodeType& type,
                                              uint16_t index, Protocol protocol)
     : _route(),
-      _retryEnabled(false),
       _protocol(protocol),
+      _precomputed_storage_hash(0),
       _cluster(cluster),
       _type(&type),
       _index(index)
 {
     std::vector<mbus::IHopDirective::SP> directives;
-    directives.emplace_back(std::make_shared<mbus::VerbatimDirective>(createAddress(cluster, type, index)));
+    auto address_as_str = createAddress(cluster, type, index);
+    // We reuse the string representation and pass it to vespalib's hashValue instead of
+    // explicitly combining a running hash over the individual fields. This is because
+    // hashValue internally uses xxhash, which offers great dispersion of bits even for
+    // minimal changes in the input (such as single bit differences in the index).
+    _precomputed_storage_hash = vespalib::hashValue(address_as_str.data(), address_as_str.size());
+    directives.emplace_back(std::make_shared<mbus::VerbatimDirective>(std::move(address_as_str)));
     _route.addHop(mbus::Hop(std::move(directives), false));
 }
 
@@ -207,7 +225,6 @@ bool
 StorageMessageAddress::operator==(const StorageMessageAddress& other) const
 {
     if (_protocol != other._protocol) return false;
-    if (_retryEnabled != other._retryEnabled) return false;
     if (_type != other._type) return false;
     if (_type) {
         if (_cluster != other._cluster) return false;
@@ -234,9 +251,6 @@ StorageMessageAddress::print(vespalib::asciistream & out) const
     } else {
         out << "Document protocol";
     }
-    if (_retryEnabled) {
-        out << ", retry enabled";
-    }
     if (!_type) {
         out << ", " << _route.toString() << ")";
     } else {
diff --git a/storageapi/src/vespa/storageapi/messageapi/storagemessage.h b/storageapi/src/vespa/storageapi/messageapi/storagemessage.h
index 415bd7717f2..85d4e072171 100644
--- a/storageapi/src/vespa/storageapi/messageapi/storagemessage.h
+++ b/storageapi/src/vespa/storageapi/messageapi/storagemessage.h
@@ -269,14 +269,15 @@ public:
 
 private:
     mbus::Route _route;
-    bool        _retryEnabled;
     Protocol    _protocol;
     // Used for internal VDS addresses only
+    size_t               _precomputed_storage_hash;
     vespalib::string     _cluster;
     const lib::NodeType* _type;
     uint16_t             _index;
 
 public:
+    StorageMessageAddress(); // Only to be used when transient default ctor semantics are needed by containers
     StorageMessageAddress(const mbus::Route& route);
     StorageMessageAddress(vespalib::stringref clusterName,
                           const lib::NodeType& type, uint16_t index,
@@ -284,15 +285,18 @@ public:
     ~StorageMessageAddress();
 
     void setProtocol(Protocol p) { _protocol = p; }
-    void enableRetry(bool enable = true) { _retryEnabled = enable; }
 
     const mbus::Route& getRoute() const { return _route; }
-    bool retryEnabled() const { return _retryEnabled; }
     Protocol getProtocol() const { return _protocol; }
     uint16_t getIndex() const;
     const lib::NodeType& getNodeType() const;
     const vespalib::string& getCluster() const;
 
+    // Returns precomputed hash over <cluster, type, index> tuple. Other fields not included.
+    [[nodiscard]] size_t internal_storage_hash() const noexcept {
+        return _precomputed_storage_hash;
+    }
+
     bool operator==(const StorageMessageAddress& other) const;
     vespalib::string toString() const;
     friend std::ostream & operator << (std::ostream & os, const StorageMessageAddress & addr);