Skip to content

Commit b2919fa

Browse files
authored
[native] Save 1 memcpy in parsing message body (#24941)
## Description In our HTTP callbacks, we parse the vector of iobufs into a string; however, we assume that each iobuf is singularly chained. This is not guaranteed to be the case. This diff resolves two issues: - Correctly consumes the vector of iobufs by taking each buffer chain into account - Saves 1 memcopy. The previous code path would copy each iobuf into a stream, and then use.c_str(), which also is a copy ([https://en.cppreference.com/w/cpp/io/basic_ostringstream/str](https://l.facebook.com/l.php?u=https%3A%2F%2Fen.cppreference.com%2Fw%2Fcpp%2Fio%2Fbasic_ostringstream%2Fstr&h=AT2RH-TyloQE5MPfdcxYizn__by-8eZd_7-9saGWz05YOZfunpxrYWFq0lHU6WVhx1zpPwW_rnjI9dtanJo1HqBKf5SDv96S4UZBgEOE02Jzb2MevW9chO9bBLeyNU4g7pEs9yMea4EnPqVa9h_K4QrRx_c)). This diff will just copy once into the string buffer ## Test Plan Added Unit test ## Contributor checklist - [X] Please make sure your submission complies with our [contributing guide](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md), in particular [code style](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#code-style) and [commit standards](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#commit-standards). - [ ] PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced. - [ ] Documented new properties (with its default value), SQL syntax, functions, or other functionality. - [ ] If release notes are required, they follow the [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines). - [ ] Adequate tests were added if applicable. - [ ] CI passed.
1 parent e76f5a5 commit b2919fa

File tree

2 files changed

+27
-6
lines changed

2 files changed

+27
-6
lines changed

presto-native-execution/presto_cpp/main/common/Utils.cpp

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414

1515
#include "presto_cpp/main/common/Utils.h"
1616
#include <fmt/format.h>
17+
#include <folly/io/Cursor.h>
1718
#include <sys/resource.h>
1819
#include "velox/common/process/ThreadDebugInfo.h"
1920

@@ -66,12 +67,20 @@ void installSignalHandler() {
6667

6768
std::string extractMessageBody(
6869
const std::vector<std::unique_ptr<folly::IOBuf>>& body) {
69-
// TODO Avoid copy
70-
std::ostringstream oss;
71-
for (auto& buf : body) {
72-
oss << std::string(
73-
reinterpret_cast<const char*>(buf->data()), buf->length());
70+
std::string ret;
71+
size_t bodySize = 0;
72+
for (const auto& buf : body) {
73+
bodySize += buf->computeChainDataLength();
7474
}
75-
return oss.str();
75+
ret.resize(bodySize);
76+
77+
size_t offset = 0;
78+
for (const auto& buf : body) {
79+
folly::io::Cursor cursor(buf.get());
80+
size_t chainLength = buf->computeChainDataLength();
81+
cursor.pull(ret.data() + offset, chainLength);
82+
offset += chainLength;
83+
}
84+
return ret;
7685
}
7786
} // namespace facebook::presto::util

presto-native-execution/presto_cpp/main/common/tests/CommonTest.cpp

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,3 +133,15 @@ TEST(UtilsTest, general) {
133133
EXPECT_EQ("2021-05-20T19:18:27.001Z", util::toISOTimestamp(1621538307001l));
134134
EXPECT_EQ("2021-05-20T19:18:27.000Z", util::toISOTimestamp(1621538307000l));
135135
}
136+
137+
TEST(UtilsTest, extractMessageBody) {
138+
std::vector<std::unique_ptr<folly::IOBuf>> body;
139+
body.push_back(folly::IOBuf::copyBuffer("body1"));
140+
body.push_back(folly::IOBuf::copyBuffer("body2"));
141+
body.push_back(folly::IOBuf::copyBuffer("body3"));
142+
auto iobuf = folly::IOBuf::copyBuffer("body4");
143+
iobuf->appendToChain(folly::IOBuf::copyBuffer("body5"));
144+
body.push_back(std::move(iobuf));
145+
auto messageBody = util::extractMessageBody(body);
146+
EXPECT_EQ(messageBody, "body1body2body3body4body5");
147+
}

0 commit comments

Comments
 (0)