Introduce support for move-only types in InlinedVector, and use move during resize.

Note: the previous code in Move seemed to use std::move, but without effect, since the argument was a const&.
This cl fixes the problem, shown by my test with a MoveOnly type.

From:
Benchmark                        Time(ns)    CPU(ns) Iterations
---------------------------------------------------------------
BM_InlinedVectorFillString/0            2          2  357702278
BM_InlinedVectorFillString/1           23         23   30020181  40.8M items/s
BM_InlinedVectorFillString/8          177        177    3938795  43.1M items/s
BM_InlinedVectorFillString/64        2515       2516     278714  24.3M items/s
BM_InlinedVectorFillString/512      21497      21478      31994  22.7M items/s
BM_InlinedVectorFillString/1k       43361      43312      16184  22.5M items/s

To:
Benchmark                        Time(ns)    CPU(ns) Iterations
---------------------------------------------------------------
BM_InlinedVectorFillString/0            2          2  357070962
BM_InlinedVectorFillString/1           23         23   31134543  42.0M items/s
BM_InlinedVectorFillString/8          166        166    4246010  45.9M items/s
BM_InlinedVectorFillString/64        1645       1644     427302  37.1M items/s
BM_InlinedVectorFillString/512      13830      13806      49712  35.4M items/s
BM_InlinedVectorFillString/1k       28164      28114      24588  34.7M items/s
Change: 130922812

Author: tensorflower-gardener

tensorflow/core/lib/gtl/inlined_vector.h

@@ -187,21 +187,29 @@ class InlinedVector {
     return at(0);
   }
 
-  // Append t to the vector.
+  // Append a T constructed with args to the vector.
   // Increases size() by one.
   // Amortized complexity: O(1)
   // Worst-case complexity: O(size())
