From: Mark Logan <mlogan@fb.com>
Date: Fri, 3 Feb 2017 18:09:52 +0000 (-0800)
Subject: Fix worst-case quadratic behavior of iterator constructors and range insert()
X-Git-Tag: v2017.03.06.00~59
X-Git-Url: http://plrg.eecs.uci.edu/git/?a=commitdiff_plain;h=3ffcb010fb7333a9ad6b233b47e1ecfdfb0fc2f0;p=folly.git

Fix worst-case quadratic behavior of iterator constructors and range insert()

Summary:
The iterator constructors and the range insert() method previously
had quadratic runtime if given an unsorted range. This is now fixed. We
append the entire range to the container, sort that subrange, and merge it
into the already-sorted container. Sorting and merging is skipped when possible.

Reviewed By: yfeldblum

Differential Revision: D4493534

fbshipit-source-id: e6d73b5c19e374001f9e340ff527c5257bef2ca3
---

diff --git a/folly/sorted_vector_types.h b/folly/sorted_vector_types.h
index f7b0293b..5d8d5344 100644
--- a/folly/sorted_vector_types.h
+++ b/folly/sorted_vector_types.h
@@ -145,6 +145,34 @@ namespace detail {
     return hint;
   }
 
+  template <class OurContainer, class Vector, class InputIterator>
+  void bulk_insert(
+      OurContainer& sorted,
+      Vector& cont,
+      InputIterator first,
+      InputIterator last) {
+    // prevent deref of middle where middle == cont.end()
+    if (first == last) {
+      return;
+    }
+
+    auto const& cmp(sorted.value_comp());
+
+    int const d = distance_if_multipass(first, last);
+    if (d != -1) {
+      cont.reserve(cont.size() + d);
+    }
+    auto const prev_size = cont.size();
+
+    std::copy(first, last, std::back_inserter(cont));
+    auto const middle = cont.begin() + prev_size;
+    if (!std::is_sorted(middle, cont.end(), cmp)) {
+      std::sort(middle, cont.end(), cmp);
+    }
+    if (middle != cont.begin() && cmp(*middle, *(middle - 1))) {
+      std::inplace_merge(cont.begin(), middle, cont.end(), cmp);
+    }
+  }
 }
 
 //////////////////////////////////////////////////////////////////////
