From b78255f6523aac8fc4a5061f5abacd9e42586d42 Mon Sep 17 00:00:00 2001 From: Daniel Thwaites Date: Tue, 8 Sep 2026 16:49:09 +0100 Subject: [PATCH 1/2] Add `IdentifierHash::if_exists` This constructs an identifier hash only if it already exists, which is useful to avoid allocating. --- .../src/daemon/src/common/identifier_hash.cpp | 57 ++++++++++++++++ .../src/daemon/src/common/identifier_hash.hpp | 20 ++++++ .../daemon/src/common/identifier_hash_UT.cpp | 65 +++++++++++++++++++ 3 files changed, 142 insertions(+) diff --git a/score/launch_manager/src/daemon/src/common/identifier_hash.cpp b/score/launch_manager/src/daemon/src/common/identifier_hash.cpp index c6d9b1482b..6580677708 100644 --- a/score/launch_manager/src/daemon/src/common/identifier_hash.cpp +++ b/score/launch_manager/src/daemon/src/common/identifier_hash.cpp @@ -101,6 +101,63 @@ IdentifierHash::IdentifierHash(const char* id) get_registry()[hash_id_] = sv; } +IdentifierHash::IdentifierHash(std::size_t hash_id) +{ + hash_id_ = hash_id; +} + +std::optional IdentifierHash::if_exists(const std::string& id) +{ + const std::size_t hash_id = Fnv1aHash(id); + + const std::lock_guard lock(get_registry_mutex()); + const std::unordered_map& registry = get_registry(); + + if (registry.find(hash_id) == registry.end()) + { + return std::nullopt; + } + else + { + return IdentifierHash(hash_id); + } +} + +std::optional IdentifierHash::if_exists(std::string_view id) +{ + const std::size_t hash_id = Fnv1aHash(id); + + const std::lock_guard lock(get_registry_mutex()); + const std::unordered_map& registry = get_registry(); + + if (registry.find(hash_id) == registry.end()) + { + return std::nullopt; + } + else + { + return IdentifierHash(hash_id); + } +} + +std::optional IdentifierHash::if_exists(const char* id) +{ + const std::string_view sv = (id != nullptr) ? std::string_view(id) : std::string_view(""); + const std::size_t hash_id = Fnv1aHash(sv); + + const std::lock_guard lock(get_registry_mutex()); + const std::unordered_map& registry = get_registry(); + + if (registry.find(hash_id) == registry.end()) + { + return std::nullopt; + } + else + { + return IdentifierHash(hash_id); + } +} + bool IdentifierHash::operator==(const IdentifierHash& other) const { return hash_id_ == other.hash_id_; diff --git a/score/launch_manager/src/daemon/src/common/identifier_hash.hpp b/score/launch_manager/src/daemon/src/common/identifier_hash.hpp index 67aec10452..2a71802348 100644 --- a/score/launch_manager/src/daemon/src/common/identifier_hash.hpp +++ b/score/launch_manager/src/daemon/src/common/identifier_hash.hpp @@ -16,6 +16,7 @@ #include #include +#include #include #include #include @@ -52,6 +53,21 @@ class IdentifierHash final /// @param A C-string representing an ID. explicit IdentifierHash(const char* id); + /// @brief Constructs an IdentifierHash object from the given ID, + /// iff that ID is already in the registry. + /// @param id A const reference to std::string representing an ID. + static std::optional if_exists(const std::string& id); + + /// @brief Constructs an IdentifierHash object from the given ID, + /// iff that ID is already in the registry. + /// @param id A std::string_view representing an ID. + static std::optional if_exists(std::string_view id); + + /// @brief Constructs an IdentifierHash object from the given ID, + /// iff that ID is already in the registry. + /// @param A C-string representing an ID. + static std::optional if_exists(const char* id); + // This class is trivially copyable / movable // For this reason we are applying the rule of zero @@ -135,6 +151,10 @@ class IdentifierHash final static std::mutex& get_registry_mutex(); private: + /// @brief Constructs an IdentifierHash object with the given ID. + /// @param A raw ID. + explicit IdentifierHash(std::size_t hash_id); + /// internal representation of the ID, that was passed in constructor std::size_t hash_id_ = 0; }; diff --git a/score/launch_manager/src/daemon/src/common/identifier_hash_UT.cpp b/score/launch_manager/src/daemon/src/common/identifier_hash_UT.cpp index 36cddc3e35..79c1f0733f 100644 --- a/score/launch_manager/src/daemon/src/common/identifier_hash_UT.cpp +++ b/score/launch_manager/src/daemon/src/common/identifier_hash_UT.cpp @@ -22,6 +22,9 @@ using std::stringstream; using score::mw::lifecycle::IdentifierHash; +using std::literals::string_literals::operator""s; +using std::literals::string_view_literals::operator""sv; + class IdentifierHashTest : public ::testing::Test { protected: @@ -154,6 +157,14 @@ TEST_F(IdentifierHashTest, IdentifierHash_ConstructorOverloadsAgreeOnTheSameCont ASSERT_EQ(IdentifierHash(as_string).data(), IdentifierHash(as_string_view).data()); ASSERT_EQ(IdentifierHash(as_string).data(), IdentifierHash(as_c_string).data()); ASSERT_EQ(IdentifierHash(as_string_view).data(), IdentifierHash(as_c_string).data()); + + ASSERT_EQ( + IdentifierHash::if_exists(as_string).value().data(), IdentifierHash::if_exists(as_string_view).value().data()); + ASSERT_EQ( + IdentifierHash::if_exists(as_string).value().data(), IdentifierHash::if_exists(as_c_string).value().data()); + ASSERT_EQ( + IdentifierHash::if_exists(as_string_view).value().data(), + IdentifierHash::if_exists(as_c_string).value().data()); } TEST_F(IdentifierHashTest, IdentifierHash_LessThanOperator) @@ -183,3 +194,57 @@ TEST_F(IdentifierHashTest, IdentifierHash_LessThanOperator) ASSERT_FALSE(hash2 < hash1); } } + +TEST_F(IdentifierHashTest, IdentifierHash_IfExists_Existing_CString) +{ + RecordProperty( + "Description", "Verify that IdentifierHash::if_exists(const char*) returns the hash when it exists."); + + IdentifierHash("Hello"); + EXPECT_TRUE(IdentifierHash::if_exists("Hello").has_value()); +} + +TEST_F(IdentifierHashTest, IdentifierHash_IfExists_NotExisting_CString) +{ + RecordProperty( + "Description", + "Verify that IdentifierHash::if_exists(const char*) returns std::nullopt when the hash does not exist."); + + EXPECT_FALSE(IdentifierHash::if_exists("Hello C-string").has_value()); +} + +TEST_F(IdentifierHashTest, IdentifierHash_IfExists_Existing_StringView) +{ + RecordProperty( + "Description", "Verify that IdentifierHash::if_exists(std::string_view) returns the hash when it exists."); + + IdentifierHash("Hello"sv); + EXPECT_TRUE(IdentifierHash::if_exists("Hello"sv).has_value()); +} + +TEST_F(IdentifierHashTest, IdentifierHash_IfExists_NotExisting_StringView) +{ + RecordProperty( + "Description", + "Verify that IdentifierHash::if_exists(std::string_view) returns std::nullopt when the hash does not exist."); + + EXPECT_FALSE(IdentifierHash::if_exists("Hello string view"sv).has_value()); +} + +TEST_F(IdentifierHashTest, IdentifierHash_IfExists_Existing_String) +{ + RecordProperty( + "Description", "Verify that IdentifierHash::if_exists(const std::string&) returns the hash when it exists."); + + IdentifierHash("Hello"s); + EXPECT_TRUE(IdentifierHash::if_exists("Hello"s).has_value()); +} + +TEST_F(IdentifierHashTest, IdentifierHash_IfExists_NotExisting_String) +{ + RecordProperty( + "Description", + "Verify that IdentifierHash::if_exists(const std::string&) returns std::nullopt when the hash does not exist."); + + EXPECT_FALSE(IdentifierHash::if_exists("Hello string"s).has_value()); +} From 2b6f6ea9b57f9206afb295f0c12156917701740b Mon Sep 17 00:00:00 2001 From: Daniel Thwaites Date: Wed, 9 Sep 2026 09:15:37 +0100 Subject: [PATCH 2/2] Remove duplicate `IdentifierHash` constructors --- .../src/daemon/src/common/identifier_hash.cpp | 50 --------------- .../src/daemon/src/common/identifier_hash.hpp | 24 +------ .../daemon/src/common/identifier_hash_UT.cpp | 63 ++----------------- 3 files changed, 8 insertions(+), 129 deletions(-) diff --git a/score/launch_manager/src/daemon/src/common/identifier_hash.cpp b/score/launch_manager/src/daemon/src/common/identifier_hash.cpp index 6580677708..6484b81b0c 100644 --- a/score/launch_manager/src/daemon/src/common/identifier_hash.cpp +++ b/score/launch_manager/src/daemon/src/common/identifier_hash.cpp @@ -79,13 +79,6 @@ std::size_t Fnv1aHash(std::string_view data) noexcept } // namespace -IdentifierHash::IdentifierHash(const std::string& id) -{ - hash_id_ = Fnv1aHash(id); - const std::lock_guard lock(get_registry_mutex()); - get_registry()[hash_id_] = id; -} - IdentifierHash::IdentifierHash(std::string_view id) { hash_id_ = Fnv1aHash(id); @@ -93,36 +86,11 @@ IdentifierHash::IdentifierHash(std::string_view id) get_registry()[hash_id_] = id; } -IdentifierHash::IdentifierHash(const char* id) -{ - const std::string_view sv = (id != nullptr) ? std::string_view(id) : std::string_view(""); - hash_id_ = Fnv1aHash(sv); - const std::lock_guard lock(get_registry_mutex()); - get_registry()[hash_id_] = sv; -} - IdentifierHash::IdentifierHash(std::size_t hash_id) { hash_id_ = hash_id; } -std::optional IdentifierHash::if_exists(const std::string& id) -{ - const std::size_t hash_id = Fnv1aHash(id); - - const std::lock_guard lock(get_registry_mutex()); - const std::unordered_map& registry = get_registry(); - - if (registry.find(hash_id) == registry.end()) - { - return std::nullopt; - } - else - { - return IdentifierHash(hash_id); - } -} - std::optional IdentifierHash::if_exists(std::string_view id) { const std::size_t hash_id = Fnv1aHash(id); @@ -140,24 +108,6 @@ std::optional IdentifierHash::if_exists(std::string_view id) } } -std::optional IdentifierHash::if_exists(const char* id) -{ - const std::string_view sv = (id != nullptr) ? std::string_view(id) : std::string_view(""); - const std::size_t hash_id = Fnv1aHash(sv); - - const std::lock_guard lock(get_registry_mutex()); - const std::unordered_map& registry = get_registry(); - - if (registry.find(hash_id) == registry.end()) - { - return std::nullopt; - } - else - { - return IdentifierHash(hash_id); - } -} - bool IdentifierHash::operator==(const IdentifierHash& other) const { return hash_id_ == other.hash_id_; diff --git a/score/launch_manager/src/daemon/src/common/identifier_hash.hpp b/score/launch_manager/src/daemon/src/common/identifier_hash.hpp index 2a71802348..1ea0195f6f 100644 --- a/score/launch_manager/src/daemon/src/common/identifier_hash.hpp +++ b/score/launch_manager/src/daemon/src/common/identifier_hash.hpp @@ -42,32 +42,14 @@ class IdentifierHash final { public: /// @brief Constructs an IdentifierHash object from the given ID. - /// @param id A const reference to std::string representing an ID. - explicit IdentifierHash(const std::string& id); - - /// @brief Constructs an IdentifierHash object from the given ID. - /// @param id A std::string_view representing an ID. + /// @param id A string representing an ID. explicit IdentifierHash(std::string_view id); - /// @brief Constructs an IdentifierHash object with the given ID. - /// @param A C-string representing an ID. - explicit IdentifierHash(const char* id); - /// @brief Constructs an IdentifierHash object from the given ID, - /// iff that ID is already in the registry. - /// @param id A const reference to std::string representing an ID. - static std::optional if_exists(const std::string& id); - - /// @brief Constructs an IdentifierHash object from the given ID, - /// iff that ID is already in the registry. - /// @param id A std::string_view representing an ID. + /// if that ID is already in the registry. + /// @param id A string representing an ID. static std::optional if_exists(std::string_view id); - /// @brief Constructs an IdentifierHash object from the given ID, - /// iff that ID is already in the registry. - /// @param A C-string representing an ID. - static std::optional if_exists(const char* id); - // This class is trivially copyable / movable // For this reason we are applying the rule of zero diff --git a/score/launch_manager/src/daemon/src/common/identifier_hash_UT.cpp b/score/launch_manager/src/daemon/src/common/identifier_hash_UT.cpp index 79c1f0733f..7a3dc417f0 100644 --- a/score/launch_manager/src/daemon/src/common/identifier_hash_UT.cpp +++ b/score/launch_manager/src/daemon/src/common/identifier_hash_UT.cpp @@ -22,9 +22,6 @@ using std::stringstream; using score::mw::lifecycle::IdentifierHash; -using std::literals::string_literals::operator""s; -using std::literals::string_view_literals::operator""sv; - class IdentifierHashTest : public ::testing::Test { protected: @@ -150,21 +147,9 @@ TEST_F(IdentifierHashTest, IdentifierHash_HashValueIsStableAcrossCompilersAndPro TEST_F(IdentifierHashTest, IdentifierHash_ConstructorOverloadsAgreeOnTheSameContent) { RecordProperty("Description", "Verify all constructors of IdentifierHash have the same hash."); - const std::string as_string = "ProcessGroup1/Startup"; - const std::string_view as_string_view = "ProcessGroup1/Startup"; - const char* as_c_string = "ProcessGroup1/Startup"; - - ASSERT_EQ(IdentifierHash(as_string).data(), IdentifierHash(as_string_view).data()); - ASSERT_EQ(IdentifierHash(as_string).data(), IdentifierHash(as_c_string).data()); - ASSERT_EQ(IdentifierHash(as_string_view).data(), IdentifierHash(as_c_string).data()); - - ASSERT_EQ( - IdentifierHash::if_exists(as_string).value().data(), IdentifierHash::if_exists(as_string_view).value().data()); - ASSERT_EQ( - IdentifierHash::if_exists(as_string).value().data(), IdentifierHash::if_exists(as_c_string).value().data()); - ASSERT_EQ( - IdentifierHash::if_exists(as_string_view).value().data(), - IdentifierHash::if_exists(as_c_string).value().data()); + + ASSERT_EQ(IdentifierHash().data(), IdentifierHash("").data()); + ASSERT_EQ(IdentifierHash().data(), IdentifierHash::if_exists("").value().data()); } TEST_F(IdentifierHashTest, IdentifierHash_LessThanOperator) @@ -197,8 +182,7 @@ TEST_F(IdentifierHashTest, IdentifierHash_LessThanOperator) TEST_F(IdentifierHashTest, IdentifierHash_IfExists_Existing_CString) { - RecordProperty( - "Description", "Verify that IdentifierHash::if_exists(const char*) returns the hash when it exists."); + RecordProperty("Description", "Verify that IdentifierHash::if_exists returns the hash when it exists."); IdentifierHash("Hello"); EXPECT_TRUE(IdentifierHash::if_exists("Hello").has_value()); @@ -207,44 +191,7 @@ TEST_F(IdentifierHashTest, IdentifierHash_IfExists_Existing_CString) TEST_F(IdentifierHashTest, IdentifierHash_IfExists_NotExisting_CString) { RecordProperty( - "Description", - "Verify that IdentifierHash::if_exists(const char*) returns std::nullopt when the hash does not exist."); + "Description", "Verify that IdentifierHash::if_exists returns std::nullopt when the hash does not exist."); EXPECT_FALSE(IdentifierHash::if_exists("Hello C-string").has_value()); } - -TEST_F(IdentifierHashTest, IdentifierHash_IfExists_Existing_StringView) -{ - RecordProperty( - "Description", "Verify that IdentifierHash::if_exists(std::string_view) returns the hash when it exists."); - - IdentifierHash("Hello"sv); - EXPECT_TRUE(IdentifierHash::if_exists("Hello"sv).has_value()); -} - -TEST_F(IdentifierHashTest, IdentifierHash_IfExists_NotExisting_StringView) -{ - RecordProperty( - "Description", - "Verify that IdentifierHash::if_exists(std::string_view) returns std::nullopt when the hash does not exist."); - - EXPECT_FALSE(IdentifierHash::if_exists("Hello string view"sv).has_value()); -} - -TEST_F(IdentifierHashTest, IdentifierHash_IfExists_Existing_String) -{ - RecordProperty( - "Description", "Verify that IdentifierHash::if_exists(const std::string&) returns the hash when it exists."); - - IdentifierHash("Hello"s); - EXPECT_TRUE(IdentifierHash::if_exists("Hello"s).has_value()); -} - -TEST_F(IdentifierHashTest, IdentifierHash_IfExists_NotExisting_String) -{ - RecordProperty( - "Description", - "Verify that IdentifierHash::if_exists(const std::string&) returns std::nullopt when the hash does not exist."); - - EXPECT_FALSE(IdentifierHash::if_exists("Hello string"s).has_value()); -}