-  inline void push_back(const value_type& t) {
+  template <typename... Args>
+  void emplace_back(Args&&... args) {
     size_t s = size();
     DCHECK_LE(s, capacity());
     if (s < capacity()) {
-      new (data() + s) T(t);
+      new (data() + s) T(std::forward<Args>(args)...);
       set_size_internal(s + 1);
     } else {
-      PushBackSlow(t);
+      EmplaceBackSlow(std::forward<Args>(args)...);
     }
   }
 
+  // Append t to the vector.
+  // Increases size() by one.
+  // Amortized complexity: O(1)
+  // Worst-case complexity: O(size())
+  void push_back(const value_type& t) { emplace_back(t); }
+  void push_back(value_type&& t) { emplace_back(std::move(t)); }
+
   inline void pop_back() {
     DCHECK(!empty());
     const size_t s = size();
@@ -348,16 +356,26 @@ class InlinedVector {
     }
   }
 
-  void PushBackSlow(const T& t) {
+  template <typename... Args>
+  void EmplaceBackSlow(Args&&... args) {
     const size_t s = size();
     DCHECK_EQ(s, capacity());
-    Grow<Move, Fill>(s + 1, &t);
+    Grow<Move, Construct>(s + 1, std::forward<Args>(args)...);
     set_size_internal(s + 1);
   }
 
+  // Movers for Grow
   // Does nothing.
-  static void Nop(const T* src, size_t n, T* dst) {}
+  static void Nop(T* src, size_t n, T* dst) {}
+
+  // Moves srcs[0,n-1] contents to dst[0,n-1].
+  static void Move(T* src, size_t n, T* dst) {
+    for (size_t i = 0; i < n; i++) {
+      new (dst + i) T(std::move(*(src + i)));
+    }
+  }
 
+  // Initializers for Resize.
   // Initializes dst[0,n-1] with empty constructor.
   static void ValueInit(const T*, size_t n, T* dst) {
     for (size_t i = 0; i < n; i++) {
@@ -372,13 +390,6 @@ class InlinedVector {
     }
   }
 
-  // Moves srcs[0,n-1] contents to dst[0,n-1].
-  static void Move(const T* src, size_t n, T* dst) {
-    for (size_t i = 0; i < n; i++) {
-      new (dst + i) T(std::move(*(src + i)));
-    }
-  }
-
   void Destroy(T* src, int n) {
     if (!std::is_trivially_destructible<T>::value) {
       for (int i = 0; i < n; i++) {
@@ -387,14 +398,28 @@ class InlinedVector {
     }
   }
 
+  // Initialization methods for Grow.
+  // 1) Leave uninitialized memory.
+  struct Uninitialized {
+    void operator()(T*) const {}
+  };
+  // 2) Construct a T with args at not-yet-initialized memory pointed by dst.
+  struct Construct {
+    template<class... Args>
+    void operator()(T* dst, Args&&... args) const {
+      new (dst) T(std::forward<Args>(args)...);
+    }
+  };
+
   // Grow so that capacity >= n.  Uses Mover to move existing elements
-  // to new buffer.  If elem is not-null, stores it at slot numbered
-  // size() before destroying the old buffer by calling Initializer.
-  // We pass the Initializer and Mover as template arguments so that
-  // this code compiles even if T does not support copying.
-  template <void(Mover)(const T*, size_t, T*),
-            void(Initializer)(const T*, size_t, T*) = Nop>
-  void Grow(size_t n, const T* elem = nullptr) {
+  // to new buffer, and possibly initialize the new element according
+  // to InitType.
+  // We pass the InitType and Mover as template arguments so that
+  // this code compiles even if T does not support copying or default
+  // construction.
+  template <void(Mover)(T*, size_t, T*), class InitType = Uninitialized,
+            class... Args>
+  void Grow(size_t n, Args&&... args) {
     size_t s = size();
     DCHECK_LE(s, capacity());
 
@@ -411,10 +436,7 @@ class InlinedVector {
     T* dst = static_cast<T*>(malloc(target * sizeof(T)));
 
     // Need to copy elem before discarding src since it might alias src.
-    if (elem) {
-      Initializer(elem, 1, dst + s);
-    }
-
+    InitType{}(dst + s, std::forward<Args>(args)...);
     Mover(src, s, dst);
     DiscardStorage();
 

tensorflow/core/lib/gtl/inlined_vector_test.cc

@@ -236,15 +236,20 @@ TEST(RefCountedVec, EraseBeginEnd) {
 }
 
 struct NoDefaultCtor {
-  explicit NoDefaultCtor(int /* x */) {}
+  explicit NoDefaultCtor(int) {}
 };
 struct NoCopy {
   NoCopy() {}
-  NoCopy(const NoCopy& /* x */) = delete;
+  NoCopy(const NoCopy&) = delete;
 };
 struct NoAssign {
   NoAssign() {}
-  NoAssign& operator=(const NoAssign& /* x */) = delete;
+  NoAssign& operator=(const NoAssign&) = delete;
+};
+struct MoveOnly {
+  MoveOnly() {}
+  MoveOnly(MoveOnly&&) = default;
+  MoveOnly& operator=(MoveOnly&&) = default;
 };
 TEST(InlinedVectorTest, NoDefaultCtor) {
   tensorflow::gtl::InlinedVector<NoDefaultCtor, 1> v(10, NoDefaultCtor(2));
@@ -258,6 +263,12 @@ TEST(InlinedVectorTest, NoAssign) {
   tensorflow::gtl::InlinedVector<NoAssign, 1> v(10);
   (void)v;
 }
+TEST(InlinedVectorTest, MoveOnly) {
+  gtl::InlinedVector<MoveOnly, 2> v;
+  v.push_back(MoveOnly{});
+  v.push_back(MoveOnly{});
+  v.push_back(MoveOnly{});
+}
 
 TEST(IntVec, Insert) {
   for (int len = 0; len < 20; len++) {
@@ -432,7 +443,7 @@ static std::vector<typename T::value_type> Vec(const T& src) {
 TEST(IntVec, SelfRefPushBack) {
   std::vector<string> std_v;
   tensorflow::gtl::InlinedVector<string, 4> v;
-  const string s = "A very long string to ensure heap.";
+  const string s = "A quite long string to ensure heap.";
   std_v.push_back(s);
   v.push_back(s);
   for (int i = 0; i < 20; ++i) {
@@ -444,6 +455,21 @@ TEST(IntVec, SelfRefPushBack) {
   EXPECT_EQ(std_v, Vec(v));
 }
 
+TEST(IntVec, SelfRefPushBackWithMove) {
+  std::vector<string> std_v;
+  gtl::InlinedVector<string, 4> v;
+  const string s = "A quite long string to ensure heap.";
+  std_v.push_back(s);
+  v.push_back(s);
+  for (int i = 0; i < 20; ++i) {
+    EXPECT_EQ(v.back(), std_v.back());
+
+    v.push_back(std::move(v.back()));
+    std_v.push_back(std::move(std_v.back()));
+  }
+  EXPECT_EQ(v.back(), std_v.back());
+}
+
 TEST(IntVec, Swap) {
   for (int l1 = 0; l1 < 20; l1++) {
     SCOPED_TRACE(l1);
@@ -733,7 +759,7 @@ static void BM_InlinedVectorFill(int iters, int len) {
       v.push_back(j);
     }
   }
-  testing::BytesProcessed((static_cast<int64>(iters) * len) * sizeof(int));
+  testing::BytesProcessed((int64{iters} * len) * sizeof(int));
 }
 BENCHMARK(BM_InlinedVectorFill)->Range(0, 1024);
 
@@ -745,7 +771,7 @@ static void BM_InlinedVectorFillRange(int iters, int len) {
   for (int i = 0; i < iters; i++) {
     IntVec TF_ATTRIBUTE_UNUSED v(ia.get(), ia.get() + len);
   }
-  testing::BytesProcessed((static_cast<int64>(iters) * len) * sizeof(int));
+  testing::BytesProcessed((int64{iters} * len) * sizeof(int));
 }
 BENCHMARK(BM_InlinedVectorFillRange)->Range(0, 1024);
 
@@ -756,10 +782,46 @@ static void BM_StdVectorFill(int iters, int len) {
       v.push_back(j);
     }
   }
-  testing::BytesProcessed((static_cast<int64>(iters) * len) * sizeof(int));
+  testing::BytesProcessed((int64{iters} * len) * sizeof(int));
 }
 BENCHMARK(BM_StdVectorFill)->Range(0, 1024);
 
+bool StringRepresentedInline(string s) {
+  const char* chars = s.data();
+  string s1 = std::move(s);
+  return s1.data() != chars;
+}
+
+static void BM_InlinedVectorFillString(int iters, int len) {
+  string strings[4] = {"a quite long string", "another long string",
+                       "012345678901234567", "to cause allocation"};
+  for (int i = 0; i < iters; i++) {
+    gtl::InlinedVector<string, 8> v;
+    for (int j = 0; j < len; j++) {
+      v.push_back(strings[j & 3]);
+    }
+  }
+  testing::ItemsProcessed(int64{iters} * len);
+}
+BENCHMARK(BM_InlinedVectorFillString)->Range(0, 1024);
+
+static void BM_StdVectorFillString(int iters, int len) {
+  string strings[4] = {"a quite long string", "another long string",
+                       "012345678901234567", "to cause allocation"};
+  for (int i = 0; i < iters; i++) {
+    std::vector<string> v;
+    for (int j = 0; j < len; j++) {
+      v.push_back(strings[j & 3]);
+    }
+  }
+  testing::ItemsProcessed(int64{iters} * len);
+  // The purpose of the benchmark is to verify that inlined vector is
+  // efficient when moving is more efficent than copying. To do so, we
+  // use strings that are larger than the small string optimization.
+  CHECK(!StringRepresentedInline(strings[0]));
+}
+BENCHMARK(BM_StdVectorFillString)->Range(0, 1024);
+
 namespace {
 struct Buffer {  // some arbitrary structure for benchmarking.
   char* base;