From 70d6b0b6ef1f09c63ab51ceb15cc843024cf4ffe Mon Sep 17 00:00:00 2001 From: Chip Turner Date: Wed, 10 Dec 2014 22:14:09 -0800 Subject: [PATCH] Improve performance of stringPrintf and related functions Summary: It turned out at least one optimization we were doing for stringPrintf (using the tail of the input buffer) was causing a performance degradation in some cases -- if the string was already pre-sized, our resize() call would end up memset'ing the tail. In some instances, this could cause significant performance penalties, such as when multiple stringAppendf calls were made. So, this diff removes the "optimization" around using the tail of the input buffer and instead uses a standalone stack buffer. If vsnprintf deems that buffer too small, a heap buffer is instead used. As there is no std::string method that resizes the string to fill the underlying buffer without setting the values to a default, and as it is not legal to write past the end of the data buffer (even if capacity() says there is enough), let's just abandon that optimization. It turns out this doesn't have a major performance loss for most cases since, with this diff, most small strings will fit on-stack and then hopefully in the string's tail anyway. Test Plan: runtests Reviewed By: simpkins@fb.com Subscribers: trunkagent, net-systems@, lins, anca, folly-diffs@ FB internal diff: D1733774 Tasks: 5735468 Signature: t1:1733774:1418323943:ec47007c9756aca6cf0466bce92722ac4c7e89b2 --- folly/String.cpp | 98 +++++++++++++++++++-------------------- folly/test/StringTest.cpp | 10 ++-- 2 files changed, 55 insertions(+), 53 deletions(-) diff --git a/folly/String.cpp b/folly/String.cpp index ce50ae1a..420cf2a1 100644 --- a/folly/String.cpp +++ b/folly/String.cpp @@ -31,48 +31,57 @@ namespace folly { namespace { -inline void stringPrintfImpl(std::string& output, const char* format, - va_list args) { - // Tru to the space at the end of output for our output buffer. - // Find out write point then inflate its size temporarily to its - // capacity; we will later shrink it to the size needed to represent - // the formatted string. If this buffer isn't large enough, we do a - // resize and try again. - - const auto write_point = output.size(); - auto remaining = output.capacity() - write_point; - output.resize(output.capacity()); - +int stringAppendfImplHelper(char* buf, + size_t bufsize, + const char* format, + va_list args) { va_list args_copy; va_copy(args_copy, args); - int bytes_used = vsnprintf(&output[write_point], remaining, format, - args_copy); + int bytes_used = vsnprintf(buf, bufsize, format, args_copy); va_end(args_copy); + return bytes_used; +} + +void stringAppendfImpl(std::string& output, const char* format, va_list args) { + // Very simple; first, try to avoid an allocation by using an inline + // buffer. If that fails to hold the output string, allocate one on + // the heap, use it instead. + // + // It is hard to guess the proper size of this buffer; some + // heuristics could be based on the number of format characters, or + // static analysis of a codebase. Or, we can just pick a number + // that seems big enough for simple cases (say, one line of text on + // a terminal) without being large enough to be concerning as a + // stack variable. + std::array inline_buffer; + + int bytes_used = stringAppendfImplHelper( + inline_buffer.data(), inline_buffer.size(), format, args); if (bytes_used < 0) { - throw std::runtime_error( - to("Invalid format string; snprintf returned negative " - "with format string: ", format)); - } else if (size_t(bytes_used) < remaining) { - // There was enough room, just shrink and return. - output.resize(write_point + bytes_used); - } else { - output.resize(write_point + bytes_used + 1); - remaining = bytes_used + 1; - va_list args_copy; - va_copy(args_copy, args); - bytes_used = vsnprintf(&output[write_point], remaining, format, - args_copy); - va_end(args_copy); - if (size_t(bytes_used) + 1 != remaining) { - throw std::runtime_error( - to("vsnprint retry did not manage to work " - "with format string: ", format)); - } - output.resize(write_point + bytes_used); + throw std::runtime_error(to( + "Invalid format string; snprintf returned negative " + "with format string: ", + format)); + } + + if (static_cast(bytes_used) < inline_buffer.size()) { + output.append(inline_buffer.data(), bytes_used); + return; } + + // Couldn't fit. Heap allocate a buffer, oh well. + std::unique_ptr heap_buffer(new char[bytes_used + 1]); + int final_bytes_used = + stringAppendfImplHelper(heap_buffer.get(), bytes_used + 1, format, args); + // The second call should require the same length, which is 1 less + // than the buffer size (we don't keep the trailing \0 byte in our + // output string). + CHECK(bytes_used == final_bytes_used); + + output.append(heap_buffer.get(), bytes_used); } -} // anon namespace +} // anon namespace std::string stringPrintf(const char* format, ...) { va_list ap; @@ -84,19 +93,8 @@ std::string stringPrintf(const char* format, ...) { } std::string stringVPrintf(const char* format, va_list ap) { - // snprintf will tell us how large the output buffer should be, but - // we then have to call it a second time, which is costly. By - // guestimating the final size, we avoid the double snprintf in many - // cases, resulting in a performance win. We use this constructor - // of std::string to avoid a double allocation, though it does pad - // the resulting string with nul bytes. Our guestimation is twice - // the format string size, or 32 bytes, whichever is larger. This - // is a hueristic that doesn't affect correctness but attempts to be - // reasonably fast for the most common cases. - std::string ret(std::max(size_t(32), strlen(format) * 2), '\0'); - ret.resize(0); - - stringPrintfImpl(ret, format, ap); + std::string ret; + stringAppendfImpl(ret, format, ap); return ret; } @@ -114,7 +112,7 @@ std::string& stringAppendf(std::string* output, const char* format, ...) { std::string& stringVAppendf(std::string* output, const char* format, va_list ap) { - stringPrintfImpl(*output, format, ap); + stringAppendfImpl(*output, format, ap); return *output; } @@ -129,7 +127,7 @@ void stringPrintf(std::string* output, const char* format, ...) { void stringVPrintf(std::string* output, const char* format, va_list ap) { output->clear(); - stringPrintfImpl(*output, format, ap); + stringAppendfImpl(*output, format, ap); }; namespace { diff --git a/folly/test/StringTest.cpp b/folly/test/StringTest.cpp index 80b9a30d..93b0fb6e 100644 --- a/folly/test/StringTest.cpp +++ b/folly/test/StringTest.cpp @@ -112,10 +112,14 @@ TEST(StringPrintf, VPrintf) { } TEST(StringPrintf, VariousSizes) { - // Test a wide variety of output sizes - for (int i = 0; i < 100; ++i) { + // Test a wide variety of output sizes, making sure to cross the + // vsnprintf buffer boundary implementation detail. + for (int i = 0; i < 4096; ++i) { string expected(i + 1, 'a'); - EXPECT_EQ("X" + expected + "X", stringPrintf("X%sX", expected.c_str())); + expected = "X" + expected + "X"; + string result = stringPrintf("%s", expected.c_str()); + EXPECT_EQ(expected.size(), result.size()); + EXPECT_EQ(expected, result); } EXPECT_EQ("abc12345678910111213141516171819202122232425xyz", -- 2.34.1