native_types: Reduce append allocation
This builds the string from left to right, instead of dealing with
string inserts, optimizing for allocations. Uses our own hex encoder
instead of relying on systemd libraries. This maintains explicit
compatability with the systemd encoding scheme.
Change-Id: Ia6c2f4fb19984f549c6bac2d67d0b5d610022038
Signed-off-by: William A. Kennington III <wak@google.com>
diff --git a/include/sdbusplus/message/native_types.hpp b/include/sdbusplus/message/native_types.hpp
index 1b5335b..d76604b 100644
--- a/include/sdbusplus/message/native_types.hpp
+++ b/include/sdbusplus/message/native_types.hpp
@@ -1,6 +1,7 @@
#pragma once
#include <string>
+#include <string_view>
namespace sdbusplus
{
@@ -144,18 +145,8 @@
std::string filename() const;
string_path_wrapper parent_path() const;
- string_path_wrapper operator/(const char* extId) const;
- string_path_wrapper& operator/=(const char* extId);
-
- string_path_wrapper operator/(const std::string& extId) const
- {
- return this->operator/(extId.c_str());
- }
-
- string_path_wrapper& operator/=(const std::string& extId)
- {
- return this->operator/=(extId.c_str());
- }
+ string_path_wrapper operator/(std::string_view) const;
+ string_path_wrapper& operator/=(std::string_view);
};
/** Typename for sdbus SIGNATURE types. */
diff --git a/src/message/native_types.cpp b/src/message/native_types.cpp
index 5e77cb5..cd0f010 100644
--- a/src/message/native_types.cpp
+++ b/src/message/native_types.cpp
@@ -1,9 +1,7 @@
-#include <systemd/sd-bus.h>
-
#include <sdbusplus/message/native_types.hpp>
-#include <sdbusplus/utility/memory.hpp>
#include <array>
+#include <cctype>
namespace sdbusplus
{
@@ -12,31 +10,81 @@
namespace details
{
+constexpr std::array<char, 16> hex{'0', '1', '2', '3', '4', '5', '6', '7',
+ '8', '9', 'a', 'b', 'c', 'd', 'e', 'f'};
+
+constexpr std::array<char, 256> unhex = [] {
+ std::array<char, 256> ret;
+ for (size_t i = 0; i < ret.size(); ++i)
+ {
+ ret[i] = -1;
+ }
+ for (char i = 0; i < 10; ++i)
+ {
+ ret['0' + i] = i;
+ }
+ for (char i = 0; i < 6; ++i)
+ {
+ ret['A' + i] = i + 10;
+ ret['a' + i] = i + 10;
+ }
+ return ret;
+}();
+
+inline bool pathShouldEscape(char c)
+{
+ return !std::isalnum(c);
+}
+
+inline void pathAppendEscape(std::string& s, char c)
+{
+ s.append(1, '_');
+ s.append(1, hex[(c >> 4) & 0xf]);
+ s.append(1, hex[c & 0xf]);
+}
+
std::string string_path_wrapper::filename() const
{
- size_t firstIndex = str.rfind('/');
+ std::string_view strv(str);
+ size_t firstIndex = strv.rfind('/');
// Dbus paths must start with /, if we don't find one, it's an error
if (firstIndex == std::string::npos)
{
return "";
}
- firstIndex++;
+ auto filename = strv.substr(firstIndex + 1);
+
// If we don't see that this was encoded by sdbusplus, return the naive
// version of the filename path.
- const char* filename = str.c_str() + firstIndex;
- if (*filename != '_')
+ if (filename[0] != '_')
{
return std::string(filename);
}
- _cleanup_free_ char* out = nullptr;
- int r = sd_bus_path_decode_many(filename, "%", &out);
- if (r <= 0)
+ std::string out;
+ out.reserve(filename.size());
+ for (size_t i = 0; i < filename.size(); ++i)
{
- return "";
+ if (filename[i] != '_')
+ {
+ out.append(1, filename[i]);
+ continue;
+ }
+ if (i + 2 >= filename.size())
+ {
+ return "";
+ }
+ auto ch = unhex[filename[i + 1]];
+ auto cl = unhex[filename[i + 2]];
+ if (ch == -1 || cl == -1)
+ {
+ return "";
+ }
+ out.append(1, (ch << 4) | cl);
+ i += 2;
}
- return std::string(out);
+ return out;
}
string_path_wrapper string_path_wrapper::parent_path() const
@@ -54,50 +102,37 @@
return str.substr(0, index);
}
-string_path_wrapper string_path_wrapper::operator/(const char* extId) const
+string_path_wrapper string_path_wrapper::operator/(std::string_view extId) const
{
string_path_wrapper out;
- _cleanup_free_ char* encOut = nullptr;
- int ret = sd_bus_path_encode(str.c_str(), extId, &encOut);
- if (ret < 0)
- {
- return out;
- }
- out.str = encOut;
-
- size_t firstIndex = str.size();
- if (str != "/")
- {
- firstIndex++;
- }
-
- // Attempt to encode the first character of the path. This allows the
- // filename() method to "detect" that this is a path that's been encoded
- // and to decode it properly. This was needed to support a number of
- // paths that currently dont' have any encoding, and utilize underscores
- // Ideally this, and the equivalent code in filename() would go away
- // when all paths are being encoded per systemds methods.
- if (out.str[firstIndex] == '_')
- {
- return out;
- }
-
- constexpr std::array<char, 16> hex{'0', '1', '2', '3', '4', '5', '6', '7',
- '8', '9', 'a', 'b', 'c', 'd', 'e', 'f'};
- uint8_t firstChar = static_cast<uint8_t>(*extId);
- out.str[firstIndex] = '_';
- std::array<char, 2> encoded{hex[(firstChar >> 4) & 0xF],
- hex[firstChar & 0xF]};
- out.str.insert(out.str.begin() + firstIndex + 1, encoded.begin(),
- encoded.end());
-
- return out;
+ out.str.reserve(str.size() + 1 + extId.size() * 3);
+ out.str.append(str);
+ return out /= extId;
}
-string_path_wrapper& string_path_wrapper::operator/=(const char* extId)
+string_path_wrapper& string_path_wrapper::operator/=(std::string_view extId)
{
- string_path_wrapper out = this->operator/(extId);
- this->str = std::move(out.str);
+ str.reserve(str.size() + 1 + extId.size() * 3);
+ if (!str.empty() && str[str.size() - 1] != '/')
+ {
+ str.append(1, '/');
+ }
+ if (extId.empty())
+ {
+ return *this;
+ }
+ pathAppendEscape(str, extId[0]);
+ for (auto c : extId.substr(1))
+ {
+ if (pathShouldEscape(c))
+ {
+ pathAppendEscape(str, c);
+ }
+ else
+ {
+ str.append(1, c);
+ }
+ }
return *this;
}
diff --git a/test/message/types.cpp b/test/message/types.cpp
index 3bbd937..5e4e982 100644
--- a/test/message/types.cpp
+++ b/test/message/types.cpp
@@ -53,6 +53,10 @@
EXPECT_EQ(sdbusplus::message::object_path("/_2d").filename(), "-");
EXPECT_EQ(sdbusplus::message::object_path("/_20").filename(), " ");
EXPECT_EQ(sdbusplus::message::object_path("/_2F").filename(), "/");
+ EXPECT_EQ(sdbusplus::message::object_path("/_").filename(), "");
+ EXPECT_EQ(sdbusplus::message::object_path("/_2").filename(), "");
+ EXPECT_EQ(sdbusplus::message::object_path("/_2y").filename(), "");
+ EXPECT_EQ(sdbusplus::message::object_path("/_y2").filename(), "");
EXPECT_EQ(sdbusplus::message::object_path("/bios_active").filename(),
"bios_active");
}
@@ -73,6 +77,8 @@
{
EXPECT_EQ(sdbusplus::message::object_path("/") / "abc",
sdbusplus::message::object_path("/_61bc"));
+ EXPECT_EQ(sdbusplus::message::object_path("/") / "abc",
+ sdbusplus::message::object_path("/_61bc"));
EXPECT_EQ(sdbusplus::message::object_path("/abc") / "def",
sdbusplus::message::object_path("/abc/_64ef"));
EXPECT_EQ(sdbusplus::message::object_path("/abc") / "-",
@@ -83,6 +89,10 @@
sdbusplus::message::object_path("/abc/_2f"));
EXPECT_EQ(sdbusplus::message::object_path("/abc") / "ab_cd",
sdbusplus::message::object_path("/abc/_61b_5fcd"));
+ EXPECT_EQ(sdbusplus::message::object_path("/abc") / "_ab_cd",
+ sdbusplus::message::object_path("/abc/_5fab_5fcd"));
+ EXPECT_EQ(sdbusplus::message::object_path("/abc") / "ab-c_d",
+ sdbusplus::message::object_path("/abc/_61b_2dc_5fd"));
// Test the std::string overload. This is largely just for coverage
EXPECT_EQ(sdbusplus::message::object_path("/") / std::string("abc"),
@@ -96,8 +106,8 @@
EXPECT_EQ(path, sdbusplus::message::object_path("/_61bc"));
sdbusplus::message::object_path path2("/");
- path2 /= std::string("def");
- EXPECT_EQ(path2, sdbusplus::message::object_path("/_64ef"));
+ path2 /= std::string("d-ef");
+ EXPECT_EQ(path2, sdbusplus::message::object_path("/_64_2def"));
}
TEST(MessageTypes, Signature)