As we have have now removed the expensive Route member we can further compact the message objects.

- Compact StorageMessageAddress to 16 bytes by
   - using reference to cluster name.
   - Use small enums for protocol and node type.
- Avoid having StorageMessage as separate allocation.
- Avoid default values

5 files changed, 78 insertions, 97 deletions

diff --git a/storageapi/src/tests/mbusprot/storageprotocoltest.cpp b/storageapi/src/tests/mbusprot/storageprotocoltest.cpp
index e413a62ae39..e807ab673d5 100644
--- a/storageapi/src/tests/mbusprot/storageprotocoltest.cpp
+++ b/storageapi/src/tests/mbusprot/storageprotocoltest.cpp
@@ -115,7 +115,8 @@ namespace {
 }
 
 TEST_F(StorageProtocolTest, testAddress50) {
-    StorageMessageAddress address("foo", lib::NodeType::STORAGE, 3);
+    vespalib::string cluster("foo");
+    StorageMessageAddress address(&cluster, lib::NodeType::STORAGE, 3);
     EXPECT_EQ(vespalib::string("storage/cluster.foo/storage/3/default"),
                          address.to_mbus_route().toString());
 }
@@ -816,25 +817,25 @@ TEST_P(StorageProtocolTest, serialized_size_is_used_to_set_approx_size_of_storag
 }
 
 TEST_P(StorageProtocolTest, track_memory_footprint_for_some_messages) {
-    EXPECT_EQ(64u, sizeof(StorageMessage));
-    EXPECT_EQ(80u, sizeof(StorageReply));
-    EXPECT_EQ(104u, sizeof(BucketReply));
+    EXPECT_EQ(72u, sizeof(StorageMessage));
+    EXPECT_EQ(88u, sizeof(StorageReply));
+    EXPECT_EQ(112u, sizeof(BucketReply));
     EXPECT_EQ(8u, sizeof(document::BucketId));
     EXPECT_EQ(16u, sizeof(document::Bucket));
     EXPECT_EQ(32u, sizeof(BucketInfo));
-    EXPECT_EQ(136u, sizeof(BucketInfoReply));
-    EXPECT_EQ(280u, sizeof(PutReply));
-    EXPECT_EQ(264u, sizeof(UpdateReply));
-    EXPECT_EQ(256u, sizeof(RemoveReply));
-    EXPECT_EQ(344u, sizeof(GetReply));
-    EXPECT_EQ(80u, sizeof(StorageCommand));
-    EXPECT_EQ(104u, sizeof(BucketCommand));
-    EXPECT_EQ(104u, sizeof(BucketInfoCommand));
-    EXPECT_EQ(104u + sizeof(std::string), sizeof(TestAndSetCommand));
-    EXPECT_EQ(136u + sizeof(std::string), sizeof(PutCommand));
-    EXPECT_EQ(136u + sizeof(std::string), sizeof(UpdateCommand));
-    EXPECT_EQ(216u + sizeof(std::string), sizeof(RemoveCommand));
-    EXPECT_EQ(288u, sizeof(GetCommand));
+    EXPECT_EQ(144u, sizeof(BucketInfoReply));
+    EXPECT_EQ(288u, sizeof(PutReply));
+    EXPECT_EQ(272u, sizeof(UpdateReply));
+    EXPECT_EQ(264u, sizeof(RemoveReply));
+    EXPECT_EQ(352u, sizeof(GetReply));
+    EXPECT_EQ(88u, sizeof(StorageCommand));
+    EXPECT_EQ(112u, sizeof(BucketCommand));
+    EXPECT_EQ(112u, sizeof(BucketInfoCommand));
+    EXPECT_EQ(112u + sizeof(std::string), sizeof(TestAndSetCommand));
+    EXPECT_EQ(144u + sizeof(std::string), sizeof(PutCommand));
+    EXPECT_EQ(144u + sizeof(std::string), sizeof(UpdateCommand));
+    EXPECT_EQ(224u + sizeof(std::string), sizeof(RemoveCommand));
+    EXPECT_EQ(296u, sizeof(GetCommand));
 }
 
 } // storage::api
