From 6a888e14944fbf8369a19354605bd54f7f0e518f Mon Sep 17 00:00:00 2001 From: Yonas Habteab Date: Thu, 27 Feb 2025 17:20:07 +0100 Subject: [PATCH 1/3] String: Mark move constructor & assignment op as `noexcept` The Icinga DB code performs intensive operations on certain STL containers, primarily on `std::vector`. Specifically, it inserts 2-3 new elements at the beginning of a vector containing thousands of elements. Without this commit, all the existing elements would be unnecessarily copied just to accommodate the new elements at the front. By making this change, the compiler is able to optimize STL operations like `push_back`, `emplace_back`, and `insert`, enabling it to prefer the move constructor over copy operations, provided it is guaranteed that no exceptions will be thrown. --- lib/base/string.cpp | 4 ++-- lib/base/string.hpp | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/base/string.cpp b/lib/base/string.cpp index 3c440cd77..bad3116a5 100644 --- a/lib/base/string.cpp +++ b/lib/base/string.cpp @@ -33,7 +33,7 @@ String::String(const String& other) : m_Data(other) { } -String::String(String&& other) +String::String(String&& other) noexcept : m_Data(std::move(other.m_Data)) { } @@ -66,7 +66,7 @@ String& String::operator=(const String& rhs) return *this; } -String& String::operator=(String&& rhs) +String& String::operator=(String&& rhs) noexcept { m_Data = std::move(rhs.m_Data); return *this; diff --git a/lib/base/string.hpp b/lib/base/string.hpp index 0eb08b527..896c74d0b 100644 --- a/lib/base/string.hpp +++ b/lib/base/string.hpp @@ -44,7 +44,7 @@ public: String(std::string data); String(String::SizeType n, char c); String(const String& other); - String(String&& other); + String(String&& other) noexcept; #ifndef _MSC_VER String(Value&& other); @@ -56,7 +56,7 @@ public: { } String& operator=(const String& rhs); - String& operator=(String&& rhs); + String& operator=(String&& rhs) noexcept; String& operator=(Value&& rhs); String& operator=(const std::string& rhs); String& operator=(const char *rhs); From e308552eccbab74069c30bc2234cd26e5e5e1f9f Mon Sep 17 00:00:00 2001 From: Julian Brost Date: Thu, 6 Mar 2025 11:46:48 +0100 Subject: [PATCH 2/3] Add test that std::vector uses move overloads --- test/CMakeLists.txt | 1 + test/base-string.cpp | 25 +++++++++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index a255178da..6ceb48683 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -165,6 +165,7 @@ add_boost_test(base base_string/replace base_string/index base_string/find + base_string/vector_move base_timer/construct base_timer/interval base_timer/invoke diff --git a/test/base-string.cpp b/test/base-string.cpp index 835b1a643..5b28c5481 100644 --- a/test/base-string.cpp +++ b/test/base-string.cpp @@ -1,6 +1,7 @@ /* Icinga 2 | (c) 2012 Icinga GmbH | GPLv2+ */ #include "base/string.hpp" +#include #include using namespace icinga; @@ -101,4 +102,28 @@ BOOST_AUTO_TEST_CASE(find) BOOST_CHECK(s.FindFirstOf("xl") == 2); } +// Check that if a std::vector is grown beyond its capacity (i.e. it has to reallocate the memory), +// it uses the move constructor of icinga::String (i.e. the underlying string storage stays the same). +BOOST_AUTO_TEST_CASE(vector_move) +{ + std::vector vec { + // std::string (which is internally used by icinga::String) has an optimization that small strings can be + // allocated inside it instead of in a separate heap allocation. In that case, the small string would still be + // copied even by the move constructor. Using sizeof() ensures that the string is long enough so that it must + // be allocated separately and can be used to test for the desired move to happen. + std::string(sizeof(String) + 1, 'A'), + }; + + void *oldAddr = vec[0].GetData().data(); + // Sanity check that the data buffer is actually allocated outside the icinga::String instance. + BOOST_CHECK(!(&vec[0] <= oldAddr && oldAddr < &vec[1])); + + // Force the vector to grow. + vec.reserve(vec.capacity() + 1); + + // If the string was moved, the location of its underlying data buffer should not have changed. + void *newAddr = vec[0].GetData().data(); + BOOST_CHECK_EQUAL(oldAddr, newAddr); +} + BOOST_AUTO_TEST_SUITE_END() From 3e9292a349daf9432430487c57bbce73869b1b22 Mon Sep 17 00:00:00 2001 From: Yonas Habteab Date: Thu, 27 Feb 2025 17:38:05 +0100 Subject: [PATCH 3/3] Value: Add a specialized rvalue reference of `Get()` The move `String(Value&&)` constructor tries to partially move `String` values from a `Value` type. However, since there was no an appropriate `Value::Get()` implementation that binds to the requested move operation, the compiler will actually not move the value but copy it instead as the only available implementation of `Value::Get()` returns a const reference `const T&`. This commit adds a new overload that returns a non-const reference and allows to optionally move the string value of a Value type. --- lib/base/string.cpp | 2 +- lib/base/value.cpp | 4 ++++ lib/base/value.hpp | 10 ++++++++++ test/CMakeLists.txt | 1 + test/base-string.cpp | 17 +++++++++++++++++ 5 files changed, 33 insertions(+), 1 deletion(-) diff --git a/lib/base/string.cpp b/lib/base/string.cpp index bad3116a5..2806902ac 100644 --- a/lib/base/string.cpp +++ b/lib/base/string.cpp @@ -47,7 +47,7 @@ String::String(Value&& other) String& String::operator=(Value&& other) { if (other.IsString()) - m_Data = std::move(other.Get()); + *this = std::move(other.Get()); // Will atomically bind to the move assignment operator below. else *this = static_cast(other); diff --git a/lib/base/value.cpp b/lib/base/value.cpp index ebf3ba60b..1e357a92a 100644 --- a/lib/base/value.cpp +++ b/lib/base/value.cpp @@ -9,9 +9,13 @@ using namespace icinga; template class boost::variant; template const double& Value::Get() const; +template double& Value::Get(); template const bool& Value::Get() const; +template bool& Value::Get(); template const String& Value::Get() const; +template String& Value::Get(); template const Object::Ptr& Value::Get() const; +template Object::Ptr& Value::Get(); const Value icinga::Empty; diff --git a/lib/base/value.hpp b/lib/base/value.hpp index 6e64abb43..9533d710c 100644 --- a/lib/base/value.hpp +++ b/lib/base/value.hpp @@ -140,14 +140,24 @@ public: return boost::get(m_Value); } + template + T& Get() + { + return boost::get(m_Value); + } + private: boost::variant m_Value; }; extern template const double& Value::Get() const; +extern template double& Value::Get(); extern template const bool& Value::Get() const; +extern template bool& Value::Get(); extern template const String& Value::Get() const; +extern template String& Value::Get(); extern template const Object::Ptr& Value::Get() const; +extern template Object::Ptr& Value::Get(); extern const Value Empty; diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 6ceb48683..c4b1041dd 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -166,6 +166,7 @@ add_boost_test(base base_string/index base_string/find base_string/vector_move + base_string/move_string_out_of_Value_type base_timer/construct base_timer/interval base_timer/invoke diff --git a/test/base-string.cpp b/test/base-string.cpp index 5b28c5481..50c1f6af8 100644 --- a/test/base-string.cpp +++ b/test/base-string.cpp @@ -1,6 +1,7 @@ /* Icinga 2 | (c) 2012 Icinga GmbH | GPLv2+ */ #include "base/string.hpp" +#include "base/value.hpp" #include #include @@ -126,4 +127,20 @@ BOOST_AUTO_TEST_CASE(vector_move) BOOST_CHECK_EQUAL(oldAddr, newAddr); } +// Test that the move constructor of icinga::String actually moves the underlying std::string out of a Value instance. +// The constructor overload is only available on non-Windows platforms though, so we need to skip the test on Windows. +BOOST_AUTO_TEST_CASE(move_string_out_of_Value_type) +{ +#ifndef _MSC_VER + Value value("Icinga 2"); + String other = value.Get(); // We didn't request a move, so this should just copy the string. + BOOST_CHECK_EQUAL("Icinga 2", value.Get()); + BOOST_CHECK_EQUAL("Icinga 2", other); + + String newStr = std::move(value); + BOOST_CHECK_EQUAL("", value.Get()); + BOOST_CHECK_EQUAL(newStr, "Icinga 2"); +#endif /* _MSC_VER */ +} + BOOST_AUTO_TEST_SUITE_END()