diff options
author | Henning Baldersheim <balder@yahoo-inc.com> | 2020-11-25 18:17:27 +0000 |
---|---|---|
committer | Henning Baldersheim <balder@yahoo-inc.com> | 2020-11-26 00:23:06 +0000 |
commit | ee2baa1b001a282bd57318a3f0b8881cdcbc3049 (patch) | |
tree | e1bd88266adb509a9ce4006f7d68cbc59db3c295 /storageapi/src | |
parent | e1584673531bc771fa94731da337ce311b4ff7d1 (diff) |
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
Diffstat (limited to 'storageapi/src')
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 << ")"; } |