stop prereserving space in toAppend

Summary:
previous change would cause reserving
as much as needed in toAppend for all arguements.
This had bad consequences for appending in a loop,
described here: https://phabricator.fb.com/D1420588#22
Not split the interfaces so that user has to decide

Test Plan: unit tests

Reviewed By: soren@fb.com

Subscribers: trunkagent, njormrod

FB internal diff: D1601614

Tasks: 5303991

diff --git a/folly/Conv.h b/folly/Conv.h
index 4ed4139423ccd3130d229cae8f21f8ef255a8a2e..fd418107aa95b4608ecd2bc0c8f495c4278707fc 100644 (file)
--- a/folly/Conv.h
+++ b/folly/Conv.h
@@ -717,8 +717,10 @@ toAppendDelimStrImpl(const Delimiter& delim, const T& v, const Ts&... vs) {
 
 /**
  * Variadic conversion to string. Appends each element in turn.
- * If we have two or more things to append, we will reserve
- * the space for them (at least we will try).
+ * If we have two or more things to append, we it will not reserve
+ * the space for them and will depend on strings exponential growth.
+ * If you just append once consider using toAppendFit which reserves
+ * the space needed (but does not have exponential as a result).
  */
 template <class... Ts>
 typename std::enable_if<sizeof...(Ts) >= 3
@@ -727,10 +729,32 @@ typename std::enable_if<sizeof...(Ts) >= 3
     typename detail::last_element<Ts...>::type
   >::type>::value>::type
 toAppend(const Ts&... vs) {
-  detail::reserveInTarget(vs...);
   detail::toAppendStrImpl(vs...);
 }
 
+/**
+ * Special version of the call that preallocates exaclty as much memory
+ * as need for arguments to be stored in target. This means we are
+ * not doing exponential growth when we append. If you are using it
+ * in a loop you are aiming at your foot with a big perf-destroying
+ * bazooka.
+ * On the other hand if you are appending to a string once, this
+ * will probably save a few calls to malloc.
+ */
+template <class... Ts>
+typename std::enable_if<
+  IsSomeString<
+  typename std::remove_pointer<
+    typename detail::last_element<Ts...>::type
+  >::type>::value>::type
+toAppendFit(const Ts&... vs) {
+  detail::reserveInTarget(vs...);
+  toAppend(vs...);
+}
+
+template <class Ts>
+void toAppendFit(const Ts&) {}
+
 /**
  * Variadic base case: do nothing.
  */
@@ -757,7 +781,8 @@ toAppendDelim(const Delimiter& delim, const T& v, Tgt* tgt) {
 }
 
 /**
- * Append to string with a delimiter in between elements.
+ * Append to string with a delimiter in between elements. Check out
+ * comments for toAppend for details about memory allocation.
  */
 template <class Delimiter, class... Ts>
 typename std::enable_if<sizeof...(Ts) >= 3
@@ -766,10 +791,26 @@ typename std::enable_if<sizeof...(Ts) >= 3
     typename detail::last_element<Ts...>::type
   >::type>::value>::type
 toAppendDelim(const Delimiter& delim, const Ts&... vs) {
-  detail::reserveInTargetDelim(delim, vs...);
   detail::toAppendDelimStrImpl(delim, vs...);
 }
 
+/**
+ * Detail in comment for toAppendFit
+ */
+template <class Delimiter, class... Ts>
+typename std::enable_if<
+  IsSomeString<
+  typename std::remove_pointer<
+    typename detail::last_element<Ts...>::type
+  >::type>::value>::type
+toAppendDelimFit(const Delimiter& delim, const Ts&... vs) {
+  detail::reserveInTargetDelim(delim, vs...);
+  toAppendDelim(delim, vs...);
+}
+
+template <class De, class Ts>
+void toAppendDelimFit(const De&, const Ts&) {}
+
 /**
  * to<SomeString>(SomeString str) returns itself. As both std::string and
  * folly::fbstring use Copy-on-Write, it's much more efficient by
@@ -795,7 +836,7 @@ typename std::enable_if<
   Tgt>::type
 to(const Ts&... vs) {
   Tgt result;
-  toAppend(vs..., &result);
+  toAppendFit(vs..., &result);
   return result;
 }
 
@@ -822,7 +863,7 @@ typename std::enable_if<
   Tgt>::type
 toDelim(const Delim& delim, const Ts&... vs) {
   Tgt result;
-  toAppendDelim(delim, vs..., &result);
+  toAppendDelimFit(delim, vs..., &result);
   return result;
 }
 

diff --git a/folly/test/ConvTest.cpp b/folly/test/ConvTest.cpp
index a52869ea6967dd3e61a4e793699553030629a167..d15b638da82ced1f1b2bc70c7f8177357e929cf5 100644 (file)
--- a/folly/test/ConvTest.cpp
+++ b/folly/test/ConvTest.cpp
@@ -748,6 +748,22 @@ TEST(Conv, NewUint64ToString) {
 #undef THE_GREAT_EXPECTATIONS
 }
 
+TEST(Conv, allocate_size) {
+  std::string str1 = "meh meh meh";
+  std::string str2 = "zdech zdech zdech";
+
+  auto res1 = folly::to<std::string>(str1, ".", str2);
+  EXPECT_EQ(res1, str1 + "." + str2);
+
+  std::string res2; //empty
+  toAppendFit(str1, str2, 1, &res2);
+  EXPECT_EQ(res2, str1 + str2 + "1");
+
+  std::string res3;
+  toAppendDelimFit(",", str1, str2, &res3);
+  EXPECT_EQ(res3, str1 + "," + str2);
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // Benchmarks for ASCII to int conversion
 ////////////////////////////////////////////////////////////////////////////////