Skip to content

perf: stop copying every pugixml node().value() into std::string in ParseXML #234

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
harshavardhana merged 3 commits into from
Jun 19, 2026
+89 −62
Merged

perf: stop copying every pugixml node().value() into std::string in ParseXML #234

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
87b2cd8
refactor
Jun 16, 2026
e87ae8e
simple
Jun 16, 2026
e70ec69
fix
Jun 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 31 additions & 23 deletions include/miniocpp/response.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,10 @@

#include <list>
#include <map>
#include <memory>
#include <pugixml.hpp>
#include <string>
#include <string_view>
#include <type_traits>
Comment thread
coderabbitai[bot] marked this conversation as resolved.

#include "error.h"
Expand Down Expand Up @@ -238,19 +241,21 @@ MINIO_S3_DERIVE_FROM_RESPONSE(DownloadObjectResponse)
MINIO_S3_DERIVE_FROM_RESPONSE(GetObjectResponse)

struct Item : public Response {
std::string etag; // except DeleteMarker
std::string name;
// etag and name stored in owning ListObjectsResponse's owned_.
std::string_view etag; // except DeleteMarker
std::string_view name;
utils::UtcTime last_modified;
std::string owner_id;
std::string owner_name;
// Fields below point into owning ListObjectsResponse's xml_document memory.
std::string_view owner_id;
std::string_view owner_name;
size_t size = 0; // except DeleteMarker
std::string storage_class;
bool is_latest = false; // except ListObjects V1/V2
std::string version_id; // except ListObjects V1/V2
std::string_view storage_class;
bool is_latest = false; // except ListObjects V1/V2
std::string_view version_id; // except ListObjects V1/V2
std::map<std::string, std::string> user_metadata;
bool is_prefix = false;
bool is_delete_marker = false;
std::string encoding_type;
std::string_view encoding_type;

Item() = default;

Expand All @@ -263,38 +268,41 @@ struct Item : public Response {

struct ListObjectsResponse : public Response {
// Common
std::string name;
std::string encoding_type;
std::string prefix;
std::string delimiter;
std::string_view name;
std::string_view encoding_type;
std::string_view prefix;
std::string_view delimiter;
bool is_truncated;
unsigned int max_keys;
std::list<Item> contents;

// Owned XML document for zero-copy string_view backing.
std::shared_ptr<pugi::xml_document> doc_;
// Owned strings for non-XML-assigned string_view backing storage.
std::list<std::string> owned_;

// ListObjectsV1
std::string marker;
std::string next_marker;
std::string_view marker;
std::string_view next_marker;

// ListObjectsV2
unsigned int key_count;
std::string start_after;
std::string continuation_token;
std::string next_continuation_token;
std::string_view start_after;
std::string_view continuation_token;
std::string_view next_continuation_token;

// ListObjectVersions
std::string key_marker;
std::string next_key_marker;
std::string version_id_marker;
std::string next_version_id_marker;
std::string_view key_marker;
std::string_view next_key_marker;
std::string_view version_id_marker;
std::string_view next_version_id_marker;

ListObjectsResponse() = default;

explicit ListObjectsResponse(error::Error err) : Response(std::move(err)) {}

explicit ListObjectsResponse(const Response& resp) : Response(resp) {}

~ListObjectsResponse() = default;

static ListObjectsResponse ParseXML(std::string_view data, bool version);
}; // struct ListObjectsResponse

Expand Down
97 changes: 58 additions & 39 deletions src/response.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ Response Response::ParseXML(std::string_view data, int status_code,
resp.headers = headers;

pugi::xml_document xdoc;
pugi::xml_parse_result result = xdoc.load_string(data.data());
pugi::xml_parse_result result = xdoc.load_buffer(data.data(), data.size());
if (!result) {
resp.err_ = error::Error("unable to parse XML; " + std::string(data));
return resp;
Expand Down Expand Up @@ -91,7 +91,7 @@ ListBucketsResponse ListBucketsResponse::ParseXML(std::string_view data) {
std::list<Bucket> buckets;

pugi::xml_document xdoc;
pugi::xml_parse_result result = xdoc.load_string(data.data());
pugi::xml_parse_result result = xdoc.load_buffer(data.data(), data.size());
if (!result) {
return error::make<ListBucketsResponse>("unable to parse XML");
}
Expand Down Expand Up @@ -119,7 +119,7 @@ CompleteMultipartUploadResponse CompleteMultipartUploadResponse::ParseXML(
CompleteMultipartUploadResponse resp;

pugi::xml_document xdoc;
pugi::xml_parse_result result = xdoc.load_string(data.data());
pugi::xml_parse_result result = xdoc.load_buffer(data.data(), data.size());
if (!result) {
return error::make<CompleteMultipartUploadResponse>("unable to parse XML");
}
Expand Down Expand Up @@ -160,17 +160,31 @@ ListObjectsResponse ListObjectsResponse::ParseXML(std::string_view data,
bool version) {
ListObjectsResponse resp;

pugi::xml_document xdoc;
pugi::xml_parse_result result = xdoc.load_string(data.data());
auto xdoc = std::make_shared<pugi::xml_document>();
pugi::xml_parse_result result = xdoc->load_buffer(data.data(), data.size());
if (!result) {
return error::make<ListObjectsResponse>("unable to parse XML");
}
resp.doc_ = std::move(xdoc);
std::string xpath = version ? "/ListVersionsResult" : "/ListBucketResult";

auto root = xdoc.select_node(xpath.c_str());
auto root = resp.doc_->select_node(xpath.c_str());

pugi::xpath_node text;
std::string value;
const char* raw = nullptr;

// Helper to store an unescaped string or point into doc memory.
auto assign_unescaped = [&resp](std::string_view encoding_type,
const char* raw,
std::string_view& target) -> void {
if (encoding_type == "url") {
resp.owned_.emplace_back(curlpp::unescape(raw));
target = resp.owned_.back();
} else {
target = raw;
}
};

text = root.node().select_node("Name/text()");
resp.name = text.node().value();
Expand All @@ -179,8 +193,8 @@ ListObjectsResponse ListObjectsResponse::ParseXML(std::string_view data,
resp.encoding_type = text.node().value();

text = root.node().select_node("Prefix/text()");
value = text.node().value();
resp.prefix = (resp.encoding_type == "url") ? curlpp::unescape(value) : value;
raw = text.node().value();
assign_unescaped(resp.encoding_type, raw, resp.prefix);

text = root.node().select_node("Delimiter/text()");
resp.delimiter = text.node().value();
Expand All @@ -196,14 +210,12 @@ ListObjectsResponse ListObjectsResponse::ParseXML(std::string_view data,
// ListBucketResult V1
{
text = root.node().select_node("Marker/text()");
value = text.node().value();
resp.marker =
(resp.encoding_type == "url") ? curlpp::unescape(value) : value;
raw = text.node().value();
assign_unescaped(resp.encoding_type, raw, resp.marker);

text = root.node().select_node("NextMarker/text()");
value = text.node().value();
resp.next_marker =
(resp.encoding_type == "url") ? curlpp::unescape(value) : value;
raw = text.node().value();
assign_unescaped(resp.encoding_type, raw, resp.next_marker);
}

// ListBucketResult V2
Expand All @@ -214,9 +226,8 @@ ListObjectsResponse ListObjectsResponse::ParseXML(std::string_view data,
resp.key_count = static_cast<unsigned>(std::stoul(value));

text = root.node().select_node("StartAfter/text()");
value = text.node().value();
resp.start_after =
(resp.encoding_type == "url") ? curlpp::unescape(value) : value;
raw = text.node().value();
assign_unescaped(resp.encoding_type, raw, resp.start_after);

text = root.node().select_node("ContinuationToken/text()");
resp.continuation_token = text.node().value();
Expand All @@ -228,14 +239,12 @@ ListObjectsResponse ListObjectsResponse::ParseXML(std::string_view data,
// ListVersionsResult
{
text = root.node().select_node("KeyMarker/text()");
value = text.node().value();
resp.key_marker =
(resp.encoding_type == "url") ? curlpp::unescape(value) : value;
raw = text.node().value();
assign_unescaped(resp.encoding_type, raw, resp.key_marker);

text = root.node().select_node("NextKeyMarker/text()");
value = text.node().value();
resp.next_key_marker =
(resp.encoding_type == "url") ? curlpp::unescape(value) : value;
raw = text.node().value();
assign_unescaped(resp.encoding_type, raw, resp.next_key_marker);

text = root.node().select_node("VersionIdMarker/text()");
resp.version_id_marker = text.node().value();
Expand All @@ -249,18 +258,24 @@ ListObjectsResponse ListObjectsResponse::ParseXML(std::string_view data,
auto populate = [&resp = resp, &last_item = last_item](
std::list<Item>& items, pugi::xpath_node_set& contents,
bool is_delete_marker) -> void {
const char* raw = nullptr;
for (auto content : contents) {
pugi::xpath_node text;
std::string value;
Item item;

text = content.node().select_node("ETag/text()");
item.etag = utils::Trim(text.node().value(), '"');
resp.owned_.emplace_back(utils::Trim(text.node().value(), '"'));
item.etag = resp.owned_.back();

text = content.node().select_node("Key/text()");
value = text.node().value();
item.name =
(resp.encoding_type == "url") ? curlpp::unescape(value) : value;
raw = text.node().value();
if (resp.encoding_type == "url") {
resp.owned_.emplace_back(curlpp::unescape(raw));
item.name = resp.owned_.back();
} else {
item.name = raw;
}

text = content.node().select_node("LastModified/text()");
value = text.node().value();
Expand Down Expand Up @@ -303,17 +318,21 @@ ListObjectsResponse ListObjectsResponse::ParseXML(std::string_view data,
populate(resp.contents, contents, false);
// Only for ListObjectsV1.
if (resp.is_truncated && resp.next_marker.empty()) {
resp.next_marker = last_item.name;
resp.owned_.emplace_back(last_item.name);
resp.next_marker = resp.owned_.back();
}

auto common_prefixes = root.node().select_nodes("CommonPrefixes");
for (auto common_prefix : common_prefixes) {
Item item;

text = common_prefix.node().select_node("Prefix/text()");
value = text.node().value();
item.name = (resp.encoding_type == "url") ? curlpp::unescape(value) : value;

raw = text.node().value();
if (resp.encoding_type == "url") {
resp.owned_.emplace_back(curlpp::unescape(raw));
item.name = resp.owned_.back();
} else {
item.name = raw;
}
item.is_prefix = true;

resp.contents.push_back(item);
Expand All @@ -329,7 +348,7 @@ RemoveObjectsResponse RemoveObjectsResponse::ParseXML(std::string_view data) {
RemoveObjectsResponse resp;

pugi::xml_document xdoc;
pugi::xml_parse_result result = xdoc.load_string(data.data());
pugi::xml_parse_result result = xdoc.load_buffer(data.data(), data.size());
if (!result) {
return error::make<RemoveObjectsResponse>("unable to parse XML");
}
Expand Down Expand Up @@ -386,7 +405,7 @@ GetBucketNotificationResponse GetBucketNotificationResponse::ParseXML(
NotificationConfig config;

pugi::xml_document xdoc;
pugi::xml_parse_result result = xdoc.load_string(data.data());
pugi::xml_parse_result result = xdoc.load_buffer(data.data(), data.size());
if (!result) {
return error::make<GetBucketNotificationResponse>("unable to parse XML");
}
Expand Down Expand Up @@ -481,7 +500,7 @@ GetBucketEncryptionResponse GetBucketEncryptionResponse::ParseXML(
SseConfig config;

pugi::xml_document xdoc;
pugi::xml_parse_result result = xdoc.load_string(data.data());
pugi::xml_parse_result result = xdoc.load_buffer(data.data(), data.size());
if (!result) {
return error::make<GetBucketEncryptionResponse>("unable to parse XML");
}
Expand All @@ -503,7 +522,7 @@ GetBucketReplicationResponse GetBucketReplicationResponse::ParseXML(
ReplicationConfig config;

pugi::xml_document xdoc;
pugi::xml_parse_result result = xdoc.load_string(data.data());
pugi::xml_parse_result result = xdoc.load_buffer(data.data(), data.size());
if (!result) {
return error::make<GetBucketReplicationResponse>("unable to parse XML");
}
Expand Down Expand Up @@ -669,7 +688,7 @@ GetBucketLifecycleResponse GetBucketLifecycleResponse::ParseXML(
LifecycleConfig config;

pugi::xml_document xdoc;
pugi::xml_parse_result result = xdoc.load_string(data.data());
pugi::xml_parse_result result = xdoc.load_buffer(data.data(), data.size());
if (!result) {
return error::make<GetBucketLifecycleResponse>("unable to parse XML");
}
Expand Down Expand Up @@ -796,7 +815,7 @@ GetBucketTagsResponse GetBucketTagsResponse::ParseXML(std::string_view data) {
std::map<std::string, std::string> map;

pugi::xml_document xdoc;
pugi::xml_parse_result result = xdoc.load_string(data.data());
pugi::xml_parse_result result = xdoc.load_buffer(data.data(), data.size());
if (!result) {
return error::make<GetBucketTagsResponse>("unable to parse XML");
}
Expand All @@ -821,7 +840,7 @@ GetObjectTagsResponse GetObjectTagsResponse::ParseXML(std::string_view data) {
std::map<std::string, std::string> map;

pugi::xml_document xdoc;
pugi::xml_parse_result result = xdoc.load_string(data.data());
pugi::xml_parse_result result = xdoc.load_buffer(data.data(), data.size());
if (!result) {
return error::make<GetObjectTagsResponse>("unable to parse XML");
}
Expand Down
Loading