diff --git a/storageapi/src/tests/messageapi/storage_message_address_test.cpp b/storageapi/src/tests/messageapi/storage_message_address_test.cpp
index f7f254e9119..edcf795368a 100644
--- a/storageapi/src/tests/messageapi/storage_message_address_test.cpp
+++ b/storageapi/src/tests/messageapi/storage_message_address_test.cpp
@@ -9,8 +9,8 @@ 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();
+size_t hash_of(const vespalib::string & cluster, const lib::NodeType& type, uint16_t index) {
+    return StorageMessageAddress(&cluster, type, index).internal_storage_hash();
 }
 
 }
@@ -32,8 +32,8 @@ TEST(StorageMessageAddressTest, storage_hash_covers_all_expected_fields) {
     EXPECT_NE(hash_of("foo", lib::NodeType::STORAGE, 0),
               hash_of("foo", lib::NodeType::STORAGE, 1));
 
-    EXPECT_EQ(88u, sizeof(StorageMessageAddress));
-    EXPECT_EQ(64u, sizeof(StorageMessage));
+    EXPECT_EQ(16u, sizeof(StorageMessageAddress));
+    EXPECT_EQ(72u, sizeof(StorageMessage));
     EXPECT_EQ(16u, sizeof(mbus::Trace));
 }
 
diff --git a/storageapi/src/vespa/storageapi/messageapi/storagemessage.cpp b/storageapi/src/vespa/storageapi/messageapi/storagemessage.cpp
index 9c5df379d22..978b5d4a7e4 100644
--- a/storageapi/src/vespa/storageapi/messageapi/storagemessage.cpp
+++ b/storageapi/src/vespa/storageapi/messageapi/storagemessage.cpp
@@ -143,39 +143,47 @@ std::ostream & operator << (std::ostream & os, const StorageMessageAddress & add
     return os << addr.toString();
 }
 