@@ -271,13 +299,7 @@ public:
 
   template<class InputIterator>
   void insert(InputIterator first, InputIterator last) {
-    int d = detail::distance_if_multipass(first, last);
-    if (d != -1) {
-      m_.cont_.reserve(m_.cont_.size() + d);
-    }
-    for (; first != last; ++first) {
-      insert(end(), *first);
-    }
+    detail::bulk_insert(*this, m_.cont_, first, last);
   }
 
   size_type erase(const key_type& key) {
@@ -514,13 +536,7 @@ public:
 
   template<class InputIterator>
   void insert(InputIterator first, InputIterator last) {
-    int d = detail::distance_if_multipass(first, last);
-    if (d != -1) {
-      m_.cont_.reserve(m_.cont_.size() + d);
-    }
-    for (; first != last; ++first) {
-      insert(end(), *first);
-    }
+    detail::bulk_insert(*this, m_.cont_, first, last);
   }
 
   size_type erase(const key_type& key) {
diff --git a/folly/test/sorted_vector_test.cpp b/folly/test/sorted_vector_test.cpp
index fede251b..474d7e96 100644
--- a/folly/test/sorted_vector_test.cpp
+++ b/folly/test/sorted_vector_test.cpp
@@ -16,9 +16,11 @@
 
 #include <folly/sorted_vector_types.h>
 
+#include <iterator>
 #include <list>
 #include <memory>
 
+#include <folly/portability/GMock.h>
 #include <folly/portability/GTest.h>
 
 using folly::sorted_vector_set;
@@ -347,3 +349,158 @@ TEST(SortedVectorTypes, EraseTest) {
   EXPECT_EQ(0, s1.erase(0));
   EXPECT_EQ(s2, s1);
 }
+
+std::vector<int> extractValues(sorted_vector_set<CountCopyCtor> const& in) {
+  std::vector<int> ret;
+  std::transform(
+      in.begin(),
+      in.end(),
+      std::back_inserter(ret),
+      [](const CountCopyCtor& c) { return c.val_; });
+  return ret;
+}
+
+template <typename T, typename S>
+std::vector<T> makeVectorOfWrappers(std::vector<S> ss) {
+  std::vector<T> ts;
+  ts.reserve(ss.size());
+  for (auto const& s : ss) {
+    ts.emplace_back(s);
+  }
+  return ts;
+}
+
+TEST(SortedVectorTypes, TestSetBulkInsertionSortMerge) {
+  auto s = makeVectorOfWrappers<CountCopyCtor, int>({6, 4, 8, 2});
+
+  sorted_vector_set<CountCopyCtor> vset(s.begin(), s.end());
+  check_invariant(vset);
+
+  // Add an unsorted range that will have to be merged in.
+  s = makeVectorOfWrappers<CountCopyCtor, int>({10, 7, 5, 1});
+
+  vset.insert(s.begin(), s.end());
+  check_invariant(vset);
+  EXPECT_EQ(vset.rbegin()->count_, 1);
+
+  EXPECT_THAT(
+      extractValues(vset),
+      testing::ElementsAreArray({1, 2, 4, 5, 6, 7, 8, 10}));
+}
+
+TEST(SortedVectorTypes, TestSetBulkInsertionSortNoMerge) {
+  auto s = makeVectorOfWrappers<CountCopyCtor, int>({6, 4, 8, 2});
+
+  sorted_vector_set<CountCopyCtor> vset(s.begin(), s.end());
+  check_invariant(vset);
+
+  // Add an unsorted range that will not have to be merged in.
+  s = makeVectorOfWrappers<CountCopyCtor, int>({20, 15, 16, 13});
+
+  vset.insert(s.begin(), s.end());
+  check_invariant(vset);
+  EXPECT_EQ(vset.rbegin()->count_, 1);
+  EXPECT_THAT(
+      extractValues(vset),
+      testing::ElementsAreArray({2, 4, 6, 8, 13, 15, 16, 20}));
+}
+
+TEST(SortedVectorTypes, TestSetBulkInsertionNoSortMerge) {
+  auto s = makeVectorOfWrappers<CountCopyCtor, int>({6, 4, 8, 2});
+
+  sorted_vector_set<CountCopyCtor> vset(s.begin(), s.end());
+  check_invariant(vset);
+
+  // Add a sorted range that will have to be merged in.
+  s = makeVectorOfWrappers<CountCopyCtor, int>({1, 3, 5, 9});
+
+  vset.insert(s.begin(), s.end());
+  check_invariant(vset);
+  EXPECT_EQ(vset.rbegin()->count_, 1);
+  EXPECT_THAT(
+      extractValues(vset), testing::ElementsAreArray({1, 2, 3, 4, 5, 6, 8, 9}));
+}
+
+TEST(SortedVectorTypes, TestSetBulkInsertionNoSortNoMerge) {
+  auto s = makeVectorOfWrappers<CountCopyCtor, int>({6, 4, 8, 2});
+
+  sorted_vector_set<CountCopyCtor> vset(s.begin(), s.end());
+  check_invariant(vset);
+
+  // Add a sorted range that will not have to be merged in.
+  s = makeVectorOfWrappers<CountCopyCtor, int>({21, 22, 23, 24});
+
+  vset.insert(s.begin(), s.end());
+  check_invariant(vset);
+  EXPECT_EQ(vset.rbegin()->count_, 1);
+  EXPECT_THAT(
+      extractValues(vset),
+      testing::ElementsAreArray({2, 4, 6, 8, 21, 22, 23, 24}));
+}
+
+TEST(SortedVectorTypes, TestSetBulkInsertionEmptyRange) {
+  std::vector<CountCopyCtor> s;
+  EXPECT_TRUE(s.empty());
+
+  // insertion of empty range into empty container.
+  sorted_vector_set<CountCopyCtor> vset(s.begin(), s.end());
+  check_invariant(vset);
+
+  s = makeVectorOfWrappers<CountCopyCtor, int>({6, 4, 8, 2});
+
+  vset.insert(s.begin(), s.end());
+
+  // insertion of empty range into non-empty container.
+  s.clear();
+  vset.insert(s.begin(), s.end());
+  check_invariant(vset);
+
+  EXPECT_THAT(extractValues(vset), testing::ElementsAreArray({2, 4, 6, 8}));
+}
+
+// This is a test of compilation - the behavior has already been tested
+// extensively above.
+TEST(SortedVectorTypes, TestBulkInsertionUncopyableTypes) {
+  std::vector<std::pair<int, std::unique_ptr<int>>> s;
+  s.emplace_back(1, std::make_unique<int>(0));
+
+  sorted_vector_map<int, std::unique_ptr<int>> vmap(
+      std::make_move_iterator(s.begin()), std::make_move_iterator(s.end()));
+
+  s.clear();
+  s.emplace_back(3, std::make_unique<int>(0));
+  vmap.insert(
+      std::make_move_iterator(s.begin()), std::make_move_iterator(s.end()));
+}
+
+// A moveable and copyable struct, which we use to make sure that no copy
+// operations are performed during bulk insertion if moving is an option.
+struct Movable {
+  int x_;
+  explicit Movable(int x) : x_(x) {}
+  Movable(const Movable&) {
+    ADD_FAILURE() << "Copy ctor should not be called";
+  }
+  Movable& operator=(const Movable&) {
+    ADD_FAILURE() << "Copy assignment should not be called";
+    return *this;
+  }
+
+  Movable(Movable&&) = default;
+  Movable& operator=(Movable&&) = default;
+};
+
+TEST(SortedVectorTypes, TestBulkInsertionMovableTypes) {
+  std::vector<std::pair<int, Movable>> s;
+  s.emplace_back(3, Movable(2));
+  s.emplace_back(1, Movable(0));
+
+  sorted_vector_map<int, Movable> vmap(
+      std::make_move_iterator(s.begin()), std::make_move_iterator(s.end()));
+
+  s.clear();
+  s.emplace_back(4, Movable(3));
+  s.emplace_back(2, Movable(1));
+  vmap.insert(
+      std::make_move_iterator(s.begin()), std::make_move_iterator(s.end()));
+}