Use sd-bus array methods for read and append
Currently sdbusplus uses sd_bus_message_read_basic and
message_append_basic for decoding and encoding of arrays. For large
arrays, this imposes a significant overhead in calling the library each
time. This leads to some extreme performance degradation in some cases,
for example, passing a 4K bytes data array over sdbusplus interface now
takes 4096 + 2 calls of sd_bus_message_read_basic and
message_append_basic, this will consume about 10ms CPU time on a Aspeed
2600 platform for each package on both send and receive side.
While in this case, a DBus interface design should likely opt for using
an FD rather than an array of bytes, this isn't a reason to not optimize
this case.
sd-bus, in version 240 added methods to deal with this performance
degradation, namely sd_bus_message_read_array and
sd_bus_message_append_array, which each only require a single call to
load values into the array using pointers and lengths.
This patchset is based on the one submitted here:
https://gerrit.openbmc.org/c/openbmc/sdbusplus/+/68614
But opts for a different approach to a number of technical details on
how it accomplishes the result, including changing the underlying sd-bus
calls used to utilize ones that don't require sdbus internal malloc,
avoiding unit tests ASAN issues in the process.
This commit adds support for utilizing the above methods when doing
calls to the library if the following requirements are met:
1. The container is contiguous (using the std::contiguous_iterator
concept)
2. The container represents a list of values of type int/uint 8,16,32,
or 64 bits wide. Note, per the sd-bus documentation, arrays of bool are
explicitly not supported (presumably because the internal representation
of a bool array for both libc++ and sd-bus is implementation defined).
To accomplish this, the arrays handling for contiguous arrays is moved
to use concepts, simplifying the code significantly, and allowing the
use of std::contiguous_iterator.
Change-Id: I3bd9f97ed160835e8c30c302b80553a1d450bbf9
Signed-off-by: Ed Tanous <ed@tanous.net>
Signed-off-by: Yongbing Chen <yongbingchen@google.com>
diff --git a/include/sdbusplus/message/append.hpp b/include/sdbusplus/message/append.hpp
index 46bc581..a34d7ef 100644
--- a/include/sdbusplus/message/append.hpp
+++ b/include/sdbusplus/message/append.hpp
@@ -9,6 +9,7 @@
#include <sdbusplus/utility/type_traits.hpp>
#include <bit>
+#include <iterator>
#include <string_view>
#include <tuple>
#include <type_traits>
@@ -78,10 +79,8 @@
struct can_append_multiple<bool> : std::false_type
{};
// std::vector/map/unordered_map/set need loops
-template <typename T>
-struct can_append_multiple<
- T, typename std::enable_if_t<utility::has_const_iterator_v<T>>> :
- std::false_type
+template <utility::is_dbus_array T>
+struct can_append_multiple<T> : std::false_type
{};
// std::pair needs to be broken down into components.
template <typename T1, typename T2>
@@ -264,10 +263,18 @@
}
};
-/** @brief Specialization of append_single for containers (ie vector, array,
- * set, map, etc) */
+// Determines if fallback to normal iteration and append is required (can't use
+// sd_bus_message_append_array)
template <typename T>
-struct append_single<T, std::enable_if_t<utility::has_const_iterator_v<T>>>
+concept can_append_array_non_contigious =
+ utility::is_dbus_array<T> && !utility::can_append_array_value<T>;
+
+/** @brief Specialization of append_single for containers
+ * (ie vector, array, etc), where the contiguous elements can be loaded in a
+ * single operation
+ */
+template <can_append_array_non_contigious T>
+struct append_single<T>
{
template <typename S>
static void op(sdbusplus::SdBusInterface* intf, sd_bus_message* m, S&& s)
@@ -284,6 +291,28 @@
}
};
+// Determines if the iterable type (vector, array) meets the requirements for
+// using sd_bus_message_append_array
+template <typename T>
+concept can_append_array_contigious =
+ utility::is_dbus_array<T> && utility::can_append_array_value<T>;
+
+/** @brief Specialization of append_single for vector and array T,
+ * with its elements is trivially copyable, and is an integral type,
+ * Bool is explicitly disallowed by sd-bus, so avoid it here */
+template <can_append_array_contigious T>
+struct append_single<T>
+{
+ template <typename S>
+ static void op(sdbusplus::SdBusInterface* intf, sd_bus_message* m, S&& s)
+ {
+ constexpr auto dbusType = utility::tuple_to_array(types::type_id<T>());
+ intf->sd_bus_message_append_array(
+ m, dbusType[1], s.data(),
+ s.size() * sizeof(typename T::value_type));
+ }
+};
+
/** @brief Specialization of append_single for std::pairs. */
template <typename T1, typename T2>
struct append_single<std::pair<T1, T2>>
diff --git a/include/sdbusplus/message/read.hpp b/include/sdbusplus/message/read.hpp
index d285241..84e9ee0 100644
--- a/include/sdbusplus/message/read.hpp
+++ b/include/sdbusplus/message/read.hpp
@@ -78,11 +78,12 @@
{};
// std::vector/map/unordered_vector/set need loops
-template <typename T>
-struct can_read_multiple<
- T, typename std::enable_if_t<utility::has_emplace_method_v<T> ||
- utility::has_emplace_back_method_v<T>>> :
- std::false_type
+template <utility::has_emplace T>
+struct can_read_multiple<T> : std::false_type
+{};
+
+template <utility::has_emplace_back T>
+struct can_read_multiple<T> : std::false_type
{};
// std::pair needs to be broken down into components.
@@ -210,9 +211,14 @@
}
};
-/** @brief Specialization of read_single for std::vectors. */
-template <typename S>
- requires(utility::has_emplace_back_method_v<S>)
+template <typename T>
+concept can_read_array_non_contigious =
+ utility::has_emplace_back<T> && !utility::can_append_array_value<T>;
+
+/** @brief Specialization of read_single for std::vectors, with elements that
+ * are not an integral type.
+ */
+template <can_read_array_non_contigious S>
struct read_single<S>
{
static void op(sdbusplus::SdBusInterface* intf, sd_bus_message* m, S& t)
@@ -247,9 +253,38 @@
}
};
+// Determines if fallback to normal iteration and append is required (can't use
+// sd_bus_message_append_array)
+template <typename T>
+concept CanReadArrayOneShot =
+ utility::has_emplace_back<T> && utility::can_append_array_value<T>;
+
+/** @brief Specialization of read_single for std::vectors, when elements are
+ * an integral type
+ */
+template <CanReadArrayOneShot S>
+struct read_single<S>
+{
+ template <typename T>
+ static void op(sdbusplus::SdBusInterface* intf, sd_bus_message* m, T&& t)
+ {
+ size_t sizeInBytes = 0;
+ const void* p = nullptr;
+ constexpr auto dbusType = utility::tuple_to_array(types::type_id<S>());
+ int r =
+ intf->sd_bus_message_read_array(m, dbusType[1], &p, &sizeInBytes);
+ if (r < 0)
+ {
+ throw exception::SdBusError(-r, "sd_bus_message_read_array");
+ }
+ using ContainedType = typename S::value_type;
+ const ContainedType* begin = static_cast<const ContainedType*>(p);
+ t.insert(t.end(), begin, begin + (sizeInBytes / sizeof(ContainedType)));
+ }
+};
+
/** @brief Specialization of read_single for std::map. */
-template <typename S>
- requires(utility::has_emplace_method_v<S>)
+template <utility::has_emplace S>
struct read_single<S>
{
static void op(sdbusplus::SdBusInterface* intf, sd_bus_message* m, S& t)
diff --git a/include/sdbusplus/message/types.hpp b/include/sdbusplus/message/types.hpp
index 91c7f96..b83ab95 100644
--- a/include/sdbusplus/message/types.hpp
+++ b/include/sdbusplus/message/types.hpp
@@ -229,9 +229,8 @@
struct type_id<signature> : tuple_type_id<SD_BUS_TYPE_SIGNATURE>
{};
-template <typename T>
-struct type_id<T, std::enable_if_t<utility::has_const_iterator_v<T>>> :
- std::false_type
+template <utility::is_dbus_array T>
+struct type_id<T> : std::false_type
{
static constexpr auto value =
std::tuple_cat(tuple_type_id_v<SD_BUS_TYPE_ARRAY>,
diff --git a/include/sdbusplus/sdbus.hpp b/include/sdbusplus/sdbus.hpp
index be582a6..ac14868 100644
--- a/include/sdbusplus/sdbus.hpp
+++ b/include/sdbusplus/sdbus.hpp
@@ -165,6 +165,11 @@
virtual int sd_bus_is_open(sd_bus* bus) = 0;
virtual int sd_bus_wait(sd_bus* bus, uint64_t timeout_usec) = 0;
+
+ virtual int sd_bus_message_append_array(sd_bus_message* m, char type,
+ const void* ptr, size_t size) = 0;
+ virtual int sd_bus_message_read_array(sd_bus_message* m, char type,
+ const void** ptr, size_t* size) = 0;
};
class SdBusImpl : public SdBusInterface
@@ -565,6 +570,18 @@
{
return ::sd_bus_wait(bus, timeout_usec);
}
+
+ int sd_bus_message_append_array(sd_bus_message* m, char type,
+ const void* ptr, size_t size) override
+ {
+ return ::sd_bus_message_append_array(m, type, ptr, size);
+ }
+
+ int sd_bus_message_read_array(sd_bus_message* m, char type,
+ const void** ptr, size_t* size) override
+ {
+ return ::sd_bus_message_read_array(m, type, ptr, size);
+ }
};
extern SdBusImpl sdbus_impl;
diff --git a/include/sdbusplus/test/sdbus_mock.hpp b/include/sdbusplus/test/sdbus_mock.hpp
index 2f58eae..fa65ac5 100644
--- a/include/sdbusplus/test/sdbus_mock.hpp
+++ b/include/sdbusplus/test/sdbus_mock.hpp
@@ -164,6 +164,10 @@
MOCK_METHOD(void, sd_bus_close, (sd_bus*), (override));
MOCK_METHOD(int, sd_bus_is_open, (sd_bus*), (override));
MOCK_METHOD(int, sd_bus_wait, (sd_bus*, uint64_t), (override));
+ MOCK_METHOD(int, sd_bus_message_append_array,
+ (sd_bus_message*, char, const void*, size_t), (override));
+ MOCK_METHOD(int, sd_bus_message_read_array,
+ (sd_bus_message*, char, const void**, size_t*), (override));
SdBusMock()
{
diff --git a/include/sdbusplus/utility/container_traits.hpp b/include/sdbusplus/utility/container_traits.hpp
index 340a381..5a30d4d 100644
--- a/include/sdbusplus/utility/container_traits.hpp
+++ b/include/sdbusplus/utility/container_traits.hpp
@@ -7,92 +7,30 @@
{
namespace utility
{
-
-/** has_const_iterator - Determine if type has const iterator
- *
- * @tparam T - Type to be tested.
- *
- * @value A value as to whether or not the type supports iteration
- */
template <typename T>
-struct has_const_iterator
-{
- private:
- typedef char yes;
- typedef struct
- {
- char array[2];
- } no;
+concept is_dbus_array = requires(T v) {
+ { v.begin() } -> std::forward_iterator;
+ !std::is_same_v<std::decay<T>, std::string>;
+ };
- template <typename C>
- static constexpr yes test(decltype(std::cbegin(std::declval<C>()))*);
- template <typename C>
- static constexpr no test(...);
-
- public:
- static constexpr bool value = sizeof(test<T>(nullptr)) == sizeof(yes);
-};
+template <typename T, typename... U>
+concept is_any_of = (std::same_as<T, U> || ...);
template <typename T>
-inline constexpr bool has_const_iterator_v = has_const_iterator<T>::value;
-
-/** has_emplace_method - Determine if type has a method template named emplace
- *
- * @tparam T - Type to be tested.
- *
- * @value A value as to whether or not the type has an emplace method
- */
-template <typename T>
-struct has_emplace_method
-{
- private:
- struct dummy
- {};
-
- template <typename C, typename P>
- static constexpr auto test(P* p)
- -> decltype(std::declval<C>().emplace(*p), std::true_type());
-
- template <typename, typename>
- static std::false_type test(...);
-
- public:
- static constexpr bool value =
- std::is_same_v<std::true_type, decltype(test<T, dummy>(nullptr))>;
-};
+concept can_append_array_value =
+ requires(T v) {
+ // Require that the container be contiguous
+ { v.begin() } -> std::contiguous_iterator;
+ // The sd-bus append array docs are very specific about what types are
+ // allowed; verify this is one of those types.
+ requires(is_any_of<typename T::value_type, uint8_t, int16_t, uint16_t,
+ int32_t, uint32_t, int64_t, uint64_t, double>);
+ };
template <typename T>
-inline constexpr bool has_emplace_method_v = has_emplace_method<T>::value;
-
-/** has_emplace_method - Determine if type has a method template named
- * emplace_back
- *
- * @tparam T - Type to be tested.
- *
- * @value A value as to whether or not the type has an emplace_back method
- */
-template <typename T>
-struct has_emplace_back_method
-{
- private:
- struct dummy
- {};
-
- template <typename C, typename P>
- static constexpr auto test(P* p)
- -> decltype(std::declval<C>().emplace_back(*p), std::true_type());
-
- template <typename, typename>
- static std::false_type test(...);
-
- public:
- static constexpr bool value =
- std::is_same_v<std::true_type, decltype(test<T, dummy>(nullptr))>;
-};
+concept has_emplace_back = requires(T v) { v.emplace_back(); };
template <typename T>
-inline constexpr bool has_emplace_back_method_v =
- has_emplace_back_method<T>::value;
-
+concept has_emplace = requires(T v) { v.emplace(); };
} // namespace utility
} // namespace sdbusplus
diff --git a/test/message/append.cpp b/test/message/append.cpp
index e753b01..8e0e558 100644
--- a/test/message/append.cpp
+++ b/test/message/append.cpp
@@ -90,6 +90,18 @@
EXPECT_CALL(mock, sd_bus_message_close_container(nullptr))
.WillOnce(Return(0));
}
+
+ static int on_array_append(sd_bus_message*, char, const void*, size_t)
+ {
+ return 0;
+ }
+
+ void expect_append_array(char type, size_t sz)
+ {
+ EXPECT_CALL(mock,
+ sd_bus_message_append_array(nullptr, type, testing::_, sz))
+ .WillOnce(testing::Invoke(on_array_append));
+ }
};
TEST_F(AppendTest, RValueInt)
@@ -253,13 +265,7 @@
!sdbusplus::message::details::can_append_multiple_v<decltype(a)>);
{
- testing::InSequence seq;
- expect_open_container(SD_BUS_TYPE_ARRAY, "d");
- for (const auto& i : a)
- {
- expect_basic<double>(SD_BUS_TYPE_DOUBLE, i);
- }
- expect_close_container();
+ expect_append_array(SD_BUS_TYPE_DOUBLE, a.size() * sizeof(double));
}
new_message().append(a);
}
@@ -272,13 +278,7 @@
!sdbusplus::message::details::can_append_multiple_v<decltype(s)>);
{
- testing::InSequence seq;
- expect_open_container(SD_BUS_TYPE_ARRAY, "d");
- for (const auto& i : s)
- {
- expect_basic<double>(SD_BUS_TYPE_DOUBLE, i);
- }
- expect_close_container();
+ expect_append_array(SD_BUS_TYPE_DOUBLE, a.size() * sizeof(double));
}
new_message().append(s);
}
@@ -299,6 +299,31 @@
new_message().append(v);
}
+TEST_F(AppendTest, VectorIntegral)
+{
+ const std::vector<int32_t> v{1, 2, 3, 4};
+ expect_append_array(SD_BUS_TYPE_INT32, v.size() * sizeof(int32_t));
+ new_message().append(v);
+}
+
+TEST_F(AppendTest, VectorNestIntegral)
+{
+ const std::vector<std::array<int32_t, 3>> v{
+ {1, 2, 3}, {3, 4, 5}, {6, 7, 8}};
+
+ {
+ testing::InSequence seq;
+ expect_open_container(SD_BUS_TYPE_ARRAY, "ai");
+ for (long unsigned int i = 0; i < v.size(); i++)
+ {
+ expect_append_array(SD_BUS_TYPE_INT32,
+ v[i].size() * sizeof(int32_t));
+ }
+ expect_close_container();
+ }
+ new_message().append(v);
+}
+
TEST_F(AppendTest, Set)
{
const std::set<std::string> s{"one", "two", "eight"};
diff --git a/test/message/read.cpp b/test/message/read.cpp
index a70fd3b..581f7d0 100644
--- a/test/message/read.cpp
+++ b/test/message/read.cpp
@@ -21,6 +21,8 @@
{
using testing::DoAll;
+using testing::ElementsAre;
+using testing::Invoke;
using testing::Return;
using testing::StrEq;
@@ -62,6 +64,26 @@
.WillOnce(DoAll(AssignReadVal<T>(val), Return(0)));
}
+ template <typename T>
+ static void read_array_callback(sd_bus_message*, char, const void** p,
+ size_t* sz)
+ {
+ static const std::array<T, 3> arr{std::numeric_limits<T>::min(), 0,
+ std::numeric_limits<T>::max()};
+
+ *p = arr.data();
+ *sz = arr.size() * sizeof(T);
+ }
+
+ template <typename T>
+ void expect_read_array(char type)
+ {
+ EXPECT_CALL(mock, sd_bus_message_read_array(nullptr, type, testing::_,
+ testing::_))
+ .WillOnce(
+ DoAll(testing::Invoke(read_array_callback<T>), Return(0)));
+ }
+
void expect_verify_type(char type, const char* contents, int ret)
{
EXPECT_CALL(mock,
@@ -243,6 +265,56 @@
EXPECT_EQ(vs, ret_vs);
}
+TEST_F(ReadTest, VectorUnsignedIntegral8)
+{
+ expect_read_array<uint8_t>(SD_BUS_TYPE_BYTE);
+ std::vector<uint8_t> ret_vi;
+ new_message().read(ret_vi);
+ EXPECT_THAT(ret_vi, ElementsAre(0, 0, 255));
+}
+
+TEST_F(ReadTest, VectorIntegral32)
+{
+ expect_read_array<int32_t>(SD_BUS_TYPE_INT32);
+ std::vector<int32_t> ret_vi;
+ new_message().read(ret_vi);
+ EXPECT_THAT(ret_vi, ElementsAre(-2147483648, 0, 2147483647));
+}
+
+TEST_F(ReadTest, VectorIntegral64)
+{
+ expect_read_array<int64_t>(SD_BUS_TYPE_INT64);
+ std::vector<int64_t> ret_vi;
+ new_message().read(ret_vi);
+ EXPECT_THAT(ret_vi, ElementsAre(-9223372036854775807LL - 1, 0,
+ 9223372036854775807LL));
+}
+
+TEST_F(ReadTest, VectorUnsignedIntegral32)
+{
+ expect_read_array<uint32_t>(SD_BUS_TYPE_UINT32);
+ std::vector<uint32_t> ret_vi;
+ new_message().read(ret_vi);
+ EXPECT_THAT(ret_vi, ElementsAre(0, 0, 4294967295));
+}
+
+TEST_F(ReadTest, VectorUnsignedIntegral64)
+{
+ expect_read_array<uint64_t>(SD_BUS_TYPE_UINT64);
+ std::vector<uint64_t> ret_vi;
+ new_message().read(ret_vi);
+ EXPECT_THAT(ret_vi, ElementsAre(0, 0, 18446744073709551615ULL));
+}
+
+TEST_F(ReadTest, VectorDouble)
+{
+ expect_read_array<double>(SD_BUS_TYPE_DOUBLE);
+ std::vector<double> ret_vi;
+ new_message().read(ret_vi);
+ EXPECT_THAT(ret_vi, ElementsAre(2.2250738585072014e-308, 0,
+ 1.7976931348623157e+308));
+}
+
TEST_F(ReadTest, VectorEnterError)
{
{