-static vespalib::string
-createAddress(vespalib::stringref cluster, const lib::NodeType& type, uint16_t index)
-{
+namespace {
+
+vespalib::string
+createAddress(vespalib::stringref cluster, const lib::NodeType &type, uint16_t index) {
     vespalib::asciistream os;
     os << STORAGEADDRESS_PREFIX << cluster << '/' << type.toString() << '/' << index << "/default";
     return os.str();
 }
 
-size_t
-calculate_node_hash(const lib::NodeType& type, uint16_t index)
-{
-    uint16_t buf[] = { type, index };
-    return vespalib::hashValue(&buf, sizeof(buf));
+uint32_t
+calculate_node_hash(const lib::NodeType &type, uint16_t index) {
+    uint16_t buf[] = {type, index};
+    size_t hash =  vespalib::hashValue(&buf, sizeof(buf));
+    return uint32_t(hash & 0xffffffffl) ^ uint32_t(hash >> 32);
+}
+
+vespalib::string Empty;
+
 }
 
 // TODO we ideally want this removed. Currently just in place to support usage as map key when emplacement not available
 StorageMessageAddress::StorageMessageAddress()
-    : _cluster(),
+    : _cluster(&Empty),
       _precomputed_storage_hash(0),
-      _type(nullptr),
+      _type(lib::NodeType::Type::UNKNOWN),
       _protocol(Protocol::STORAGE),
       _index(0)
 {}
 
-StorageMessageAddress::StorageMessageAddress(vespalib::stringref cluster, const lib::NodeType& type,
+StorageMessageAddress::StorageMessageAddress(const vespalib::string * cluster, const lib::NodeType& type, uint16_t index)
+    : StorageMessageAddress(cluster, type, index, Protocol::STORAGE)
+{ }
+
+StorageMessageAddress::StorageMessageAddress(const vespalib::string * cluster, const lib::NodeType& type,
                                              uint16_t index, Protocol protocol)
     : _cluster(cluster),
       _precomputed_storage_hash(calculate_node_hash(type, index)),
-      _type(&type),
+      _type(type.getType()),
       _protocol(protocol),
       _index(index)
-{
-}
+{ }
 
 StorageMessageAddress::~StorageMessageAddress() = default;
 
@@ -183,50 +191,20 @@ mbus::Route
 StorageMessageAddress::to_mbus_route() const
 {
     mbus::Route result;
-    auto address_as_str = createAddress(_cluster, *_type, _index);
+    auto address_as_str = createAddress(getCluster(), lib::NodeType::get(_type), _index);
     std::vector<mbus::IHopDirective::SP> directives;
     directives.emplace_back(std::make_shared<mbus::VerbatimDirective>(std::move(address_as_str)));
     result.addHop(mbus::Hop(std::move(directives), false));
     return result;
 }
 
-uint16_t
-StorageMessageAddress::getIndex() const
-{
-    if (!_type) {
-        throw vespalib::IllegalStateException("Cannot retrieve node index out of external address", VESPA_STRLOC);
-    }
-    return _index;
-}
-
-const lib::NodeType&
-StorageMessageAddress::getNodeType() const
-{
-    if (!_type) {
-        throw vespalib::IllegalStateException("Cannot retrieve node type out of external address", VESPA_STRLOC);
-    }
-    return *_type;
-}
-
-const vespalib::string&
-StorageMessageAddress::getCluster() const
-{
-    if (!_type) {
-        throw vespalib::IllegalStateException("Cannot retrieve cluster out of external address", VESPA_STRLOC);
-    }
-    return _cluster;
-}
-
 bool
 StorageMessageAddress::operator==(const StorageMessageAddress& other) const
 {
     if (_protocol != other._protocol) return false;
     if (_type != other._type) return false;
-    if (_type) {
-        if (_index != other._index) return false;
-        if (_type != other._type) return false;
-        if (_cluster != other._cluster) return false;
-    }
+    if (_index != other._index) return false;
+    if (getCluster() != other.getCluster()) return false;
     return true;
 }
 
@@ -242,15 +220,15 @@ void
 StorageMessageAddress::print(vespalib::asciistream & out) const
 {
     out << "StorageMessageAddress(";
-    if (_protocol == STORAGE) {
+    if (_protocol == Protocol::STORAGE) {
         out << "Storage protocol";
     } else {
         out << "Document protocol";
     }
-    if (!_type) {
+    if (_type == lib::NodeType::Type::UNKNOWN) {
         out << ", " << to_mbus_route().toString() << ")";
     } else {
-        out << ", cluster " << _cluster << ", nodetype " << *_type
+        out << ", cluster " << getCluster() << ", nodetype " << lib::NodeType::get(_type)
             << ", index " << _index << ")";
     }
 }
diff --git a/storageapi/src/vespa/storageapi/messageapi/storagemessage.h b/storageapi/src/vespa/storageapi/messageapi/storagemessage.h
index 98552e473c1..21d2d1b8e13 100644
--- a/storageapi/src/vespa/storageapi/messageapi/storagemessage.h
+++ b/storageapi/src/vespa/storageapi/messageapi/storagemessage.h
@@ -263,40 +263,44 @@ public:
  */
 class StorageMessageAddress {
 public:
-    enum Protocol { STORAGE, DOCUMENT };
+    enum class Protocol : uint8_t { STORAGE, DOCUMENT };
 
 private:
-    vespalib::string  _cluster;
+    const vespalib::string  *_cluster;
     // Used for internal VDS addresses only
-    size_t               _precomputed_storage_hash;
-    const lib::NodeType* _type;
-    Protocol             _protocol;
-    uint16_t             _index;
+    uint32_t                 _precomputed_storage_hash;
+    lib::NodeType::Type      _type;
+    Protocol                 _protocol;
+    uint16_t                 _index;
 
 public:
     StorageMessageAddress(); // Only to be used when transient default ctor semantics are needed by containers
-    StorageMessageAddress(vespalib::stringref clusterName,
-                          const lib::NodeType& type, uint16_t index,
-                          Protocol protocol = STORAGE);
+    StorageMessageAddress(const vespalib::string * clusterName, const lib::NodeType& type, uint16_t index);
+    StorageMessageAddress(const vespalib::string * cluster, const lib::NodeType& type, uint16_t index, Protocol protocol);
     ~StorageMessageAddress();
 
     void setProtocol(Protocol p) { _protocol = p; }
 
     mbus::Route to_mbus_route() const;
     Protocol getProtocol() const { return _protocol; }
-    uint16_t getIndex() const;
-    const lib::NodeType& getNodeType() const;
-    const vespalib::string& getCluster() const;
+    uint16_t getIndex() const { return _index; }
+    lib::NodeType::Type getNodeType() const { return _type; }
+    const vespalib::string& getCluster() const { return *_cluster; }
 
     // Returns precomputed hash over <type, index> pair. Other fields not included.
-    [[nodiscard]] size_t internal_storage_hash() const noexcept {
+    [[nodiscard]] uint32_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);
-
+    static StorageMessageAddress create(const vespalib::string * cluster, const lib::NodeType& type, uint16_t index) {
+        return api::StorageMessageAddress(cluster, type, index);
+    }
+    static StorageMessageAddress createDocApi(const vespalib::string * cluster, const lib::NodeType& type, uint16_t index) {
+        return api::StorageMessageAddress(cluster, type, index, Protocol::DOCUMENT);
+    }
 private:
     void print(vespalib::asciistream & out) const;
 };
@@ -354,12 +358,12 @@ private:
 protected:
     static Id generateMsgId();
 
-    const MessageType& _type;
-    Id                 _msgId;
-    std::unique_ptr<StorageMessageAddress> _address;
-    vespalib::Trace        _trace;
-    uint32_t    _approxByteSize;
-    Priority    _priority;
+    const MessageType&    _type;
+    Id                    _msgId;
+    StorageMessageAddress _address;
+    vespalib::Trace       _trace;
+    uint32_t              _approxByteSize;
+    Priority              _priority;
 
     StorageMessage(const MessageType& code, Id id);
     StorageMessage(const StorageMessage&, Id id);
@@ -386,10 +390,10 @@ public:
     void setPriority(Priority p) { _priority = p; }
     Priority getPriority() const { return _priority; }
 
-    const StorageMessageAddress* getAddress() const { return _address.get(); }
+    const StorageMessageAddress* getAddress() const { return (_address.getNodeType() != lib::NodeType::Type::UNKNOWN) ? &_address : nullptr; }
 
     void setAddress(const StorageMessageAddress& address) {
-        _address = std::make_unique<StorageMessageAddress>(address);
+        _address = address;
     }
 
     /**
diff --git a/storageapi/src/vespa/storageapi/messageapi/storagereply.cpp b/storageapi/src/vespa/storageapi/messageapi/storagereply.cpp
index 2bb9fabd7d5..63749c5b341 100644
--- a/storageapi/src/vespa/storageapi/messageapi/storagereply.cpp
+++ b/storageapi/src/vespa/storageapi/messageapi/storagereply.cpp
@@ -12,7 +12,7 @@ StorageReply::StorageReply(const StorageCommand& cmd, ReturnCode code)
 {
     setPriority(cmd.getPriority());
     if (cmd.getAddress()) {
-        setAddress(*cmd.getAddress());
+        setAddress(*cmd.getAddress()); // Hmm, could we steal the address ?
     }
     // TODD do we really need copy construction
     if ( ! cmd.getTrace().isEmpty()) {
@@ -26,10 +26,8 @@ StorageReply::StorageReply(const StorageCommand& cmd, ReturnCode code)
 StorageReply::~StorageReply() = default;
 
 void
-StorageReply::print(std::ostream& out, bool verbose,
-                    const std::string& indent) const
+StorageReply::print(std::ostream& out, bool , const std::string& ) const
 {
-    (void) verbose; (void) indent;
     out << "StorageReply(" << _type.getName() << ", " << _result << ")";
 }